Fix potential deadlock when generating a crash report #577
Conversation
robinmacharg
changed the title
There was a problem hiding this comment.
This approach seems reasonable from an initial review - I've left a few inline comments going through it, and I haven't attempted to run the code yet. One thing to consider would be running Instruments to give a very basic benchmark on initialization performance, so we can quantify how many milliseconds this approach is adding.
| - (void)listenForLoadedBinaries { | ||
| bsg_initialise_mach_binary_headers(); | ||
| _dyld_register_func_for_add_image(&bsg_mach_binary_image_added); | ||
| _dyld_register_func_for_remove_image(&bsg_mach_binary_image_removed); |
There was a problem hiding this comment.
Potentially worth registering bsg_mach_binary_image_removed first. This avoids the case where some binary image is removed on another thread while the bsg_mach_binary_image_added callback is executed for the first time.
| @@ -226,6 +228,9 @@ - (BOOL)install { | |||
| return false; | |||
| } | |||
|
|
|||
| // Maintain a cache of info about dynamically loaded binary images | |||
| [self listenForLoadedBinaries]; | |||
There was a problem hiding this comment.
Potentially worth putting this before bsg_kscrash_install, or at least initialising an empty array of binary images to prevent against the case where a crash occurs during/at the same time as initialisation.
| } | ||
|
|
||
| /** | ||
| * Compare two mach biunary image structures |
There was a problem hiding this comment.
spelling nit: biunary ->binary
| const char* name; | ||
| cpu_type_t cputype; /* cpu specifier */ | ||
| cpu_subtype_t cpusubtype; /* machine specifier */ | ||
| } BSG_Mach_Binary_Image_Info; |
There was a problem hiding this comment.
Encapsulating this info in a struct works well 👍
| { | ||
| BSG_Mach_Binary_Image_Info info = { 0 }; | ||
| if (populate_info(header, &info)) { | ||
| [bsg_mach_binary_images_info addObject:[NSValue valueWithBytes:&info |
There was a problem hiding this comment.
Using an Objective-C object makes me a bit nervous - as if dyld continues to execute this callback during a signal crash, then it could hang the process.
Would it be possible to initialise a C array to start with, then allocate more memory as required?
| /** | ||
| * Called when a binary image is unloaded. | ||
| */ | ||
| void bsg_mach_binary_image_removed(const struct mach_header *header, intptr_t slide) |
There was a problem hiding this comment.
Will this method need any form of synchronization if images are loaded/unloaded on different threads? Or is that a non-issue?
| /** | ||
| * Returns a C array of structs describing the loaded Mach binaries | ||
| */ | ||
| BSG_Mach_Binary_Image_Info* bsg_mach_header_array(size_t *count) { |
There was a problem hiding this comment.
My gut feel is that this function could be done away with if the structs are stored as a C array
|
This PR is superseded by #580. All noted issues have been addressed there. |
Goal
An issue has been identified that can lead to an app deadlocking when reporting a crash.
The sequence of events is:
dyldand takes a lock out.dyldlock.This PR fixes this issue.
Design
There are two main mechanisms by which a program may find out about
dyldbehaviour: by direct querying (e.g._dyld_get_image_header()etc., used by the KSCrash component of Bugsnag prior to this issue) and by callback (_dyld_register_func_for_add_image()etc.). This change modifies the KSCrash component to use callbacks and caches information about loaded binary images for when a crash does occur. Thus, when threads are suspended and a crash report is generated no additional calls todyldneed to be made and deadlock is avoided.Changeset
dyldbinary image capture was moved fromBSG_KSCrashReport.ctoBSG_KSCrash.m. A static cache is used. An additional.mfile contains functions to ease bridging between Objective C and C.Tests
TBD
Review
Outstanding Questions
masterfor fixes,nextforfeatures)