From 03d0e8e5cf2a31d214b29884deeca507f6bf56ef Mon Sep 17 00:00:00 2001 From: Tom Longridge Date: Sun, 28 Jun 2020 11:02:05 +0100 Subject: [PATCH 1/5] refactor(mach headers): improve encapsulation The layout of the MachHeaders module is not optimal as it exposes unnecessary implementation detail. This means the index of the image in the list is passed around and increases the chances of this changing if used in multi-threaded context. --- .../Source/KSCrash/Recording/BSG_KSCrash.m | 1 - .../KSCrash/Recording/BSG_KSSystemInfo.m | 3 +- .../Recording/Tools/BSG_KSDynamicLinker.c | 201 ++---------------- .../Recording/Tools/BSG_KSDynamicLinker.h | 53 +---- .../Recording/Tools/BSG_KSMachHeaders.h | 97 ++++----- .../Recording/Tools/BSG_KSMachHeaders.m | 156 +++++++++++--- Tests/KSCrash/KSDynamicLinker_Tests.m | 6 - Tests/KSCrash/KSMachHeader_Tests.m | 68 +++--- 8 files changed, 206 insertions(+), 379 deletions(-) diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m index 7396238aa..2a30f7ee2 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m @@ -278,7 +278,6 @@ - (BOOL)install { * behaviour. */ - (void)listenForLoadedBinaries { - bsg_check_unfair_lock_support(); bsg_initialise_mach_binary_headers(BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE); // Note: Access to DYLD's binary image store is guarded by locks. diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m index 07a82910d..8da7f995b 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m @@ -29,6 +29,7 @@ #import "BSG_KSSystemInfo.h" #import "BSG_KSSystemInfoC.h" #import "BSG_KSDynamicLinker.h" +#import "BSG_KSMachHeaders.h" #import "BSG_KSJSONCodecObjC.h" #import "BSG_KSMach.h" #import "BSG_KSSysCtl.h" @@ -259,7 +260,7 @@ + (NSString *)currentCPUArch { * @return YES if the device is jailbroken. */ + (BOOL)isJailbroken { - return bsg_ksdlimageNamed("MobileSubstrate", false) != UINT32_MAX; + return bsg_mach_image_named("MobileSubstrate", false) != NULL; } /** Check if the current build is a debug build. diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c index 148647937..9905fcae6 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c @@ -32,37 +32,13 @@ #include #include -uint32_t bsg_ksdlimageNamed(const char *const imageName, bool exactMatch) { - if (imageName != NULL) { - const uint32_t imageCount = bsg_dyld_image_count(); - - for (uint32_t iImg = 0; iImg < imageCount; iImg++) { - const char *name = bsg_dyld_get_image_name(iImg); - if (name == NULL) { - continue; // name is null if the index is out of range per dyld(3) - } else if (exactMatch) { - if (strcmp(name, imageName) == 0) { - return iImg; - } - } else { - if (strstr(name, imageName) != NULL) { - return iImg; - } - } - } - } - return UINT32_MAX; -} - const uint8_t *bsg_ksdlimageUUID(const char *const imageName, bool exactMatch) { if (imageName != NULL) { - const uint32_t iImg = bsg_ksdlimageNamed(imageName, exactMatch); - if (iImg != UINT32_MAX) { - const struct mach_header *header = bsg_dyld_get_image_header(iImg); - if (header != NULL) { - uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header); + BSG_Mach_Binary_Image_Info *img = bsg_mach_image_named(imageName, exactMatch); + if (img != NULL) { + uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(img->header); if (cmdPtr != 0) { - for (uint32_t iCmd = 0; iCmd < header->ncmds; iCmd++) { + for (uint32_t iCmd = 0; iCmd < img->header->ncmds; iCmd++) { const struct load_command *loadCmd = (struct load_command *)cmdPtr; if (loadCmd->cmd == LC_UUID) { @@ -75,97 +51,10 @@ const uint8_t *bsg_ksdlimageUUID(const char *const imageName, bool exactMatch) { } } } - } return NULL; } -uintptr_t bsg_ksdlfirstCmdAfterHeader(const struct mach_header *const header) { - if (header == NULL) { - return 0; - } - switch (header->magic) { - case MH_MAGIC: - case MH_CIGAM: - return (uintptr_t)(header + 1); - case MH_MAGIC_64: - case MH_CIGAM_64: - return (uintptr_t)(((struct mach_header_64 *)header) + 1); - default: - // Header is corrupt - return 0; - } -} - -uint32_t bsg_ksdlimageIndexContainingAddress(const uintptr_t address) { - const uint32_t imageCount = bsg_dyld_image_count(); - const struct mach_header *header = 0; - - for (uint32_t iImg = 0; iImg < imageCount; iImg++) { - header = bsg_dyld_get_image_header(iImg); - if (header != NULL) { - // Look for a segment command with this address within its range. - uintptr_t addressWSlide = - address - bsg_dyld_get_image_vmaddr_slide(iImg); - uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header); - if (cmdPtr == 0) { - continue; - } - for (uint32_t iCmd = 0; iCmd < header->ncmds; iCmd++) { - const struct load_command *loadCmd = - (struct load_command *)cmdPtr; - if (loadCmd->cmd == LC_SEGMENT) { - const struct segment_command *segCmd = - (struct segment_command *)cmdPtr; - if (addressWSlide >= segCmd->vmaddr && - addressWSlide < segCmd->vmaddr + segCmd->vmsize) { - return iImg; - } - } else if (loadCmd->cmd == LC_SEGMENT_64) { - const struct segment_command_64 *segCmd = - (struct segment_command_64 *)cmdPtr; - if (addressWSlide >= segCmd->vmaddr && - addressWSlide < segCmd->vmaddr + segCmd->vmsize) { - return iImg; - } - } - cmdPtr += loadCmd->cmdsize; - } - } - } - return UINT_MAX; -} - -uintptr_t bsg_ksdlsegmentBaseOfImageIndex(const uint32_t idx) { - const struct mach_header *header = bsg_dyld_get_image_header(idx); - if (header == NULL) { - return 0; - } - // Look for a segment command and return the file image address. - uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header); - if (cmdPtr == 0) { - return 0; - } - for (uint32_t i = 0; i < header->ncmds; i++) { - const struct load_command *loadCmd = (struct load_command *)cmdPtr; - if (loadCmd->cmd == LC_SEGMENT) { - const struct segment_command *segmentCmd = - (struct segment_command *)cmdPtr; - if (strcmp(segmentCmd->segname, SEG_LINKEDIT) == 0) { - return segmentCmd->vmaddr - segmentCmd->fileoff; - } - } else if (loadCmd->cmd == LC_SEGMENT_64) { - const struct segment_command_64 *segmentCmd = - (struct segment_command_64 *)cmdPtr; - if (strcmp(segmentCmd->segname, SEG_LINKEDIT) == 0) { - return (uintptr_t)(segmentCmd->vmaddr - segmentCmd->fileoff); - } - } - cmdPtr += loadCmd->cmdsize; - } - - return 0; -} bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) { info->dli_fname = NULL; @@ -173,34 +62,28 @@ bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) { info->dli_sname = NULL; info->dli_saddr = NULL; - const uint32_t idx = bsg_ksdlimageIndexContainingAddress(address); - if (idx == UINT_MAX) { + BSG_Mach_Binary_Image_Info *image = bsg_mach_image_at_address(address); + if (image == NULL) { return false; } - const struct mach_header *header = bsg_dyld_get_image_header(idx); - if (header == NULL) { - return false; - } - const uintptr_t imageVMAddrSlide = - bsg_dyld_get_image_vmaddr_slide(idx); - const uintptr_t addressWithSlide = address - imageVMAddrSlide; + const uintptr_t addressWithSlide = address - image->slide; const uintptr_t segmentBase = - bsg_ksdlsegmentBaseOfImageIndex(idx) + imageVMAddrSlide; + bsg_mach_image_base_of_image_index(image->header) + image->slide; if (segmentBase == 0) { return false; } - info->dli_fname = bsg_dyld_get_image_name(idx); - info->dli_fbase = (void *)header; + info->dli_fname = image->name; + info->dli_fbase = (void *)image->header; // Find symbol tables and get whichever symbol is closest to the address. const BSG_STRUCT_NLIST *bestMatch = NULL; uintptr_t bestDistance = ULONG_MAX; - uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header); + uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(image->header); if (cmdPtr == 0) { return false; } - for (uint32_t iCmd = 0; iCmd < header->ncmds; iCmd++) { + for (uint32_t iCmd = 0; iCmd < image->header->ncmds; iCmd++) { const struct load_command *loadCmd = (struct load_command *)cmdPtr; if (loadCmd->cmd == LC_SYMTAB) { const struct symtab_command *symtabCmd = @@ -223,7 +106,7 @@ bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) { } if (bestMatch != NULL) { info->dli_saddr = - (void *)(bestMatch->n_value + imageVMAddrSlide); + (void *)(bestMatch->n_value + image->slide); info->dli_sname = (char *)((intptr_t)stringTable + (intptr_t)bestMatch->n_un.n_strx); if (*info->dli_sname == '_') { @@ -243,61 +126,3 @@ bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) { return true; } -const void *bsg_ksdlgetSymbolAddrInImage(uint32_t imageIdx, - const char *symbolName) { - const struct mach_header *header = bsg_dyld_get_image_header(imageIdx); - if (header == NULL) { - return NULL; - } - const uintptr_t imageVMAddrSlide = - bsg_dyld_get_image_vmaddr_slide(imageIdx); - const uintptr_t segmentBase = - bsg_ksdlsegmentBaseOfImageIndex(imageIdx) + imageVMAddrSlide; - if (segmentBase == 0) { - return NULL; - } - uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header); - if (cmdPtr == 0) { - return NULL; - } - for (uint32_t iCmd = 0; iCmd < header->ncmds; iCmd++) { - const struct load_command *loadCmd = (struct load_command *)cmdPtr; - if (loadCmd->cmd == LC_SYMTAB) { - const struct symtab_command *symtabCmd = - (struct symtab_command *)cmdPtr; - const BSG_STRUCT_NLIST *symbolTable = - (BSG_STRUCT_NLIST *)(segmentBase + symtabCmd->symoff); - const uintptr_t stringTable = segmentBase + symtabCmd->stroff; - - for (uint32_t iSym = 0; iSym < symtabCmd->nsyms; iSym++) { - // If n_value is 0, the symbol refers to an external object. - if (symbolTable[iSym].n_value != 0) { - const char *sname = - (char *)((intptr_t)stringTable + - (intptr_t)symbolTable[iSym].n_un.n_strx); - if (*sname == '_') { - sname++; - } - if (strcmp(sname, symbolName) == 0) { - return (void *)(symbolTable[iSym].n_value + - imageVMAddrSlide); - } - } - } - } - cmdPtr += loadCmd->cmdsize; - } - return NULL; -} - -const void *bsg_ksdlgetSymbolAddrInAnyImage(const char *symbolName) { - const uint32_t imageCount = bsg_dyld_image_count(); - - for (uint32_t iImg = 0; iImg < imageCount; iImg++) { - const void *symbolAddr = bsg_ksdlgetSymbolAddrInImage(iImg, symbolName); - if (symbolAddr != NULL) { - return symbolAddr; - } - } - return NULL; -} diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.h b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.h index b3f8f96d8..adb5f48d2 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.h +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.h @@ -34,15 +34,7 @@ extern "C" { #include #include -/** Find a loaded binary image with the specified name. - * - * @param imageName The image name to look for. - * - * @param exactMatch If true, look for an exact match instead of a partial one. - * - * @return the index of the matched image, or UINT32_MAX if not found. - */ -uint32_t bsg_ksdlimageNamed(const char *const imageName, bool exactMatch); +#include "BSG_KSMachHeaders.h" /** Get the UUID of a loaded binary image with the specified name. * @@ -55,32 +47,6 @@ uint32_t bsg_ksdlimageNamed(const char *const imageName, bool exactMatch); */ const uint8_t *bsg_ksdlimageUUID(const char *const imageName, bool exactMatch); -/** Get the address of the first command following a header (which will be of - * type struct load_command). - * - * @param header The header to get commands for. - * - * @return The address of the first command, or NULL if none was found (which - * should not happen unless the header or image is corrupt). - */ -uintptr_t bsg_ksdlfirstCmdAfterHeader(const struct mach_header *header); - -/** Get the image index that the specified address is part of. - * - * @param address The address to examine. - * @return The index of the image it is part of, or UINT_MAX if none was found. - */ -uint32_t bsg_ksdlimageIndexContainingAddress(const uintptr_t address); - -/** Get the segment base address of the specified image. - * - * This is required for any symtab command offsets. - * - * @param idx The image index. - * @return The image's base address, or 0 if none was found. - */ -uintptr_t bsg_ksdlsegmentBaseOfImageIndex(const uint32_t idx); - /** async-safe version of dladdr. * * This method searches the dynamic loader for information about any image @@ -97,23 +63,6 @@ uintptr_t bsg_ksdlsegmentBaseOfImageIndex(const uint32_t idx); */ bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info); -/** Get the address of a symbol in the specified image. - * - * @param imageIdx The index of the image to search. - * @param symbolName The symbol to search for. - * @return The address of the symbol or NULL if not found. - */ -const void *bsg_ksdlgetSymbolAddrInImage(uint32_t imageIdx, - const char *symbolName); - -/** Get the address of a symbol in any image. - * Searches all images starting at index 0. - * - * @param symbolName The symbol to search for. - * @return The address of the symbol or NULL if not found. - */ -const void *bsg_ksdlgetSymbolAddrInAnyImage(const char *symbolName); - #ifdef __cplusplus } #endif diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h index a23313898..b229d1f68 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h @@ -26,66 +26,17 @@ typedef struct { intptr_t slide; } BSG_Mach_Binary_Image_Info; -/** - * MARK: - A Dynamic array container - * See: https://stackoverflow.com/a/3536261/2431627 - */ -typedef struct { - uint32_t used; - uint32_t size; - BSG_Mach_Binary_Image_Info *contents; -} BSG_Mach_Binary_Images; - -static BSG_Mach_Binary_Images bsg_mach_binary_images; - -// MARK: - Locking - -/** - * An OS-version-specific lock, used to synchronise access to the array of binary image info. A combination of - * compile-time determination of the OS and and run-time determination of the OS version is used to ensure that - * the correct lock mechanism is used. - * - * os_unfair_lock is available from specific OS versions onwards: - * https://developer.apple.com/documentation/os/os_unfair_lock - * - * It deprecates OSSpinLock: - * https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/spinlock.3.html - * - * The imported headers have specific version info: and - */ - -void bsg_spin_lock(void); -void bsg_spin_unlock(void); -void bsg_unfair_lock(void); -void bsg_unfair_unlock(void); -void bsg_dyld_cache_lock(void); -void bsg_dyld_cache_unlock(void); - // MARK: - Replicate the DYLD API /** - * Returns the current number of images mapped in by dyld + * Returns the current number of images mapped in by dyld. */ uint32_t bsg_dyld_image_count(void); /** - * Returns a pointer to the mach header of the image indexed by image_index. If imageIndex - * is out of range, NULL is returned. - */ -const struct mach_header* bsg_dyld_get_image_header(uint32_t imageIndex); - -/** - * Returns the virtural memory address slide amount of the image indexed by imageIndex. - * If image_index is out of range zero is returned. - */ -intptr_t bsg_dyld_get_image_vmaddr_slide(uint32_t imageIndex); - -/** - * Returns the name of the image indexed by imageIndex. - */ -const char* bsg_dyld_get_image_name(uint32_t imageIndex); - -BSG_Mach_Binary_Image_Info *bsg_dyld_get_image_info(uint32_t imageIndex); // An additional convenience function +* Returns the binary image information at the specified index. +*/ +BSG_Mach_Binary_Image_Info *bsg_dyld_get_image_info(uint32_t imageIndex); /** * Called when a binary image is loaded. @@ -100,11 +51,43 @@ void bsg_mach_binary_image_removed(const struct mach_header *mh, intptr_t slide) /** * Create an empty array with initial capacity to hold Mach header info. */ -BSG_Mach_Binary_Images *bsg_initialise_mach_binary_headers(uint32_t initialSize); +void bsg_initialise_mach_binary_headers(uint32_t initialSize); -/** - * Determines whether the OS supports unfair locks or not. +/** Get the image index that the specified address is part of. +* +* @param address The address to examine. +* @return The index of the image it is part of, or UINT_MAX if none was found. +*/ +BSG_Mach_Binary_Image_Info *bsg_mach_image_at_address(const uintptr_t address); + + +/** Find a loaded binary image with the specified name. + * + * @param imageName The image name to look for. + * + * @param exactMatch If true, look for an exact match instead of a partial one. + * + * @return the index of the matched image, or UINT32_MAX if not found. + */ +BSG_Mach_Binary_Image_Info *bsg_mach_image_named(const char *const imageName, bool exactMatch); + +/** Get the address of the first command following a header (which will be of + * type struct load_command). + * + * @param header The header to get commands for. + * + * @return The address of the first command, or NULL if none was found (which + * should not happen unless the header or image is corrupt). + */ +uintptr_t bsg_mach_image_first_cmd_after_header(const struct mach_header *header); + +/** Get the segment base address of the specified image. + * + * This is required for any symtab command offsets. + * + * @param header The header to get commands for. + * @return The image's base address, or 0 if none was found. */ -void bsg_check_unfair_lock_support(void); +uintptr_t bsg_mach_image_base_of_image_index(const struct mach_header *header); #endif /* BSG_KSMachHeaders_h */ diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m index f429b4b68..c2daef7d5 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m @@ -76,7 +76,7 @@ void bsg_dyld_cache_unlock() { } } -BOOL IsUnfairLockSupported(NSProcessInfo *processInfo) { +BOOL bsg_is_unfair_lock_supported(NSProcessInfo *processInfo) { NSOperatingSystemVersion minSdk = {0,0,0}; #if BSG_PLATFORM_IOS minSdk.majorVersion = 10; @@ -91,37 +91,24 @@ BOOL IsUnfairLockSupported(NSProcessInfo *processInfo) { return [processInfo isOperatingSystemAtLeastVersion:minSdk]; } -void bsg_check_unfair_lock_support() { - bsg_unfair_lock_supported = IsUnfairLockSupported([NSProcessInfo processInfo]); -} +/** + * MARK: - A Dynamic array container + * See: https://stackoverflow.com/a/3536261/2431627 + */ +typedef struct { + uint32_t used; + uint32_t size; + BSG_Mach_Binary_Image_Info *contents; +} BSG_Mach_Binary_Images; // MARK: - Replicate the DYLD API +static BSG_Mach_Binary_Images bsg_mach_binary_images; + uint32_t bsg_dyld_image_count(void) { return bsg_mach_binary_images.used; } -const struct mach_header* bsg_dyld_get_image_header(uint32_t imageIndex) { - if (imageIndex < bsg_mach_binary_images.used) { - return bsg_mach_binary_images.contents[imageIndex].header; - } - return NULL; -} - -intptr_t bsg_dyld_get_image_vmaddr_slide(uint32_t imageIndex) { - if (imageIndex < bsg_mach_binary_images.used) { - return bsg_mach_binary_images.contents[imageIndex].slide; - } - return 0; -} - -const char* bsg_dyld_get_image_name(uint32_t imageIndex) { - if (imageIndex < bsg_mach_binary_images.used) { - return bsg_mach_binary_images.contents[imageIndex].name; - } - return NULL; -} - BSG_Mach_Binary_Image_Info *bsg_dyld_get_image_info(uint32_t imageIndex) { if (imageIndex < bsg_mach_binary_images.used) { return &bsg_mach_binary_images.contents[imageIndex]; @@ -194,14 +181,12 @@ void bsg_remove_mach_binary_image(uint64_t imageVmAddr) { * Create an empty array with initial capacity to hold Mach header info. * * @param initialSize The initial array capacity - * - * @returns A struct for holding Mach binary image info */ -BSG_Mach_Binary_Images *bsg_initialise_mach_binary_headers(uint32_t initialSize) { +void bsg_initialise_mach_binary_headers(uint32_t initialSize) { + bsg_unfair_lock_supported = bsg_is_unfair_lock_supported([NSProcessInfo processInfo]); bsg_mach_binary_images.contents = (BSG_Mach_Binary_Image_Info *)malloc(initialSize * sizeof(BSG_Mach_Binary_Image_Info)); bsg_mach_binary_images.used = 0; bsg_mach_binary_images.size = initialSize; - return &bsg_mach_binary_images; } /** @@ -217,7 +202,7 @@ bool bsg_populate_mach_image_info(const struct mach_header *header, intptr_t sli // Early exit conditions; this is not a valid/useful binary image // 1. We can't find a sensible Mach command - uintptr_t cmdPtr = bsg_ksdlfirstCmdAfterHeader(header); + uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(header); if (cmdPtr == 0) { return false; } @@ -304,3 +289,114 @@ void bsg_mach_binary_image_removed(const struct mach_header *header, intptr_t sl bsg_remove_mach_binary_image(info.imageVmAddr); } } + +BSG_Mach_Binary_Image_Info *bsg_mach_image_named(const char *const imageName, bool exactMatch) { + + BSG_Mach_Binary_Image_Info *imageFound = NULL; + + if (imageName != NULL) { + for (uint32_t iImg = 0; iImg < bsg_dyld_image_count(); iImg++) { + BSG_Mach_Binary_Image_Info *img = bsg_dyld_get_image_info(iImg); + if (img->name == NULL) { + continue; // name is null if the index is out of range per dyld(3) + } else if (exactMatch) { + if (strcmp(img->name, imageName) == 0) { + imageFound = img; + break; + } + } else { + if (strstr(img->name, imageName) != NULL) { + imageFound = img; + break; + } + } + } + } + + return imageFound; +} + +BSG_Mach_Binary_Image_Info *bsg_mach_image_at_address(const uintptr_t address) { + + BSG_Mach_Binary_Image_Info *imageFound = NULL; + + for (uint32_t iImg = 0; iImg < bsg_dyld_image_count(); iImg++) { + BSG_Mach_Binary_Image_Info *img = bsg_dyld_get_image_info(iImg); + if (img->header != NULL) { + // Look for a segment command with this address within its range. + uintptr_t addressWSlide = address - img->slide; + uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(img->header); + if (cmdPtr == 0) { + continue; + } + for (uint32_t iCmd = 0; iCmd < img->header->ncmds; iCmd++) { + const struct load_command *loadCmd = + (struct load_command *)cmdPtr; + if (loadCmd->cmd == LC_SEGMENT) { + const struct segment_command *segCmd = + (struct segment_command *)cmdPtr; + if (addressWSlide >= segCmd->vmaddr && + addressWSlide < segCmd->vmaddr + segCmd->vmsize) { + imageFound = img; + break; + } + } else if (loadCmd->cmd == LC_SEGMENT_64) { + const struct segment_command_64 *segCmd = + (struct segment_command_64 *)cmdPtr; + if (addressWSlide >= segCmd->vmaddr && + addressWSlide < segCmd->vmaddr + segCmd->vmsize) { + imageFound = img; + break; + } + } + cmdPtr += loadCmd->cmdsize; + } + } + } + + return imageFound; +} + +uintptr_t bsg_mach_image_first_cmd_after_header(const struct mach_header *const header) { + if (header == NULL) { + return 0; + } + switch (header->magic) { + case MH_MAGIC: + case MH_CIGAM: + return (uintptr_t)(header + 1); + case MH_MAGIC_64: + case MH_CIGAM_64: + return (uintptr_t)(((struct mach_header_64 *)header) + 1); + default: + // Header is corrupt + return 0; + } +} + +uintptr_t bsg_mach_image_base_of_image_index(const struct mach_header *const header) { + // Look for a segment command and return the file image address. + uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(header); + if (cmdPtr == 0) { + return 0; + } + for (uint32_t i = 0; i < header->ncmds; i++) { + const struct load_command *loadCmd = (struct load_command *)cmdPtr; + if (loadCmd->cmd == LC_SEGMENT) { + const struct segment_command *segmentCmd = + (struct segment_command *)cmdPtr; + if (strcmp(segmentCmd->segname, SEG_LINKEDIT) == 0) { + return segmentCmd->vmaddr - segmentCmd->fileoff; + } + } else if (loadCmd->cmd == LC_SEGMENT_64) { + const struct segment_command_64 *segmentCmd = + (struct segment_command_64 *)cmdPtr; + if (strcmp(segmentCmd->segname, SEG_LINKEDIT) == 0) { + return (uintptr_t)(segmentCmd->vmaddr - segmentCmd->fileoff); + } + } + cmdPtr += loadCmd->cmdsize; + } + + return 0; +} diff --git a/Tests/KSCrash/KSDynamicLinker_Tests.m b/Tests/KSCrash/KSDynamicLinker_Tests.m index 9f8864478..320c62a3a 100755 --- a/Tests/KSCrash/KSDynamicLinker_Tests.m +++ b/Tests/KSCrash/KSDynamicLinker_Tests.m @@ -59,11 +59,5 @@ - (void) testImageUUIDPartialMatch XCTAssertTrue(uuidBytes != NULL, @""); } -- (void) testGetImageNameNULL -{ - uint32_t imageIdx = bsg_ksdlimageNamed(NULL, false); - XCTAssertEqual(imageIdx, UINT32_MAX, @""); -} - @end diff --git a/Tests/KSCrash/KSMachHeader_Tests.m b/Tests/KSCrash/KSMachHeader_Tests.m index d4d355332..9d0b512ef 100644 --- a/Tests/KSCrash/KSMachHeader_Tests.m +++ b/Tests/KSCrash/KSMachHeader_Tests.m @@ -36,138 +36,118 @@ @implementation KSMachHeader_Tests - (void)testDynamicArray { - BSG_Mach_Binary_Images *images = bsg_initialise_mach_binary_headers(2); - XCTAssertEqual(images->size, 2); + bsg_initialise_mach_binary_headers(2); XCTAssertEqual(bsg_dyld_image_count(), 0); // Add bsg_add_mach_binary_image(info1); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 1); bsg_add_mach_binary_image(info2); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 2); // Expand - double size bsg_add_mach_binary_image(info3); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 3); // Delete - third will be copied bsg_remove_mach_binary_image(12345); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 2); - XCTAssertEqual(strcmp(bsg_dyld_get_image_name(0), "header the third"), 0); - XCTAssertEqual(strcmp(bsg_dyld_get_image_name(1), "header the second"), 0); - XCTAssertEqual(images->size, 4); + XCTAssertEqual(strcmp(bsg_dyld_get_image_info(0)->name, "header the third"), 0); + XCTAssertEqual(strcmp(bsg_dyld_get_image_info(1)->name, "header the second"), 0); // Nothing happens bsg_remove_mach_binary_image(12345); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 2); bsg_remove_mach_binary_image(23456); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 1); - XCTAssertEqual(strcmp(bsg_dyld_get_image_name(0), "header the third"), 0); + XCTAssertEqual(strcmp(bsg_dyld_get_image_info(0)->name, "header the third"), 0); bsg_remove_mach_binary_image(34567); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 0); bsg_remove_mach_binary_image(34567); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 0); // Readd bsg_add_mach_binary_image(info1); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 1); } - (void)testRemoveLast1 { - BSG_Mach_Binary_Images *images = bsg_initialise_mach_binary_headers(2); + bsg_initialise_mach_binary_headers(2); bsg_add_mach_binary_image(info1); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 1); bsg_remove_mach_binary_image(12345); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 0); } - (void)testRemoveLast2 { - BSG_Mach_Binary_Images *images = bsg_initialise_mach_binary_headers(2); + bsg_initialise_mach_binary_headers(2); bsg_add_mach_binary_image(info1); bsg_add_mach_binary_image(info2); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 2); bsg_remove_mach_binary_image(23456); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 1); bsg_remove_mach_binary_image(12345); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 0); } - (void)testRemoveLast3 { - BSG_Mach_Binary_Images *images = bsg_initialise_mach_binary_headers(2); + bsg_initialise_mach_binary_headers(2); bsg_add_mach_binary_image(info1); bsg_add_mach_binary_image(info2); bsg_add_mach_binary_image(info3); bsg_add_mach_binary_image(info4); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 4); bsg_remove_mach_binary_image(45678); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 3); bsg_remove_mach_binary_image(12345); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 2); bsg_remove_mach_binary_image(12345); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 2); bsg_remove_mach_binary_image(34567); - XCTAssertEqual(images->size, 4); XCTAssertEqual(bsg_dyld_image_count(), 1); } // Test out-of-bounds behaviour of the replicated dyld API - (void)testBSGDYLDAPI { - BSG_Mach_Binary_Images *images = bsg_initialise_mach_binary_headers(2); + bsg_initialise_mach_binary_headers(2); bsg_add_mach_binary_image(info1); bsg_add_mach_binary_image(info2); - XCTAssertEqual(images->size, 2); XCTAssertEqual(bsg_dyld_image_count(), 2); - XCTAssertEqual(bsg_dyld_get_image_vmaddr_slide(0), 123); - XCTAssertEqual(bsg_dyld_get_image_vmaddr_slide(1), 1234); - XCTAssertEqual(bsg_dyld_get_image_vmaddr_slide(2), 0); - XCTAssertEqual(bsg_dyld_get_image_vmaddr_slide(999), 0); + XCTAssertTrue(bsg_dyld_get_image_info(2) == NULL); + XCTAssertTrue(bsg_dyld_get_image_info(999) == NULL); + + XCTAssertEqual(bsg_dyld_get_image_info(0)->slide, 123); + XCTAssertEqual(bsg_dyld_get_image_info(1)->slide, 1234); - XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_name(0)], @"header the first"); - XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_name(1)], @"header the second"); - XCTAssertTrue(bsg_dyld_get_image_name(2) == NULL); - XCTAssertTrue(bsg_dyld_get_image_name(999) == NULL); - - XCTAssertEqual(bsg_dyld_get_image_header(0)->filetype, 1); - XCTAssertEqual(bsg_dyld_get_image_header(1)->filetype, 1); - XCTAssertTrue(bsg_dyld_get_image_header(2) == NULL); - XCTAssertTrue(bsg_dyld_get_image_header(999) == NULL); + XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(0)->name], @"header the first"); + XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(1)->name], @"header the second"); + + XCTAssertEqual(bsg_dyld_get_image_info(0)->header->filetype, 1); + XCTAssertEqual(bsg_dyld_get_image_info(1)->header->filetype, 1); XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(0)->name], @"header the first"); XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(1)->name], @"header the second"); - XCTAssertTrue(bsg_dyld_get_image_info(999) == NULL); +} + +- (void) testGetImageNameNULL +{ + BSG_Mach_Binary_Image_Info *img = bsg_mach_image_named(NULL, false); + XCTAssertTrue(img == NULL); } @end From 52944fc6143be83c5cf0a2743f0a7798d6a8eb75 Mon Sep 17 00:00:00 2001 From: Tom Longridge Date: Sun, 28 Jun 2020 17:57:35 +0100 Subject: [PATCH 2/5] refactor(mach headers) Refactored mach images into linked list --- .../Source/KSCrash/Recording/BSG_KSCrash.m | 18 -- .../Source/KSCrash/Recording/BSG_KSCrashC.c | 5 +- .../KSCrash/Recording/BSG_KSCrashReport.c | 29 +- .../KSCrash/Recording/BSG_KSSystemInfo.m | 2 +- .../Recording/Tools/BSG_KSDynamicLinker.c | 12 +- .../Recording/Tools/BSG_KSMachHeaders.h | 43 ++- .../Recording/Tools/BSG_KSMachHeaders.m | 292 +++++++----------- Tests/KSCrash/KSDynamicLinker_Tests.m | 14 + Tests/KSCrash/KSMachHeader_Tests.m | 171 ++++------ 9 files changed, 230 insertions(+), 356 deletions(-) diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m index 2a30f7ee2..8c5e1cc7b 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m @@ -26,14 +26,12 @@ #import "BugsnagPlatformConditional.h" -#import #import #import "BSG_KSCrashAdvanced.h" #import "BSG_KSCrashC.h" #import "BSG_KSJSONCodecObjC.h" #import "BSG_KSSingleton.h" -#import "BSG_KSMachHeaders.h" #import "NSError+BSG_SimpleConstructor.h" //#define BSG_KSLogger_LocalLevel TRACE @@ -234,8 +232,6 @@ - (NSString *)stateFilePath { } - (BOOL)install { - // Maintain a cache of info about dynamically loaded binary images - [self listenForLoadedBinaries]; _handlingCrashTypes = bsg_kscrash_install( [self.crashReportPath UTF8String], [self.recrashReportPath UTF8String], @@ -271,20 +267,6 @@ - (BOOL)install { return true; } -/** - * Set up listeners for un/loaded frameworks. Maintaining our own list of framework Mach - * headers means that we avoid potential deadlock situations where we try and suspend - * lock-holding threads prior to loading mach headers as part of our normal event handling - * behaviour. - */ -- (void)listenForLoadedBinaries { - bsg_initialise_mach_binary_headers(BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE); - - // Note: Access to DYLD's binary image store is guarded by locks. - _dyld_register_func_for_remove_image(&bsg_mach_binary_image_removed); - _dyld_register_func_for_add_image(&bsg_mach_binary_image_added); -} - - (void)sendAllReports { [self.crashReportStore pruneFilesLeaving:self.maxStoredReports]; diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c index 1981d84c5..c7183b40f 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c @@ -29,6 +29,7 @@ #include "BSG_KSCrashReport.h" #include "BSG_KSCrashSentry_User.h" #include "BSG_KSMach.h" +#include "BSG_KSMachHeaders.h" #include "BSG_KSObjC.h" #include "BSG_KSString.h" #include "BSG_KSSystemInfoC.h" @@ -97,7 +98,9 @@ BSG_KSCrashType bsg_kscrash_install(const char *const crashReportFilePath, BSG_KSLOG_DEBUG("Installing crash reporter."); BSG_KSCrash_Context *context = crashContext(); - + + bsg_mach_headers_initialize(); + if (bsg_g_installed) { BSG_KSLOG_DEBUG("Crash reporter already installed."); return context->config.handlingCrashTypes; diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c index 432974d50..7c8d07c49 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c @@ -1104,23 +1104,21 @@ int bsg_kscrw_i_threadIndex(const thread_t thread) { * * @param key The object key, if needed. * - * @param index Cached info about the binary image. + * @param img Cached info about the binary image. */ void bsg_kscrw_i_writeBinaryImage(const BSG_KSCrashReportWriter *const writer, const char *const key, - const uint32_t index) + const BSG_Mach_Header_Info *img) { - BSG_Mach_Binary_Image_Info info = *bsg_dyld_get_image_info(index); - writer->beginObject(writer, key); { - writer->addUIntegerElement(writer, BSG_KSCrashField_ImageAddress, (uintptr_t)info.header); - writer->addUIntegerElement(writer, BSG_KSCrashField_ImageVmAddress, info.imageVmAddr); - writer->addUIntegerElement(writer, BSG_KSCrashField_ImageSize, info.imageSize); - writer->addStringElement(writer, BSG_KSCrashField_Name, info.name); - writer->addUUIDElement(writer, BSG_KSCrashField_UUID, info.uuid); - writer->addIntegerElement(writer, BSG_KSCrashField_CPUType, info.header->cputype); - writer->addIntegerElement(writer, BSG_KSCrashField_CPUSubType, info.header->cpusubtype); + writer->addUIntegerElement(writer, BSG_KSCrashField_ImageAddress, (uintptr_t)img->header); + writer->addUIntegerElement(writer, BSG_KSCrashField_ImageVmAddress, img->imageVmAddr); + writer->addUIntegerElement(writer, BSG_KSCrashField_ImageSize, img->imageSize); + writer->addStringElement(writer, BSG_KSCrashField_Name, img->name); + writer->addUUIDElement(writer, BSG_KSCrashField_UUID, img->uuid); + writer->addIntegerElement(writer, BSG_KSCrashField_CPUType, img->header->cputype); + writer->addIntegerElement(writer, BSG_KSCrashField_CPUSubType, img->header->cpusubtype); } writer->endContainer(writer); } @@ -1134,13 +1132,12 @@ void bsg_kscrw_i_writeBinaryImage(const BSG_KSCrashReportWriter *const writer, void bsg_kscrw_i_writeBinaryImages(const BSG_KSCrashReportWriter *const writer, const char *const key) { - const uint32_t imageCount = bsg_dyld_image_count(); - writer->beginArray(writer, key); { - for (uint32_t iImg = 0; iImg < imageCount; iImg++) { - // Threads are suspended at this point; no need to synchronise/lock - bsg_kscrw_i_writeBinaryImage(writer, NULL, iImg); + for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { + if (!img->removed) { + bsg_kscrw_i_writeBinaryImage(writer, NULL, img); + } } } writer->endContainer(writer); diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m index 8da7f995b..109ecac92 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSSystemInfo.m @@ -260,7 +260,7 @@ + (NSString *)currentCPUArch { * @return YES if the device is jailbroken. */ + (BOOL)isJailbroken { - return bsg_mach_image_named("MobileSubstrate", false) != NULL; + return bsg_mach_headers_image_named("MobileSubstrate", false) != NULL; } /** Check if the current build is a debug build. diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c index 9905fcae6..5423520d6 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c @@ -34,9 +34,9 @@ const uint8_t *bsg_ksdlimageUUID(const char *const imageName, bool exactMatch) { if (imageName != NULL) { - BSG_Mach_Binary_Image_Info *img = bsg_mach_image_named(imageName, exactMatch); + BSG_Mach_Header_Info *img = bsg_mach_headers_image_named(imageName, exactMatch); if (img != NULL) { - uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(img->header); + uintptr_t cmdPtr = bsg_mach_headers_first_cmd_after_header(img->header); if (cmdPtr != 0) { for (uint32_t iCmd = 0; iCmd < img->header->ncmds; iCmd++) { const struct load_command *loadCmd = @@ -54,21 +54,19 @@ const uint8_t *bsg_ksdlimageUUID(const char *const imageName, bool exactMatch) { return NULL; } - - bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) { info->dli_fname = NULL; info->dli_fbase = NULL; info->dli_sname = NULL; info->dli_saddr = NULL; - BSG_Mach_Binary_Image_Info *image = bsg_mach_image_at_address(address); + BSG_Mach_Header_Info *image = bsg_mach_headers_image_at_address(address); if (image == NULL) { return false; } const uintptr_t addressWithSlide = address - image->slide; const uintptr_t segmentBase = - bsg_mach_image_base_of_image_index(image->header) + image->slide; + bsg_mach_headers_image_at_base_of_image_index(image->header) + image->slide; if (segmentBase == 0) { return false; } @@ -79,7 +77,7 @@ bool bsg_ksdldladdr(const uintptr_t address, Dl_info *const info) { // Find symbol tables and get whichever symbol is closest to the address. const BSG_STRUCT_NLIST *bestMatch = NULL; uintptr_t bestDistance = ULONG_MAX; - uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(image->header); + uintptr_t cmdPtr = bsg_mach_headers_first_cmd_after_header(image->header); if (cmdPtr == 0) { return false; } diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h index b229d1f68..873662798 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h @@ -14,51 +14,50 @@ #import /** - * An encapsulation of the Mach header - either 64 or 32 bit, along with some additional information required for - * detailing a crash report's binary images. + * An encapsulation of the Mach header data as a linked list - either 64 or 32 bit, along with some additiona + * information required for detailing a crash report's binary images. + * + * The removed field indicates that the library has since been unloaded by dyld and can be ignored. */ -typedef struct { +typedef struct bsg_mach_image { const struct mach_header *header; /* The mach_header - 32 or 64 bit */ uint64_t imageVmAddr; uint64_t imageSize; uint8_t *uuid; const char* name; intptr_t slide; -} BSG_Mach_Binary_Image_Info; + bool removed; + struct bsg_mach_image *next; +} BSG_Mach_Header_Info; -// MARK: - Replicate the DYLD API +// MARK: - Operations /** - * Returns the current number of images mapped in by dyld. + * Resets mach header data */ -uint32_t bsg_dyld_image_count(void); +void bsg_mach_headers_initialize(void); /** -* Returns the binary image information at the specified index. -*/ -BSG_Mach_Binary_Image_Info *bsg_dyld_get_image_info(uint32_t imageIndex); + * Returns the head of the link list of headers + */ +BSG_Mach_Header_Info *bsg_mach_headers_get_images(void); /** * Called when a binary image is loaded. */ -void bsg_mach_binary_image_added(const struct mach_header *mh, intptr_t slide); +void bsg_mach_headers_add_image(const struct mach_header *mh, intptr_t slide); /** * Called when a binary image is unloaded. */ -void bsg_mach_binary_image_removed(const struct mach_header *mh, intptr_t slide); - -/** - * Create an empty array with initial capacity to hold Mach header info. - */ -void bsg_initialise_mach_binary_headers(uint32_t initialSize); +void bsg_mach_headers_remove_image(const struct mach_header *mh, intptr_t slide); /** Get the image index that the specified address is part of. * * @param address The address to examine. * @return The index of the image it is part of, or UINT_MAX if none was found. */ -BSG_Mach_Binary_Image_Info *bsg_mach_image_at_address(const uintptr_t address); +BSG_Mach_Header_Info *bsg_mach_headers_image_at_address(const uintptr_t address); /** Find a loaded binary image with the specified name. @@ -67,9 +66,9 @@ BSG_Mach_Binary_Image_Info *bsg_mach_image_at_address(const uintptr_t address); * * @param exactMatch If true, look for an exact match instead of a partial one. * - * @return the index of the matched image, or UINT32_MAX if not found. + * @return the matched image, or NULL if not found. */ -BSG_Mach_Binary_Image_Info *bsg_mach_image_named(const char *const imageName, bool exactMatch); +BSG_Mach_Header_Info *bsg_mach_headers_image_named(const char *const imageName, bool exactMatch); /** Get the address of the first command following a header (which will be of * type struct load_command). @@ -79,7 +78,7 @@ BSG_Mach_Binary_Image_Info *bsg_mach_image_named(const char *const imageName, bo * @return The address of the first command, or NULL if none was found (which * should not happen unless the header or image is corrupt). */ -uintptr_t bsg_mach_image_first_cmd_after_header(const struct mach_header *header); +uintptr_t bsg_mach_headers_first_cmd_after_header(const struct mach_header *header); /** Get the segment base address of the specified image. * @@ -88,6 +87,6 @@ uintptr_t bsg_mach_image_first_cmd_after_header(const struct mach_header *header * @param header The header to get commands for. * @return The image's base address, or 0 if none was found. */ -uintptr_t bsg_mach_image_base_of_image_index(const struct mach_header *header); +uintptr_t bsg_mach_headers_image_at_base_of_image_index(const struct mach_header *header); #endif /* BSG_KSMachHeaders_h */ diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m index c2daef7d5..7ee3963ce 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m @@ -26,8 +26,6 @@ static OSSpinLock bsg_mach_binary_images_access_lock_spin = OS_SPINLOCK_INIT; _Pragma("clang diagnostic pop") -static BOOL bsg_unfair_lock_supported; - // Lock helpers. These use bulky Pragmas to hide warnings so are in their own functions for clarity. void bsg_spin_lock() { @@ -58,25 +56,7 @@ void bsg_unfair_unlock() { _Pragma("clang diagnostic pop") } -// Lock and unlock sections of code - -void bsg_dyld_cache_lock() { - if (bsg_unfair_lock_supported) { - bsg_unfair_lock(); - } else { - bsg_spin_lock(); - } -} - -void bsg_dyld_cache_unlock() { - if (bsg_unfair_lock_supported) { - bsg_unfair_unlock(); - } else { - bsg_spin_unlock(); - } -} - -BOOL bsg_is_unfair_lock_supported(NSProcessInfo *processInfo) { +BOOL bsg_is_unfair_lock_supported() { NSOperatingSystemVersion minSdk = {0,0,0}; #if BSG_PLATFORM_IOS minSdk.majorVersion = 10; @@ -88,105 +68,58 @@ BOOL bsg_is_unfair_lock_supported(NSProcessInfo *processInfo) { #elif BSG_PLATFORM_WATCHOS minSdk.majorVersion = 3; #endif - return [processInfo isOperatingSystemAtLeastVersion:minSdk]; + return [[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:minSdk]; } -/** - * MARK: - A Dynamic array container - * See: https://stackoverflow.com/a/3536261/2431627 - */ -typedef struct { - uint32_t used; - uint32_t size; - BSG_Mach_Binary_Image_Info *contents; -} BSG_Mach_Binary_Images; - -// MARK: - Replicate the DYLD API - -static BSG_Mach_Binary_Images bsg_mach_binary_images; - -uint32_t bsg_dyld_image_count(void) { - return bsg_mach_binary_images.used; -} +static void (*bsg_mach_headers_cache_lock_func)(void) = NULL; +static void (*bsg_mach_headers_cache_unlock_func)(void) = NULL; -BSG_Mach_Binary_Image_Info *bsg_dyld_get_image_info(uint32_t imageIndex) { - if (imageIndex < bsg_mach_binary_images.used) { - return &bsg_mach_binary_images.contents[imageIndex]; +void bsg_mach_headers_cache_lock() { + if (bsg_mach_headers_cache_lock_func == NULL) { + if (bsg_is_unfair_lock_supported()) { + bsg_mach_headers_cache_lock_func = &bsg_unfair_lock; + } else { + bsg_mach_headers_cache_lock_func = &bsg_spin_lock; + } } - return NULL; + bsg_mach_headers_cache_lock_func(); } -/** - * Store a Mach binary image-excapsulating struct in a dynamic array. - * The array doubles on filling-up. Typical sizes is expected to be in < 1000 (i.e. 2-3 doublings, at app start-up) - * This should be called in a threadsafe way; we lock against a simultaneous add and remove. - */ -void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element) { - - bsg_dyld_cache_lock(); - - // Expand array if necessary. We're slightly paranoid here. An OOM is likely to be indicative of bigger problems - // but we should still do *our* best not to crash the app. - if (bsg_mach_binary_images.used == bsg_mach_binary_images.size) { - uint32_t newSize = bsg_mach_binary_images.size *= 2; - uint32_t newAllocationSize = newSize * sizeof(BSG_Mach_Binary_Image_Info); - errno = 0; - BSG_Mach_Binary_Image_Info *newAllocation = (BSG_Mach_Binary_Image_Info *)realloc(bsg_mach_binary_images.contents, newAllocationSize); - - if (newAllocation != NULL && errno != ENOMEM) { - bsg_mach_binary_images.size = newSize; - bsg_mach_binary_images.contents = newAllocation; - } - else { - // Exit early, don't expand the array, don't store the header info and unlock - bsg_dyld_cache_unlock(); - return; +void bsg_mach_headers_cache_unlock() { + if (bsg_mach_headers_cache_unlock_func == NULL) { + if (bsg_is_unfair_lock_supported()) { + bsg_mach_headers_cache_unlock_func = &bsg_unfair_unlock; + } else { + bsg_mach_headers_cache_unlock_func = &bsg_spin_unlock; } } - - // Store the value, increment the number of used elements - bsg_mach_binary_images.contents[bsg_mach_binary_images.used++] = element; - - bsg_dyld_cache_unlock(); + bsg_mach_headers_cache_unlock_func(); } -/** - * Binary images can only be loaded at most once. We can use the VMAddress as a key, without needing to compare the - * other fields. Element order is not important; deletion is accomplished by copying the last item into the deleted - * position. - */ -void bsg_remove_mach_binary_image(uint64_t imageVmAddr) { - - bsg_dyld_cache_lock(); +// MARK: - Mach Header Linked List + +static BSG_Mach_Header_Info *bsg_mach_headers_images_head; +static BSG_Mach_Header_Info *bsg_mach_headers_images_tail; + +BSG_Mach_Header_Info *bsg_mach_headers_get_images() { + return bsg_mach_headers_images_head; +} + +void bsg_mach_headers_initialize() { - for (uint32_t i=0; ilast. - if (bsg_mach_binary_images.used >= 2) { - bsg_mach_binary_images.contents[i] = bsg_mach_binary_images.contents[--bsg_mach_binary_images.used]; - } - else { - bsg_mach_binary_images.used = 0; - } - break; // an image can only be loaded singularly; exit loop once found - } + // Free any existing data (this may be present in tests) + for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; ) { + BSG_Mach_Header_Info *imgToDelete = img; + img = img->next; + free(imgToDelete); } - bsg_dyld_cache_unlock(); -} - -/** - * Create an empty array with initial capacity to hold Mach header info. - * - * @param initialSize The initial array capacity -*/ -void bsg_initialise_mach_binary_headers(uint32_t initialSize) { - bsg_unfair_lock_supported = bsg_is_unfair_lock_supported([NSProcessInfo processInfo]); - bsg_mach_binary_images.contents = (BSG_Mach_Binary_Image_Info *)malloc(initialSize * sizeof(BSG_Mach_Binary_Image_Info)); - bsg_mach_binary_images.used = 0; - bsg_mach_binary_images.size = initialSize; + bsg_mach_headers_images_head = NULL; + bsg_mach_headers_images_tail = NULL; + + // Register for binary images being loaded and unloaded + _dyld_register_func_for_add_image(&bsg_mach_headers_add_image); + _dyld_register_func_for_remove_image(&bsg_mach_headers_remove_image); } /** @@ -198,11 +131,11 @@ void bsg_initialise_mach_binary_headers(uint32_t initialSize) { * * @returns a boolean indicating success */ -bool bsg_populate_mach_image_info(const struct mach_header *header, intptr_t slide, BSG_Mach_Binary_Image_Info *info) { +bool bsg_mach_headers_populate_info(const struct mach_header *header, intptr_t slide, BSG_Mach_Header_Info *info) { // Early exit conditions; this is not a valid/useful binary image // 1. We can't find a sensible Mach command - uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(header); + uintptr_t cmdPtr = bsg_mach_headers_first_cmd_after_header(header); if (cmdPtr == 0) { return false; } @@ -257,107 +190,106 @@ bool bsg_populate_mach_image_info(const struct mach_header *header, intptr_t sli info->uuid = uuid; info->name = imageName; info->slide = slide; + info->removed = FALSE; + info->next = NULL; return true; } -/** - * A callback invoked when dyld loads binary images. It stores enough relevant info about the - * image to populate a crash report later. - * - * @param header A mach_header structure - * - * @param slide A virtual memory slide amount. The virtual memory slide amount specifies the difference between the - * address at which the image was linked and the address at which the image is loaded. - */ -void bsg_mach_binary_image_added(const struct mach_header *header, intptr_t slide) -{ - BSG_Mach_Binary_Image_Info info = { 0 }; - if (bsg_populate_mach_image_info(header, slide, &info)) { - bsg_add_mach_binary_image(info); +void bsg_mach_headers_add_image(const struct mach_header *header, intptr_t slide) { + + BSG_Mach_Header_Info *newImage = malloc(sizeof(BSG_Mach_Header_Info)); + if (newImage != NULL && errno != ENOMEM) { + if (bsg_mach_headers_populate_info(header, slide, newImage)) { + + bsg_mach_headers_cache_lock(); + + if (bsg_mach_headers_images_head == NULL) { + bsg_mach_headers_images_head = newImage; + } else { + bsg_mach_headers_images_tail->next = newImage; + } + bsg_mach_headers_images_tail = newImage; + + bsg_mach_headers_cache_unlock(); + } } + } -/** - * Called when a binary image is unloaded. - */ -void bsg_mach_binary_image_removed(const struct mach_header *header, intptr_t slide) -{ - // Convert header into an info struct - BSG_Mach_Binary_Image_Info info = { 0 }; - if (bsg_populate_mach_image_info(header, slide, &info)) { - bsg_remove_mach_binary_image(info.imageVmAddr); +void bsg_mach_headers_remove_image(const struct mach_header *header, intptr_t slide) { + BSG_Mach_Header_Info existingImage = { 0 }; + if (bsg_mach_headers_populate_info(header, slide, &existingImage)) { + for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { + if (img->imageVmAddr == existingImage.imageVmAddr) { + img->removed = true; + } + } } } -BSG_Mach_Binary_Image_Info *bsg_mach_image_named(const char *const imageName, bool exactMatch) { - - BSG_Mach_Binary_Image_Info *imageFound = NULL; - +BSG_Mach_Header_Info *bsg_mach_headers_image_named(const char *const imageName, bool exactMatch) { + if (imageName != NULL) { - for (uint32_t iImg = 0; iImg < bsg_dyld_image_count(); iImg++) { - BSG_Mach_Binary_Image_Info *img = bsg_dyld_get_image_info(iImg); + + for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { if (img->name == NULL) { continue; // name is null if the index is out of range per dyld(3) + } else if (img->removed == true) { + continue; // ignore unloaded libraries } else if (exactMatch) { if (strcmp(img->name, imageName) == 0) { - imageFound = img; - break; + return img; } } else { if (strstr(img->name, imageName) != NULL) { - imageFound = img; - break; + return img; } } } } - return imageFound; + return NULL; } -BSG_Mach_Binary_Image_Info *bsg_mach_image_at_address(const uintptr_t address) { - - BSG_Mach_Binary_Image_Info *imageFound = NULL; - - for (uint32_t iImg = 0; iImg < bsg_dyld_image_count(); iImg++) { - BSG_Mach_Binary_Image_Info *img = bsg_dyld_get_image_info(iImg); - if (img->header != NULL) { - // Look for a segment command with this address within its range. - uintptr_t addressWSlide = address - img->slide; - uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(img->header); - if (cmdPtr == 0) { - continue; - } - for (uint32_t iCmd = 0; iCmd < img->header->ncmds; iCmd++) { - const struct load_command *loadCmd = - (struct load_command *)cmdPtr; - if (loadCmd->cmd == LC_SEGMENT) { - const struct segment_command *segCmd = - (struct segment_command *)cmdPtr; - if (addressWSlide >= segCmd->vmaddr && - addressWSlide < segCmd->vmaddr + segCmd->vmsize) { - imageFound = img; - break; - } - } else if (loadCmd->cmd == LC_SEGMENT_64) { - const struct segment_command_64 *segCmd = - (struct segment_command_64 *)cmdPtr; - if (addressWSlide >= segCmd->vmaddr && - addressWSlide < segCmd->vmaddr + segCmd->vmsize) { - imageFound = img; - break; - } +BSG_Mach_Header_Info *bsg_mach_headers_image_at_address(const uintptr_t address) { + + for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { + if (img->removed == true) { + continue; + } + // Look for a segment command with this address within its range. + uintptr_t cmdPtr = bsg_mach_headers_first_cmd_after_header(img->header); + if (cmdPtr == 0) { + continue; + } + uintptr_t addressWSlide = address - img->slide; + for (uint32_t iCmd = 0; iCmd < img->header->ncmds; iCmd++) { + const struct load_command *loadCmd = + (struct load_command *)cmdPtr; + if (loadCmd->cmd == LC_SEGMENT) { + const struct segment_command *segCmd = + (struct segment_command *)cmdPtr; + if (addressWSlide >= segCmd->vmaddr && + addressWSlide < segCmd->vmaddr + segCmd->vmsize) { + return img; + } + } else if (loadCmd->cmd == LC_SEGMENT_64) { + const struct segment_command_64 *segCmd = + (struct segment_command_64 *)cmdPtr; + if (addressWSlide >= segCmd->vmaddr && + addressWSlide < segCmd->vmaddr + segCmd->vmsize) { + return img; } - cmdPtr += loadCmd->cmdsize; } + cmdPtr += loadCmd->cmdsize; } } - return imageFound; + return NULL; } -uintptr_t bsg_mach_image_first_cmd_after_header(const struct mach_header *const header) { +uintptr_t bsg_mach_headers_first_cmd_after_header(const struct mach_header *const header) { if (header == NULL) { return 0; } @@ -374,9 +306,9 @@ uintptr_t bsg_mach_image_first_cmd_after_header(const struct mach_header *const } } -uintptr_t bsg_mach_image_base_of_image_index(const struct mach_header *const header) { +uintptr_t bsg_mach_headers_image_at_base_of_image_index(const struct mach_header *const header) { // Look for a segment command and return the file image address. - uintptr_t cmdPtr = bsg_mach_image_first_cmd_after_header(header); + uintptr_t cmdPtr = bsg_mach_headers_first_cmd_after_header(header); if (cmdPtr == 0) { return 0; } diff --git a/Tests/KSCrash/KSDynamicLinker_Tests.m b/Tests/KSCrash/KSDynamicLinker_Tests.m index 320c62a3a..6cdbef07f 100755 --- a/Tests/KSCrash/KSDynamicLinker_Tests.m +++ b/Tests/KSCrash/KSDynamicLinker_Tests.m @@ -28,13 +28,18 @@ #import #import "BSG_KSDynamicLinker.h" +#import "BSG_KSMachHeaders.h" @interface KSDynamicLinker_Tests : XCTestCase @end @implementation KSDynamicLinker_Tests +// TODO:TL Fix test - (void) testImageUUID { + bsg_mach_headers_initialize(); + _dyld_register_func_for_add_image(&bsg_mach_headers_add_image); + // Just abritrarily grab the name of the 4th image... const char* name = _dyld_get_image_name(4); const uint8_t* uuidBytes = bsg_ksdlimageUUID(name, true); @@ -43,18 +48,27 @@ - (void) testImageUUID - (void) testImageUUIDInvalidName { + bsg_mach_headers_initialize(); + _dyld_register_func_for_add_image(&bsg_mach_headers_add_image); + const uint8_t* uuidBytes = bsg_ksdlimageUUID("sdfgserghwerghwrh", true); XCTAssertTrue(uuidBytes == NULL, @""); } - (void) testImageUUIDNULLName { + bsg_mach_headers_initialize(); + _dyld_register_func_for_add_image(&bsg_mach_headers_add_image); + const uint8_t* uuidBytes = bsg_ksdlimageUUID(NULL, true); XCTAssertTrue(uuidBytes == NULL, @""); } - (void) testImageUUIDPartialMatch { + bsg_mach_headers_initialize(); + _dyld_register_func_for_add_image(&bsg_mach_headers_add_image); + const uint8_t* uuidBytes = bsg_ksdlimageUUID("libSystem", false); XCTAssertTrue(uuidBytes != NULL, @""); } diff --git a/Tests/KSCrash/KSMachHeader_Tests.m b/Tests/KSCrash/KSMachHeader_Tests.m index 9d0b512ef..a1d6207a5 100644 --- a/Tests/KSCrash/KSMachHeader_Tests.m +++ b/Tests/KSCrash/KSMachHeader_Tests.m @@ -10,143 +10,92 @@ #import "BSG_KSMachHeaders.h" #import -// Private methods -void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element); -void bsg_remove_mach_binary_image(uint64_t imageVmAddr); +void bsg_mach_headers_add_image(const struct mach_header *mh, intptr_t slide); -const struct mach_header mh = { - .magic = 1, - .cputype = 42, - .cpusubtype = 27, - .filetype = 1, + +const struct mach_header header1 = { + .magic = MH_MAGIC, + .cputype = 0, + .cpusubtype = 0, + .filetype = 0, .ncmds = 1, - .sizeofcmds = 1, - .flags = 1 + .sizeofcmds = 0, + .flags = 0 +}; +const struct segment_command command1 = { + LC_SEGMENT,0,SEG_TEXT,111,10,0,0,0,0,0,0 }; -const BSG_Mach_Binary_Image_Info info1 = {.header = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the first", .slide = 123 }; -const BSG_Mach_Binary_Image_Info info2 = {.header = &mh, .imageVmAddr = 23456, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the second", .slide = 1234 }; -const BSG_Mach_Binary_Image_Info info3 = {.header = &mh, .imageVmAddr = 34567, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the third", .slide = 12345 }; -const BSG_Mach_Binary_Image_Info info4 = {.header = &mh, .imageVmAddr = 45678, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the fourth", .slide = 123456 }; +const struct mach_header header2 = { + .magic = MH_MAGIC, + .cputype = 0, + .cpusubtype = 0, + .filetype = 0, + .ncmds = 1, + .sizeofcmds = 0, + .flags = 0 +}; +const struct segment_command command2 = { + LC_SEGMENT,0,SEG_TEXT,222,10,0,0,0,0,0,0 +}; @interface KSMachHeader_Tests : XCTestCase @end @implementation KSMachHeader_Tests -- (void)testDynamicArray { - - bsg_initialise_mach_binary_headers(2); - XCTAssertEqual(bsg_dyld_image_count(), 0); +- (void)testAddRemoveHeaders { - // Add - bsg_add_mach_binary_image(info1); - XCTAssertEqual(bsg_dyld_image_count(), 1); + bsg_mach_headers_initialize(); - bsg_add_mach_binary_image(info2); - XCTAssertEqual(bsg_dyld_image_count(), 2); - - // Expand - double size - bsg_add_mach_binary_image(info3); - XCTAssertEqual(bsg_dyld_image_count(), 3); + // Get to the end of the list of system images to check additions + BSG_Mach_Header_Info *listTail; + for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { + listTail = img; + } - // Delete - third will be copied - bsg_remove_mach_binary_image(12345); - XCTAssertEqual(bsg_dyld_image_count(), 2); - - XCTAssertEqual(strcmp(bsg_dyld_get_image_info(0)->name, "header the third"), 0); - XCTAssertEqual(strcmp(bsg_dyld_get_image_info(1)->name, "header the second"), 0); + bsg_mach_headers_add_image(&header1, 0); - // Nothing happens - bsg_remove_mach_binary_image(12345); - XCTAssertEqual(bsg_dyld_image_count(), 2); - - bsg_remove_mach_binary_image(23456); - XCTAssertEqual(bsg_dyld_image_count(), 1); - XCTAssertEqual(strcmp(bsg_dyld_get_image_info(0)->name, "header the third"), 0); + XCTAssertEqual(listTail->next->imageVmAddr, 111); + XCTAssert(listTail->next->removed == FALSE); - bsg_remove_mach_binary_image(34567); - XCTAssertEqual(bsg_dyld_image_count(), 0); + bsg_mach_headers_add_image(&header2, 0); - bsg_remove_mach_binary_image(34567); - XCTAssertEqual(bsg_dyld_image_count(), 0); + XCTAssertEqual(listTail->next->imageVmAddr, 111); + XCTAssert(listTail->next->removed == FALSE); + XCTAssertEqual(listTail->next->next->imageVmAddr, 222); + XCTAssert(listTail->next->next->removed == FALSE); - // Readd - bsg_add_mach_binary_image(info1); - XCTAssertEqual(bsg_dyld_image_count(), 1); -} - -- (void)testRemoveLast1 { - bsg_initialise_mach_binary_headers(2); - bsg_add_mach_binary_image(info1); - XCTAssertEqual(bsg_dyld_image_count(), 1); - - bsg_remove_mach_binary_image(12345); - XCTAssertEqual(bsg_dyld_image_count(), 0); -} - -- (void)testRemoveLast2 { - bsg_initialise_mach_binary_headers(2); - bsg_add_mach_binary_image(info1); - bsg_add_mach_binary_image(info2); - XCTAssertEqual(bsg_dyld_image_count(), 2); - - bsg_remove_mach_binary_image(23456); - XCTAssertEqual(bsg_dyld_image_count(), 1); - - bsg_remove_mach_binary_image(12345); - XCTAssertEqual(bsg_dyld_image_count(), 0); -} - -- (void)testRemoveLast3 { - bsg_initialise_mach_binary_headers(2); + bsg_mach_headers_remove_image(&header1, 0); + XCTAssertEqual(listTail->next->imageVmAddr, 111); + XCTAssert(listTail->next->removed == TRUE); + XCTAssertEqual(listTail->next->next->imageVmAddr, 222); + XCTAssert(listTail->next->next->removed == FALSE); + + bsg_mach_headers_remove_image(&header2, 0); + XCTAssertEqual(listTail->next->imageVmAddr, 111); + XCTAssert(listTail->next->removed == TRUE); + XCTAssertEqual(listTail->next->next->imageVmAddr, 222); + XCTAssert(listTail->next->next->removed == TRUE); - bsg_add_mach_binary_image(info1); - bsg_add_mach_binary_image(info2); - bsg_add_mach_binary_image(info3); - bsg_add_mach_binary_image(info4); - XCTAssertEqual(bsg_dyld_image_count(), 4); - - bsg_remove_mach_binary_image(45678); - XCTAssertEqual(bsg_dyld_image_count(), 3); - - bsg_remove_mach_binary_image(12345); - XCTAssertEqual(bsg_dyld_image_count(), 2); - - bsg_remove_mach_binary_image(12345); - XCTAssertEqual(bsg_dyld_image_count(), 2); - - bsg_remove_mach_binary_image(34567); - XCTAssertEqual(bsg_dyld_image_count(), 1); } -// Test out-of-bounds behaviour of the replicated dyld API -- (void)testBSGDYLDAPI { - bsg_initialise_mach_binary_headers(2); - - bsg_add_mach_binary_image(info1); - bsg_add_mach_binary_image(info2); - XCTAssertEqual(bsg_dyld_image_count(), 2); +- (void)testFindImageAtAddress { + bsg_mach_headers_initialize(); + bsg_mach_headers_add_image(&header1, 0); + bsg_mach_headers_add_image(&header2, 0); - XCTAssertTrue(bsg_dyld_get_image_info(2) == NULL); - XCTAssertTrue(bsg_dyld_get_image_info(999) == NULL); - - XCTAssertEqual(bsg_dyld_get_image_info(0)->slide, 123); - XCTAssertEqual(bsg_dyld_get_image_info(1)->slide, 1234); - - XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(0)->name], @"header the first"); - XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(1)->name], @"header the second"); - - XCTAssertEqual(bsg_dyld_get_image_info(0)->header->filetype, 1); - XCTAssertEqual(bsg_dyld_get_image_info(1)->header->filetype, 1); + BSG_Mach_Header_Info *item; + item = bsg_mach_headers_image_at_address(111); + XCTAssertEqual(item->imageVmAddr, 111); - XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(0)->name], @"header the first"); - XCTAssertEqualObjects([NSString stringWithUTF8String:bsg_dyld_get_image_info(1)->name], @"header the second"); + item = bsg_mach_headers_image_at_address(222); + XCTAssertEqual(item->imageVmAddr, 222); } - (void) testGetImageNameNULL { - BSG_Mach_Binary_Image_Info *img = bsg_mach_image_named(NULL, false); + BSG_Mach_Header_Info *img = bsg_mach_headers_image_named(NULL, false); XCTAssertTrue(img == NULL); } From a5dff3b03bdc349c9c7ab191aa6bf4d9a2ad3406 Mon Sep 17 00:00:00 2001 From: Tom Longridge Date: Mon, 29 Jun 2020 14:23:54 +0100 Subject: [PATCH 3/5] Changes following review --- .../Source/KSCrash/Recording/BSG_KSCrashC.c | 2 + .../Recording/Tools/BSG_KSDynamicLinker.c | 16 +++--- .../Recording/Tools/BSG_KSMachHeaders.h | 5 ++ .../Recording/Tools/BSG_KSMachHeaders.m | 55 ++++++++----------- Tests/KSCrash/KSDynamicLinker_Tests.m | 1 - 5 files changed, 36 insertions(+), 43 deletions(-) diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c index c7183b40f..07548fd1a 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c @@ -99,6 +99,8 @@ BSG_KSCrashType bsg_kscrash_install(const char *const crashReportFilePath, BSG_KSCrash_Context *context = crashContext(); + // Initialize local store of dynamically loaded libraries so that binary + // image information can be extracted for reports bsg_mach_headers_initialize(); if (bsg_g_installed) { diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c index 5423520d6..e3bae0284 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSDynamicLinker.c @@ -37,20 +37,18 @@ const uint8_t *bsg_ksdlimageUUID(const char *const imageName, bool exactMatch) { BSG_Mach_Header_Info *img = bsg_mach_headers_image_named(imageName, exactMatch); if (img != NULL) { uintptr_t cmdPtr = bsg_mach_headers_first_cmd_after_header(img->header); - if (cmdPtr != 0) { + if (cmdPtr != 0) { for (uint32_t iCmd = 0; iCmd < img->header->ncmds; iCmd++) { - const struct load_command *loadCmd = - (struct load_command *)cmdPtr; - if (loadCmd->cmd == LC_UUID) { - struct uuid_command *uuidCmd = - (struct uuid_command *)cmdPtr; - return uuidCmd->uuid; - } - cmdPtr += loadCmd->cmdsize; + const struct load_command *loadCmd = (struct load_command *)cmdPtr; + if (loadCmd->cmd == LC_UUID) { + struct uuid_command *uuidCmd = (struct uuid_command *)cmdPtr; + return uuidCmd->uuid; } + cmdPtr += loadCmd->cmdsize; } } } + } return NULL; } diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h index 873662798..80de0da5e 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h @@ -13,6 +13,11 @@ #import #import +/* Maintaining our own list of framework Mach headers means that we avoid potential + * deadlock situations where we try and suspend lock-holding threads prior to + * loading mach headers as part of our normal event handling behaviour. + */ + /** * An encapsulation of the Mach header data as a linked list - either 64 or 32 bit, along with some additiona * information required for detailing a crash report's binary images. diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m index 7ee3963ce..497088bac 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m @@ -15,6 +15,17 @@ // MARK: - Locking +static const NSOperatingSystemVersion minSdkForUnfairLock = + #if BSG_PLATFORM_IOS + {10,0,0}; + #elif BSG_PLATFORM_OSX + {10,12,0}; + #elif BSG_PLATFORM_TVOS + {10,0,0}; + #elif BSG_PLATFORM_WATCHOS + {3,0,0} + #endif + // Pragma's hide unavoidable (and expected) deprecation/unavailable warnings _Pragma("clang diagnostic push") _Pragma("clang diagnostic ignored \"-Wunguarded-availability\"") @@ -56,44 +67,20 @@ void bsg_unfair_unlock() { _Pragma("clang diagnostic pop") } -BOOL bsg_is_unfair_lock_supported() { - NSOperatingSystemVersion minSdk = {0,0,0}; -#if BSG_PLATFORM_IOS - minSdk.majorVersion = 10; -#elif BSG_PLATFORM_OSX - minSdk.majorVersion = 10; - minSdk.minorVersion = 12; -#elif BSG_PLATFORM_TVOS - minSdk.majorVersion = 10; -#elif BSG_PLATFORM_WATCHOS - minSdk.majorVersion = 3; -#endif - return [[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:minSdk]; -} - -static void (*bsg_mach_headers_cache_lock_func)(void) = NULL; -static void (*bsg_mach_headers_cache_unlock_func)(void) = NULL; - void bsg_mach_headers_cache_lock() { - if (bsg_mach_headers_cache_lock_func == NULL) { - if (bsg_is_unfair_lock_supported()) { - bsg_mach_headers_cache_lock_func = &bsg_unfair_lock; - } else { - bsg_mach_headers_cache_lock_func = &bsg_spin_lock; - } + if ([[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:minSdkForUnfairLock]) { + bsg_unfair_lock(); + } else { + bsg_spin_lock(); } - bsg_mach_headers_cache_lock_func(); } void bsg_mach_headers_cache_unlock() { - if (bsg_mach_headers_cache_unlock_func == NULL) { - if (bsg_is_unfair_lock_supported()) { - bsg_mach_headers_cache_unlock_func = &bsg_unfair_unlock; - } else { - bsg_mach_headers_cache_unlock_func = &bsg_spin_unlock; - } + if ([[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:minSdkForUnfairLock]) { + bsg_unfair_unlock(); + } else { + bsg_spin_unlock(); } - bsg_mach_headers_cache_unlock_func(); } // MARK: - Mach Header Linked List @@ -117,7 +104,9 @@ void bsg_mach_headers_initialize() { bsg_mach_headers_images_head = NULL; bsg_mach_headers_images_tail = NULL; - // Register for binary images being loaded and unloaded + // Register for binary images being loaded and unloaded. dyld calls the add function once + // for each library that has already been loaded and then keeps this cache up-to-date + // with future changes _dyld_register_func_for_add_image(&bsg_mach_headers_add_image); _dyld_register_func_for_remove_image(&bsg_mach_headers_remove_image); } diff --git a/Tests/KSCrash/KSDynamicLinker_Tests.m b/Tests/KSCrash/KSDynamicLinker_Tests.m index 6cdbef07f..c68bbc877 100755 --- a/Tests/KSCrash/KSDynamicLinker_Tests.m +++ b/Tests/KSCrash/KSDynamicLinker_Tests.m @@ -34,7 +34,6 @@ @interface KSDynamicLinker_Tests : XCTestCase @end @implementation KSDynamicLinker_Tests -// TODO:TL Fix test - (void) testImageUUID { bsg_mach_headers_initialize(); From c66bd18655224e6dcfb2da15464be166bea66b53 Mon Sep 17 00:00:00 2001 From: Tom Longridge Date: Mon, 29 Jun 2020 15:42:21 +0100 Subject: [PATCH 4/5] Release v6.0.1 Also corrected Makefile prerelease target path --- Bugsnag.podspec.json | 4 ++-- Bugsnag/Payload/BugsnagNotifier.m | 2 +- CHANGELOG.md | 2 +- Makefile | 2 +- VERSION | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Bugsnag.podspec.json b/Bugsnag.podspec.json index 3736e5639..1a195a39b 100644 --- a/Bugsnag.podspec.json +++ b/Bugsnag.podspec.json @@ -1,6 +1,6 @@ { "name": "Bugsnag", - "version": "6.0.0", + "version": "6.0.1", "summary": "The Bugsnag crash reporting framework for Apple platforms.", "homepage": "https://bugsnag.com", "license": "MIT", @@ -9,7 +9,7 @@ }, "source": { "git": "https://github.com/bugsnag/bugsnag-cocoa.git", - "tag": "v6.0.0" + "tag": "v6.0.1" }, "frameworks": [ "Foundation", diff --git a/Bugsnag/Payload/BugsnagNotifier.m b/Bugsnag/Payload/BugsnagNotifier.m index 4a3f3fe2b..0fe5477be 100644 --- a/Bugsnag/Payload/BugsnagNotifier.m +++ b/Bugsnag/Payload/BugsnagNotifier.m @@ -23,7 +23,7 @@ - (instancetype)init { #else self.name = @"Bugsnag Objective-C"; #endif - self.version = @"6.0.0"; + self.version = @"6.0.1"; self.url = @"https://github.com/bugsnag/bugsnag-cocoa"; self.dependencies = [NSMutableArray new]; } diff --git a/CHANGELOG.md b/CHANGELOG.md index 562e178e4..3f3f51bcd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ Changelog ========= -## TBD +## 6.0.1 (2020-06-29) * Move binary images store declaration from header file [#725](https://github.com/bugsnag/bugsnag-cocoa/pull/725) diff --git a/Makefile b/Makefile index 49f48d3d2..41943fda8 100644 --- a/Makefile +++ b/Makefile @@ -144,7 +144,7 @@ ifeq ($(VERSION),) @$(error VERSION is not defined. Run with `make VERSION=number prerelease`) endif @git checkout -b release-v$(VERSION) - @git add Source/BugsnagNotifier.m Bugsnag.podspec.json VERSION CHANGELOG.md + @git add Bugsnag/Payload/BugsnagNotifier.m Bugsnag.podspec.json VERSION CHANGELOG.md @git commit -m "Release v$(VERSION)" @git push origin release-v$(VERSION) @hub pull-request -m "Release v$(VERSION)" --browse diff --git a/VERSION b/VERSION index 09b254e90..5fe607230 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -6.0.0 +6.0.1 From 6ae9b00835b435b9f43af0d29dea822c97daf958 Mon Sep 17 00:00:00 2001 From: Tom Longridge Date: Mon, 29 Jun 2020 16:11:17 +0100 Subject: [PATCH 5/5] Addressed review comments --- .../KSCrash/Recording/BSG_KSCrashReport.c | 2 +- .../Recording/Tools/BSG_KSMachHeaders.h | 2 +- .../Recording/Tools/BSG_KSMachHeaders.m | 40 ++++++++++--------- Tests/KSCrash/KSMachHeader_Tests.m | 14 +++---- 4 files changed, 31 insertions(+), 27 deletions(-) diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c index 7c8d07c49..ccae26368 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c @@ -1135,7 +1135,7 @@ void bsg_kscrw_i_writeBinaryImages(const BSG_KSCrashReportWriter *const writer, writer->beginArray(writer, key); { for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { - if (!img->removed) { + if (!img->unloaded) { bsg_kscrw_i_writeBinaryImage(writer, NULL, img); } } diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h index 80de0da5e..94def6cfd 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h @@ -31,7 +31,7 @@ typedef struct bsg_mach_image { uint8_t *uuid; const char* name; intptr_t slide; - bool removed; + bool unloaded; struct bsg_mach_image *next; } BSG_Mach_Header_Info; diff --git a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m index 497088bac..7c091c07e 100644 --- a/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m +++ b/Bugsnag/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m @@ -85,24 +85,24 @@ void bsg_mach_headers_cache_unlock() { // MARK: - Mach Header Linked List -static BSG_Mach_Header_Info *bsg_mach_headers_images_head; -static BSG_Mach_Header_Info *bsg_mach_headers_images_tail; +static BSG_Mach_Header_Info *bsg_g_mach_headers_images_head; +static BSG_Mach_Header_Info *bsg_g_mach_headers_images_tail; BSG_Mach_Header_Info *bsg_mach_headers_get_images() { - return bsg_mach_headers_images_head; + return bsg_g_mach_headers_images_head; } void bsg_mach_headers_initialize() { - // Free any existing data (this may be present in tests) - for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; ) { + // Clear any existing headers to reset the head/tail pointers + for (BSG_Mach_Header_Info *img = bsg_g_mach_headers_images_head; img != NULL; ) { BSG_Mach_Header_Info *imgToDelete = img; img = img->next; free(imgToDelete); } - bsg_mach_headers_images_head = NULL; - bsg_mach_headers_images_tail = NULL; + bsg_g_mach_headers_images_head = NULL; + bsg_g_mach_headers_images_tail = NULL; // Register for binary images being loaded and unloaded. dyld calls the add function once // for each library that has already been loaded and then keeps this cache up-to-date @@ -179,7 +179,7 @@ bool bsg_mach_headers_populate_info(const struct mach_header *header, intptr_t s info->uuid = uuid; info->name = imageName; info->slide = slide; - info->removed = FALSE; + info->unloaded = FALSE; info->next = NULL; return true; @@ -193,12 +193,12 @@ void bsg_mach_headers_add_image(const struct mach_header *header, intptr_t slide bsg_mach_headers_cache_lock(); - if (bsg_mach_headers_images_head == NULL) { - bsg_mach_headers_images_head = newImage; + if (bsg_g_mach_headers_images_head == NULL) { + bsg_g_mach_headers_images_head = newImage; } else { - bsg_mach_headers_images_tail->next = newImage; + bsg_g_mach_headers_images_tail->next = newImage; } - bsg_mach_headers_images_tail = newImage; + bsg_g_mach_headers_images_tail = newImage; bsg_mach_headers_cache_unlock(); } @@ -206,12 +206,16 @@ void bsg_mach_headers_add_image(const struct mach_header *header, intptr_t slide } +/** + * To avoid a destructive operation that could lead thread safety problems, we maintain the + * image record, but mark it as unloaded + */ void bsg_mach_headers_remove_image(const struct mach_header *header, intptr_t slide) { BSG_Mach_Header_Info existingImage = { 0 }; if (bsg_mach_headers_populate_info(header, slide, &existingImage)) { - for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { + for (BSG_Mach_Header_Info *img = bsg_g_mach_headers_images_head; img != NULL; img = img->next) { if (img->imageVmAddr == existingImage.imageVmAddr) { - img->removed = true; + img->unloaded = true; } } } @@ -221,10 +225,10 @@ void bsg_mach_headers_remove_image(const struct mach_header *header, intptr_t sl if (imageName != NULL) { - for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { + for (BSG_Mach_Header_Info *img = bsg_g_mach_headers_images_head; img != NULL; img = img->next) { if (img->name == NULL) { continue; // name is null if the index is out of range per dyld(3) - } else if (img->removed == true) { + } else if (img->unloaded == true) { continue; // ignore unloaded libraries } else if (exactMatch) { if (strcmp(img->name, imageName) == 0) { @@ -243,8 +247,8 @@ void bsg_mach_headers_remove_image(const struct mach_header *header, intptr_t sl BSG_Mach_Header_Info *bsg_mach_headers_image_at_address(const uintptr_t address) { - for (BSG_Mach_Header_Info *img = bsg_mach_headers_get_images(); img != NULL; img = img->next) { - if (img->removed == true) { + for (BSG_Mach_Header_Info *img = bsg_g_mach_headers_images_head; img != NULL; img = img->next) { + if (img->unloaded == true) { continue; } // Look for a segment command with this address within its range. diff --git a/Tests/KSCrash/KSMachHeader_Tests.m b/Tests/KSCrash/KSMachHeader_Tests.m index a1d6207a5..f09c3f9c1 100644 --- a/Tests/KSCrash/KSMachHeader_Tests.m +++ b/Tests/KSCrash/KSMachHeader_Tests.m @@ -57,26 +57,26 @@ - (void)testAddRemoveHeaders { bsg_mach_headers_add_image(&header1, 0); XCTAssertEqual(listTail->next->imageVmAddr, 111); - XCTAssert(listTail->next->removed == FALSE); + XCTAssert(listTail->next->unloaded == FALSE); bsg_mach_headers_add_image(&header2, 0); XCTAssertEqual(listTail->next->imageVmAddr, 111); - XCTAssert(listTail->next->removed == FALSE); + XCTAssert(listTail->next->unloaded == FALSE); XCTAssertEqual(listTail->next->next->imageVmAddr, 222); - XCTAssert(listTail->next->next->removed == FALSE); + XCTAssert(listTail->next->next->unloaded == FALSE); bsg_mach_headers_remove_image(&header1, 0); XCTAssertEqual(listTail->next->imageVmAddr, 111); - XCTAssert(listTail->next->removed == TRUE); + XCTAssert(listTail->next->unloaded == TRUE); XCTAssertEqual(listTail->next->next->imageVmAddr, 222); - XCTAssert(listTail->next->next->removed == FALSE); + XCTAssert(listTail->next->next->unloaded == FALSE); bsg_mach_headers_remove_image(&header2, 0); XCTAssertEqual(listTail->next->imageVmAddr, 111); - XCTAssert(listTail->next->removed == TRUE); + XCTAssert(listTail->next->unloaded == TRUE); XCTAssertEqual(listTail->next->next->imageVmAddr, 222); - XCTAssert(listTail->next->next->removed == TRUE); + XCTAssert(listTail->next->next->unloaded == TRUE); }