fix: Check for null image name before comparing strings #321
Conversation
If the number of loaded images changes between when _dyld_image_count() is computed and when _dyld_get_image_name() is called, there can be less images than the count suggests. In this case, _dyld_get_image_name() returns NULL, and the lack of a check would crash the app (again?) because both arguments to strcmp are required to be non-null. The resulting exception backtrace looks somewhat like this: ``` _platform_strcmp bsg_ksdlimageNamed (BSG_KSDynamicLinker.c:41:21) bsg_ksdlimageUUID (BSG_KSDynamicLinker.c:56:31) +[BSG_KSSystemInfo appUUID] (BSG_KSSystemInfo.m:158:13) +[BSG_KSSystemInfo systemInfo] (BSG_KSSystemInfo.m:432:5) -[BugsnagSessionTrackingPayload toJson] (BugsnagSessionTrackingPayload.m:48:5) ```
kattrali
force-pushed
the
kattrali/fix-crash-in-async-unload
branch
from
6fa2ee6 to
73e024b
Compare
There was a problem hiding this comment.
I believe this should work for the specific case we've encountered. Do we have a reproduction case for the behaviour that would allow us to test out the scenario and confirm this?
Searching the entire codebase for usages of dyld functions reveals a few more sites which don't have null checks e.g. BSG_KSSystemInfo.m 364 (and various others). My understanding is that some of these are ok, because KSCrash will suspend all threads when reporting an error, but some either require a NULL check or additional synchronization.
This one in particular is "safe", as its loading the first image from the array, and there's going to be at least one image at all times. I went through a few other code paths in the dynamic linker though and while I can see how they eventually nope out if the image header or name is null, I'll add a few more explicit checks. |
If the number of loaded images changes between when _dyld_image_count()
is computed and when _dyld_get_image_name() is called, there can be less
images than the count suggests. In this case, _dyld_get_image_name()
returns NULL, and the lack of a check would crash the app (again?)
because both arguments to strcmp are required to be non-null. The
resulting exception backtrace looks somewhat like this:
Tests
This one is difficult to verify manually or to write a test for, (short of swapping out the implementation of
_dyld_image_count()to immediately unload an image), but the intent should be easy to follow from the source exception.Review
Outstanding Questions