Skip to content

Commit

Permalink
fix: Strip top bits from pointers before reading memory contents
Browse files Browse the repository at this point in the history
In the new Xcode 10 build system, Swift object register values have the
top bit used as a flag. This change strips the flag while not losing
anything relevant to us in our quest to see error messages for assertion
failures.

This technique does not capture messages which are less than 16
characters, as short strings are stored as raw char arrays on the stack
rather than being allocated. (See WWDC 2018 #401 for more info on new
string optimizations)

While it is possible to check for char arrays as well as pointers when
searching for notable address values, sweeping up local variables has a
likely chance of capturing unintended data as well from the surrounding
code, some of which may be sensitive. It is also not guaranteed that the
value would still be on the stack after the message is logged, so it is
possible to get only unrelated string values as the message.

In the current Swift stdlib, the following messages passed to
fatalError, preconditionFailure, and precondition (and their internal
func counterparts) are less than 16 characters:

* empty string
* `unavailable`
* `not implemented`
* `abstract method`
* `unknown value`
* `invalid count` (where a dictionary contains < 0 items(?))
* `invalid index` (where a dictionary ceases to be a dictionary)
* `don't touch me` (from SpriteKit)
* `close() failed` (from the private Subprocess implementation)

The vast majority have more meaningful messages.

Reference:
* https://asciiwwdc.com/2018/sessions/401

Fixes #318
  • Loading branch information
kattrali committed Nov 29, 2018
1 parent f0f22a4 commit 7094543
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 25 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ Changelog
addresses
[#319](https://github.com/bugsnag/bugsnag-cocoa/pull/319)

* Add `fatalError` and other assertion failure messages in reports for
Swift 4.2 apps. Note that this only includes messages which are 16
characters or longer. See the linked pull request for more information.
[#320](https://github.com/bugsnag/bugsnag-cocoa/pull/320)

## 5.17.0 (2018-09-25)

### Enhancements
Expand Down
16 changes: 13 additions & 3 deletions Source/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c
Original file line number Diff line number Diff line change
Expand Up @@ -983,20 +983,30 @@ bool bsg_kscrw_i_isValidPointer(const uintptr_t address) {
return true;
}

/**
* Strip higher order bits from addresses which aren't related to the actual
* location.
*/
#define BSG_ValidPointerMask 0x0000000fffffffff

/** Write the contents of a memory location only if it contains notable data.
* Also writes meta information about the data.
*
* @param writer The writer.
*
* @param key The object key, if needed.
*
* @param address The memory address.
* @param rawAddress The memory address.
*/
void bsg_kscrw_i_writeMemoryContentsIfNotable(
const BSG_KSCrashReportWriter *const writer, const char *const key,
const uintptr_t address) {
const uintptr_t rawAddress) {
uintptr_t address = rawAddress;
if (!bsg_kscrw_i_isValidPointer(address)) {
return;
address &= BSG_ValidPointerMask;
if (!bsg_kscrw_i_isValidPointer(address)) {
return;
}
}

const void *object = (const void *)address;
Expand Down
11 changes: 9 additions & 2 deletions features/crashprobe.feature
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,15 @@ Scenario: Crash within Swift code
And the exception "message" equals "Unexpectedly found nil while unwrapping an Optional value"
And the exception "errorClass" equals "Fatal error"

And the "method" of stack frame 0 starts with "_T0s18_fatalErrorMessages5NeverOs12StaticStringV_A2E4fileSu4lines6UInt32V5"
And the "method" of stack frame 1 starts with "_T010iOSTestApp10SwiftCrashC3run"
Scenario: Assertion failure in Swift code
When I set environment variable "BUGSNAG_API_KEY" to "a35a2a72bd230ac0aa0f52715bbdc6aa"
And I configure the app to run on "iPhone8-11.2"
And I crash the app using "SwiftAssertion"
And I relaunch the app
Then I should receive a request
And the request is a valid for the error reporting API
And the exception "errorClass" equals "Fatal error"
And the exception "message" equals "several unfortunate things just happened"

Scenario: Dereference a null pointer
When I set environment variable "BUGSNAG_API_KEY" to "a35a2a72bd230ac0aa0f52715bbdc6aa"
Expand Down
8 changes: 4 additions & 4 deletions features/fixtures/ios-swift-cocoapods/Podfile.lock
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
PODS:
- Bugsnag (5.15.6)
- Bugsnag (5.17.0)

DEPENDENCIES:
- Bugsnag (from `../../..`)

EXTERNAL SOURCES:
Bugsnag:
:path: ../../..
:path: "../../.."

SPEC CHECKSUMS:
Bugsnag: ff5f5e3059e6a9c9d27a899f3bf3774067553483
Bugsnag: 2d163d2f4c7acdb9bbcfc7a938c646f3aa39f566

PODFILE CHECKSUM: 4d026fb83571f098c9fb4fa71c1564c72c55ab1a

COCOAPODS: 1.4.0
COCOAPODS: 1.5.3
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
objects = {

/* Begin PBXBuildFile section */
8A840FBA21AF5C450041DBFA /* SwiftAssertion.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */; };
8A98400320FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m in Sources */ = {isa = PBXBuildFile; fileRef = 8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */; };
8AB8866420404DD30003E444 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8AB8866320404DD30003E444 /* AppDelegate.swift */; };
8AB8866620404DD30003E444 /* ViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8AB8866520404DD30003E444 /* ViewController.swift */; };
Expand Down Expand Up @@ -54,6 +55,7 @@

/* Begin PBXFileReference section */
4994F05E0421A0B037DD2CC5 /* Pods_iOSTestApp.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_iOSTestApp.framework; sourceTree = BUILT_PRODUCTS_DIR; };
8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SwiftAssertion.swift; sourceTree = "<group>"; };
8A98400120FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = AutoSessionCustomVersionScenario.h; sourceTree = "<group>"; };
8A98400220FD11BF0023ECD1 /* AutoSessionCustomVersionScenario.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = AutoSessionCustomVersionScenario.m; sourceTree = "<group>"; };
8AB8866020404DD30003E444 /* iOSTestApp.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = iOSTestApp.app; sourceTree = BUILT_PRODUCTS_DIR; };
Expand Down Expand Up @@ -196,6 +198,7 @@
F42953DE2BB41023C0B07F41 /* scenarios */ = {
isa = PBXGroup;
children = (
8A840FB921AF5C450041DBFA /* SwiftAssertion.swift */,
F42957FA1A3724BFBDC22E14 /* NSExceptionScenario.swift */,
F4295F595986A279FA3BDEA7 /* UserEmailScenario.swift */,
F42954E2B3FF0C5C0474DA74 /* UserEnabledScenario.swift */,
Expand Down Expand Up @@ -277,7 +280,6 @@
8AB8865D20404DD30003E444 /* Frameworks */,
8AB8865E20404DD30003E444 /* Resources */,
4D2139E03F8B071F7DB7C7A6 /* [CP] Embed Pods Frameworks */,
F4E835AC090FCF28B6B12EC4 /* [CP] Copy Pods Resources */,
);
buildRules = (
);
Expand Down Expand Up @@ -373,21 +375,6 @@
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-iOSTestApp/Pods-iOSTestApp-frameworks.sh\"\n";
showEnvVarsInLog = 0;
};
F4E835AC090FCF28B6B12EC4 /* [CP] Copy Pods Resources */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
name = "[CP] Copy Pods Resources";
outputPaths = (
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-iOSTestApp/Pods-iOSTestApp-resources.sh\"\n";
showEnvVarsInLog = 0;
};
/* End PBXShellScriptBuildPhase section */

/* Begin PBXSourcesBuildPhase section */
Expand Down Expand Up @@ -415,6 +402,7 @@
F42955DB6D08642528917FAB /* CxxExceptionScenario.mm in Sources */,
F42954B7318A02824C65C514 /* ObjCMsgSendScenario.m in Sources */,
F42953498545B853CC0B635E /* NullPointerScenario.m in Sources */,
8A840FBA21AF5C450041DBFA /* SwiftAssertion.swift in Sources */,
F429538D8941382EC2C857CE /* AsyncSafeThreadScenario.m in Sources */,
F42955869D33EE0E510B9651 /* ReadGarbagePointerScenario.m in Sources */,
8AEFC73420F8D1BB00A78779 /* ManualSessionWithUserScenario.m in Sources */,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Foundation

class SwiftAssertion: Scenario {
override func startBugsnag() {
self.config.shouldAutoCaptureSessions = false;
super.startBugsnag()
}

override func run() {
fatalError("several unfortunate things just happened")
}
}

0 comments on commit 7094543

Please sign in to comment.