Skip to content
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.

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

Fix possible crash or deadlock arising from calling notify() from multiple queues concurrently #401

Merged
merged 9 commits into from
Aug 14, 2019

Conversation

kattrali
Copy link
Contributor

@kattrali kattrali commented Aug 9, 2019

Goal

Its possible that when notify() is called concurrently from multiple queues, that a crash occurs when caching/resetting crash state. This changeset solves the underlying issues by not sharing state between calls to notify() and isolating calls to suspend threads (for backtrace collection).

Reproduction case

dispatch_queue_t queue1 = dispatch_queue_create("Queue 1", DISPATCH_QUEUE_CONCURRENT);
dispatch_queue_t queue2 = dispatch_queue_create("Queue 2", DISPATCH_QUEUE_CONCURRENT);

for (int i = 0; i < 4; i++) {
    NSString *message = [NSString stringWithFormat:@"Err %ld", (long)i];
    NSError *error = [FooError errorWithDomain:@"com.example"
                                          code:340
                                      userInfo:nil];
    dispatch_async(queue1, ^{
        [Bugsnag notifyError:error];
    });
    dispatch_async(queue2, ^{
        [Bugsnag notifyError:error];
    });
}

Fixes #399

Changeset

The changes can be divided into four parts:

  1. Adding a (failing on master) test which calls notify() several times from different concurrent queues
  2. Refactoring crash reporting internals to allow providing separate crash contexts for each invocation of notify()
  3. Implementing separate crash contexts for notify() calls
  4. Adding locking around suspending threads and writing crash state.

The easiest way to view the changeset is one commit at a time, since each chunk can be evaluated independently.

Tests

  • Tested manually on a couple different iOS/tvOS devices running iOS 11 & 12
  • Added automated tests replicating the failing scenario

Removed inlining/static to be compatible with C++ handler
Allows the system to have multiple crash report paths for different
contexts
Allows each report generator to use a different context if needed
While not async signal safe, this interface is suitable for generating
IDs on the fly when calling reportUserException().
This change allows each notify request to specify its own crash ID and
file path, avoiding cases where two notify calls either reuse the same
file path or stomp on each other when trying to set the "next" ID/path.
This avoids multiple threads from notify() stomping on this file
@kattrali kattrali mentioned this pull request Aug 9, 2019
Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

One suggestion for a unit test to cover new function (if feasible).

@interface KSCrashIdentifierTests : XCTestCase
@end

@implementation KSCrashIdentifierTests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include a test for bsg_kscrash_generate_report_path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I should - it turned out to be a bit tricky to stub the file store but I've added a task to improve coverage here.

@nkavian
Copy link

nkavian commented Aug 14, 2019

When will a new release be ready? Thanks.

@kattrali kattrali merged commit c3d5975 into master Aug 14, 2019
@kattrali kattrali deleted the kattrali/use-separate-reporting-context-in-notify branch August 14, 2019 16:07
@kattrali
Copy link
Contributor Author

Hi @nkavian! Doing a release now, should be available soon.

@nkavian
Copy link

nkavian commented Aug 15, 2019

Thanks, confirming the latest release fixed my issue!

ghazel pushed a commit to clostra/bugsnag-cocoa that referenced this pull request Aug 12, 2020
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 bugsnag#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 bugsnag#318
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIGABRT in notifyError
3 participants