Skip to content

[PlaygroundLogger] Implemented a check to avoid trying to log nil-con… #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 3, 2020
Merged

[PlaygroundLogger] Implemented a check to avoid trying to log nil-con… #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions PlaygroundLogger/PlaygroundLogger.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
objects = {

/* Begin PBXBuildFile section */
5E1069A323CFD68100C79FC2 /* PGLNilObject.m in Sources */ = {isa = PBXBuildFile; fileRef = 5E1069A223CFD68100C79FC2 /* PGLNilObject.m */; };
5E1069A423CFD68100C79FC2 /* PGLNilObject.m in Sources */ = {isa = PBXBuildFile; fileRef = 5E1069A223CFD68100C79FC2 /* PGLNilObject.m */; };
5E1069A523CFD68100C79FC2 /* PGLNilObject.m in Sources */ = {isa = PBXBuildFile; fileRef = 5E1069A223CFD68100C79FC2 /* PGLNilObject.m */; };
5E184715202BB72700F01AD1 /* TypeName.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5E184714202BB72700F01AD1 /* TypeName.swift */; };
5E184718202BB80200F01AD1 /* PGLConcurrentMap.h in Headers */ = {isa = PBXBuildFile; fileRef = 5E184716202BB80200F01AD1 /* PGLConcurrentMap.h */; settings = {ATTRIBUTES = (Public, ); }; };
5E184719202BB80200F01AD1 /* PGLConcurrentMap_MRR.m in Sources */ = {isa = PBXBuildFile; fileRef = 5E184717202BB80200F01AD1 /* PGLConcurrentMap_MRR.m */; settings = {COMPILER_FLAGS = "-fno-objc-arc"; }; };
Expand Down Expand Up @@ -192,6 +195,9 @@
/* End PBXCopyFilesBuildPhase section */

/* Begin PBXFileReference section */
5E10699E23CFD68100C79FC2 /* PlaygroundLoggerTests-Bridging-Header.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "PlaygroundLoggerTests-Bridging-Header.h"; sourceTree = "<group>"; };
5E1069A123CFD68100C79FC2 /* PGLNilObject.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = PGLNilObject.h; sourceTree = "<group>"; };
5E1069A223CFD68100C79FC2 /* PGLNilObject.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = PGLNilObject.m; sourceTree = "<group>"; };
5E11805920414E2700B73EE9 /* Debug.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; name = Debug.xcconfig; path = ../XcodeConfig/Debug.xcconfig; sourceTree = "<group>"; };
5E11805A20414E2700B73EE9 /* Release.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; name = Release.xcconfig; path = ../XcodeConfig/Release.xcconfig; sourceTree = "<group>"; };
5E184714202BB72700F01AD1 /* TypeName.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TypeName.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -416,6 +422,9 @@
5EB651C3213A081A001CC984 /* LoggerEntrypointTests.swift */,
5EB651BF213A01D0001CC984 /* LegacyEntrypointTests.swift */,
5ECE8F901FFCD2A70034D9BC /* LegacyPlaygroundLoggerTests.swift */,
5E1069A123CFD68100C79FC2 /* PGLNilObject.h */,
5E1069A223CFD68100C79FC2 /* PGLNilObject.m */,
5E10699E23CFD68100C79FC2 /* PlaygroundLoggerTests-Bridging-Header.h */,
);
path = PlaygroundLoggerTests;
sourceTree = "<group>";
Expand Down Expand Up @@ -814,12 +823,12 @@
};
5E26462F1FB64876002DC6B6 = {
CreatedOnToolsVersion = 9.1;
LastSwiftMigration = 1010;
LastSwiftMigration = 1130;
ProvisioningStyle = Automatic;
};
5EFE9188203F6CC400E21BAA = {
CreatedOnToolsVersion = 9.3;
LastSwiftMigration = 1010;
LastSwiftMigration = 1130;
ProvisioningStyle = Automatic;
TestTargetID = 5EFE9197203F6DD700E21BAA;
};
Expand All @@ -835,7 +844,7 @@
};
5EFE91CC203F6F6900E21BAA = {
CreatedOnToolsVersion = 9.3;
LastSwiftMigration = 1010;
LastSwiftMigration = 1130;
ProvisioningStyle = Automatic;
TestTargetID = 5EFE91AF203F6E8D00E21BAA;
};
Expand Down Expand Up @@ -1000,6 +1009,7 @@
5E5D77802040BD7D00EBC3A9 /* LogPolicyTests.swift in Sources */,
5E48E0AA2042925B00C7712D /* LogEntryTests.swift in Sources */,
5EF581532041387C00AC14FE /* CustomPlaygroundDisplayConvertibleTests.swift in Sources */,
5E1069A323CFD68100C79FC2 /* PGLNilObject.m in Sources */,
5ECE8F911FFCD2A70034D9BC /* LegacyPlaygroundLoggerTests.swift in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand All @@ -1013,6 +1023,7 @@
5E5D77812040BD7D00EBC3A9 /* LogPolicyTests.swift in Sources */,
5E48E0AB2042925B00C7712D /* LogEntryTests.swift in Sources */,
5EF581542041387C00AC14FE /* CustomPlaygroundDisplayConvertibleTests.swift in Sources */,
5E1069A423CFD68100C79FC2 /* PGLNilObject.m in Sources */,
5EFE9193203F6CF900E21BAA /* LegacyPlaygroundLoggerTests.swift in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand Down Expand Up @@ -1044,6 +1055,7 @@
5E5D77822040BD7D00EBC3A9 /* LogPolicyTests.swift in Sources */,
5E48E0AC2042925B00C7712D /* LogEntryTests.swift in Sources */,
5EF581552041387C00AC14FE /* CustomPlaygroundDisplayConvertibleTests.swift in Sources */,
5E1069A523CFD68100C79FC2 /* PGLNilObject.m in Sources */,
5EFE91D7203F6F8100E21BAA /* LegacyPlaygroundLoggerTests.swift in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand Down Expand Up @@ -1263,33 +1275,38 @@
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = YES;
CLANG_ENABLE_MODULES = YES;
CODE_SIGN_STYLE = Automatic;
COMBINE_HIDPI_IMAGES = YES;
INFOPLIST_FILE = PlaygroundLoggerTests_macOS/Info.plist;
LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/../Frameworks @loader_path/../Frameworks";
PRODUCT_BUNDLE_IDENTIFIER = "org.swift.PlaygroundLoggerTests-macOS";
PRODUCT_MODULE_NAME = PlaygroundLoggerTests;
PRODUCT_NAME = "$(TARGET_NAME)";
SWIFT_OBJC_BRIDGING_HEADER = "PlaygroundLoggerTests/PlaygroundLoggerTests-Bridging-Header.h";
};
name = Debug;
};
5E2646401FB64876002DC6B6 /* Release */ = {
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = YES;
CLANG_ENABLE_MODULES = YES;
CODE_SIGN_STYLE = Automatic;
COMBINE_HIDPI_IMAGES = YES;
INFOPLIST_FILE = PlaygroundLoggerTests_macOS/Info.plist;
LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/../Frameworks @loader_path/../Frameworks";
PRODUCT_BUNDLE_IDENTIFIER = "org.swift.PlaygroundLoggerTests-macOS";
PRODUCT_MODULE_NAME = PlaygroundLoggerTests;
PRODUCT_NAME = "$(TARGET_NAME)";
SWIFT_OBJC_BRIDGING_HEADER = "PlaygroundLoggerTests/PlaygroundLoggerTests-Bridging-Header.h";
};
name = Release;
};
5EFE918E203F6CC400E21BAA /* Debug */ = {
isa = XCBuildConfiguration;
buildSettings = {
CLANG_ENABLE_MODULES = YES;
CLANG_ENABLE_OBJC_WEAK = YES;
CLANG_WARN_DEPRECATED_OBJC_IMPLEMENTATIONS = YES;
CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF = YES;
Expand All @@ -1301,6 +1318,7 @@
PRODUCT_MODULE_NAME = PlaygroundLoggerTests;
PRODUCT_NAME = "$(TARGET_NAME)";
SDKROOT = iphoneos;
SWIFT_OBJC_BRIDGING_HEADER = "PlaygroundLoggerTests/PlaygroundLoggerTests-Bridging-Header.h";
TARGETED_DEVICE_FAMILY = "1,2";
TEST_HOST = "$(BUILT_PRODUCTS_DIR)/PlaygroundLoggerTestHost_iOS.app/PlaygroundLoggerTestHost_iOS";
};
Expand All @@ -1309,6 +1327,7 @@
5EFE918F203F6CC400E21BAA /* Release */ = {
isa = XCBuildConfiguration;
buildSettings = {
CLANG_ENABLE_MODULES = YES;
CLANG_ENABLE_OBJC_WEAK = YES;
CLANG_WARN_DEPRECATED_OBJC_IMPLEMENTATIONS = YES;
CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF = YES;
Expand All @@ -1320,6 +1339,7 @@
PRODUCT_MODULE_NAME = PlaygroundLoggerTests;
PRODUCT_NAME = "$(TARGET_NAME)";
SDKROOT = iphoneos;
SWIFT_OBJC_BRIDGING_HEADER = "PlaygroundLoggerTests/PlaygroundLoggerTests-Bridging-Header.h";
TARGETED_DEVICE_FAMILY = "1,2";
TEST_HOST = "$(BUILT_PRODUCTS_DIR)/PlaygroundLoggerTestHost_iOS.app/PlaygroundLoggerTestHost_iOS";
VALIDATE_PRODUCT = YES;
Expand Down Expand Up @@ -1403,6 +1423,7 @@
5EFE91D5203F6F6A00E21BAA /* Debug */ = {
isa = XCBuildConfiguration;
buildSettings = {
CLANG_ENABLE_MODULES = YES;
CLANG_ENABLE_OBJC_WEAK = YES;
CLANG_WARN_DEPRECATED_OBJC_IMPLEMENTATIONS = YES;
CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF = YES;
Expand All @@ -1414,6 +1435,7 @@
PRODUCT_MODULE_NAME = PlaygroundLoggerTests;
PRODUCT_NAME = "$(TARGET_NAME)";
SDKROOT = appletvos;
SWIFT_OBJC_BRIDGING_HEADER = "PlaygroundLoggerTests/PlaygroundLoggerTests-Bridging-Header.h";
TARGETED_DEVICE_FAMILY = 3;
TEST_HOST = "$(BUILT_PRODUCTS_DIR)/PlaygroundLoggerTestHost_tvOS.app/PlaygroundLoggerTestHost_tvOS";
};
Expand All @@ -1422,6 +1444,7 @@
5EFE91D6203F6F6A00E21BAA /* Release */ = {
isa = XCBuildConfiguration;
buildSettings = {
CLANG_ENABLE_MODULES = YES;
CLANG_ENABLE_OBJC_WEAK = YES;
CLANG_WARN_DEPRECATED_OBJC_IMPLEMENTATIONS = YES;
CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF = YES;
Expand All @@ -1433,6 +1456,7 @@
PRODUCT_MODULE_NAME = PlaygroundLoggerTests;
PRODUCT_NAME = "$(TARGET_NAME)";
SDKROOT = appletvos;
SWIFT_OBJC_BRIDGING_HEADER = "PlaygroundLoggerTests/PlaygroundLoggerTests-Bridging-Header.h";
TARGETED_DEVICE_FAMILY = 3;
TEST_HOST = "$(BUILT_PRODUCTS_DIR)/PlaygroundLoggerTestHost_tvOS.app/PlaygroundLoggerTestHost_tvOS";
VALIDATE_PRODUCT = YES;
Expand Down
88 changes: 87 additions & 1 deletion PlaygroundLogger/PlaygroundLogger/LogEntry+Reflection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2017-2018 Apple Inc. and the Swift project authors
// Copyright (c) 2017-2020 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -31,6 +31,32 @@ extension LogEntry {
return
}

// In Swift, it is undefined behavior to have a value of non-Optional class type which is set to nil.
// Unfortunately, these can be encountered in the wild; the most common example is an incorrect Objective-C nullability annotation.
// To prevent PlaygroundLogger from triggering undefined behavior (which, as of this writing, causes a crash in the Swift runtime), perform a safety check before proceeding with logging.
do {
// First, parse the existential container for `instance` so we can examine its contents directly.
let existentialContainer = AnyExistentialContainer(instance)

// Next, get the type of `instance` which is stored in the existential container.
guard let instanceType = existentialContainer.type else {
// The existential container does not contain a type, which is invalid.
// Rather than crash by trying to proceed along from here, return an error LogEntry.
self = .error(reason: "Value does not contain a type")
return
}

// If `instance` contains a non-Optional class type (e.g. AnyObject), then `instanceType is AnyClass` will be true.
// If `instance` contains an Optional class type (e.g. AnyObject? aka Optional<AnyObject>), then `instanceType is AnyClass` will be false, as Optional is *not* a class.
//
// If `instance` contains a non-Optional class type, then the first pointer in the existential container's buffer will be the object pointer. We can check to see if that pointer is nil to see if the object is nil.
if instanceType is AnyClass && existentialContainer.fixedSizeBuffer.0 == nil {
// `instance` is broken. As a result, return a log entry which indicates that the value is nil.
self = .structured(name: name, typeName: passedInTypeName ?? normalizedName(of: instanceType), summary: "nil", totalChildrenCount: 0, children: [], disposition: .aggregate)
return
}
}

// Lazily-load the Mirror for this instance. (This is factored out this way as the Mirror is needed in a few different code paths.)
var _mirrorStorage: Mirror? = nil
var mirror: Mirror {
Expand Down Expand Up @@ -261,6 +287,66 @@ extension Mirror {
}
}

/// A struct representing the memory contents of an `Any` -- the existential container for the `Any` rather than the value itself contained in the `Any`.
///
/// The memory layout of an existential container is part of Swift's ABI. It is defined as:
///
/// - Three words of fixed storage for an inline buffer containing either the value or a pointer to a box containing the value
/// - One word containing a pointer to the type metadata for the value stored in the existential
/// - Zero or more words containing pointers to the protocol witness tables for the protocols to which the existential is constrained
///
/// Since `Any` is an existential with no protocol constraints, it ultimately is a four-word value where the first three words are a fixed-size buffer
/// and the last word is a pointer to the type metadata.
///
/// This struct stores the fixed-size buffer as three `UnsafeRawPointer?` values.
/// An `Any.Type?` (aka `Optional<Any.Type>`, not `Optional<Any>.Type`) in Swift is itself a pointer to the type metadata, so this struct loads that pointer as an `Any.Type?` for examination.
fileprivate struct AnyExistentialContainer {
/// A fixed-size, three-word buffer containing the value or a pointer to a box containing the value stored in this existential container.
///
/// The contents of this buffer must only be interpreted in the context of a particular type.
/// For example, if this existential container contains an object, the first word will contain the pointer to that object while the contents of the other two words are undefined.
var fixedSizeBuffer: (UnsafeRawPointer?, UnsafeRawPointer?, UnsafeRawPointer?)

/// The type of the value stored in this existential container.
///
/// This can be used to interpret the value stored in `fixedSizeBuffer`.
/// For example, if this existential container contains an object, `self.type is AnyClass` will be true.
var type: Any.Type?

/// Initializes a new `AnyExistentialContainer` with the contents of `instance`.
///
/// This initializer does this with direct memory access to load the `Any` as its container rather than as the value contained in the `Any`.
///
/// - important: This initializer does not use the Swift runtime to interact with the contents of `instance`.
/// It is **critical** that this remain true so that an untrusted `Any` can be examined by PlaygroundLogger before proceeding with logging.
///
/// - parameter instance: The `Any` whose existential container should be made available in a new instance of `AnyExistentialContainer`
init(_ instance: Any) {
// TODO: If Swift implements support for static/compile-time assertions, we should adopt those here.

// We require that `AnyExistentialContainer` and `Any` have the same memory layouts.
assert(MemoryLayout<AnyExistentialContainer>.size == MemoryLayout<Any>.size)
assert(MemoryLayout<AnyExistentialContainer>.alignment == MemoryLayout<Any>.alignment)
assert(MemoryLayout<AnyExistentialContainer>.stride == MemoryLayout<Any>.stride)

// Ensure that we catch any changes to the struct layout which might move it out of alignment with `Any`.
// Importantly, `fixedSizeBuffer` should be at offset 0, `fixedSizeBuffer` should be exactly three words wide, and `type` should be at an offset immediately after `fixedSizeBuffer`.
assert(MemoryLayout<AnyExistentialContainer>.offset(of: \AnyExistentialContainer.fixedSizeBuffer) == 0)
assert(MemoryLayout<(UnsafeRawPointer?, UnsafeRawPointer?, UnsafeRawPointer?)>.size == (3 * MemoryLayout<UnsafeRawPointer?>.size))
assert(MemoryLayout<AnyExistentialContainer>.offset(of: \AnyExistentialContainer.type) == MemoryLayout<(UnsafeRawPointer?, UnsafeRawPointer?, UnsafeRawPointer?)>.size)

// We also require that `Any.Type?` be a trivial type so that it's safe to load this way.
assert(_isPOD(Any.Type?.self))

// Finally, we require that we are a trivial type as well.
assert(_isPOD(AnyExistentialContainer.self))

self = withUnsafeBytes(of: instance) { bytes in
return bytes.load(as: AnyExistentialContainer.self)
}
}
}

/// Construct the summary for `instance`.
///
/// In precedence order, the rules are:
Expand Down
51 changes: 50 additions & 1 deletion PlaygroundLogger/PlaygroundLoggerTests/LogEntryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2018-2019 Apple Inc. and the Swift project authors
// Copyright (c) 2018-2020 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -167,4 +167,53 @@ class LogEntryTests: XCTestCase {

XCTAssertEqual(disposition, .membershipContainer)
}

func testOptionalNilObject() throws {
let object: NSObject? = nil

let logEntry = try LogEntry(describing: object as Any, name: "object", policy: .default)

guard case let .structured(name, typeName, summary, totalChildrenCount, children, disposition) = logEntry else {
XCTFail("Expected to receive a structured log entry, but didn't")
return
}

XCTAssertEqual(name, "object")
XCTAssert(typeName.hasPrefix("Optional<"))
XCTAssertEqual(summary, "nil")
XCTAssertEqual(totalChildrenCount, 0)
XCTAssert(children.isEmpty)

guard case .aggregate = disposition else {
XCTFail("Expected to receive an aggregate, but didn't")
return
}
}

func testNonOptionalNilObject() throws {
// PGLNilObject is a type which returns nil from -init.
// This occurs despite the fact that -init is marked as return a nonnull value.
// This allows us to test PlaygroundLogger against Objective-C types which have bad nullability annotations.
let object: PGLNilObject = PGLNilObject()

XCTAssertEqual(Int(bitPattern: ObjectIdentifier(object)), 0, "We expect PGLNilObject to produce a nil object")

let logEntry = try LogEntry(describing: object, name: "object", policy: .default)

guard case let .structured(name, typeName, summary, totalChildrenCount, children, disposition) = logEntry else {
XCTFail("Expected to receive an error log entry, but didn't")
return
}

XCTAssertEqual(name, "object")
XCTAssertEqual(typeName, "PGLNilObject")
XCTAssertEqual(summary, "nil")
XCTAssertEqual(totalChildrenCount, 0)
XCTAssert(children.isEmpty)

guard case .aggregate = disposition else {
XCTFail("Expected to receive an aggregate, but didn't")
return
}
}
}
27 changes: 27 additions & 0 deletions PlaygroundLogger/PlaygroundLoggerTests/PGLNilObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//===--- PGLNilObject.h ---------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2020 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

#import <Foundation/Foundation.h>

NS_ASSUME_NONNULL_BEGIN

/// \c PGLNilObject is a test class which includes a \c -init method which is intentionally mis-annotated.
/// It returns \c nil instead of an instance of \c PGLNilObject to allow PlaygroundLogger's tests to exercise logging this sort of broken value.
///
/// See also: \c LogEntryTests.testNonOptionalNilObject()
@interface PGLNilObject : NSObject

- (instancetype)init;

@end

NS_ASSUME_NONNULL_END
21 changes: 21 additions & 0 deletions PlaygroundLogger/PlaygroundLoggerTests/PGLNilObject.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//===--- PGLNilObject.m ---------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2020 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See http://swift.org/LICENSE.txt for license information
// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//

#import "PGLNilObject.h"

@implementation PGLNilObject

- (instancetype)init {
return nil;
}

@end
Loading