From bc21cc7234ad35ec08fb09fa0c3382d2b423b228 Mon Sep 17 00:00:00 2001 From: Robin Macharg Date: Tue, 5 May 2020 12:03:38 +0100 Subject: [PATCH] DYLD deadlock: Review feedback --- .../Recording/Tools/BSG_KSMachHeaders.h | 10 +- .../Recording/Tools/BSG_KSMachHeaders.m | 66 ++++--- Tests/KSCrash/KSMachHeader_Tests.m | 169 +++++++++++------- 3 files changed, 142 insertions(+), 103 deletions(-) diff --git a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h index d142e4ab9..91620c26d 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h +++ b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h @@ -38,12 +38,14 @@ typedef struct { static BSG_Mach_Binary_Images bsg_mach_binary_images; +/** + * A lock, used to synchronise access to the array of binary image info. + */ static os_unfair_lock bsg_mach_binary_images_access_lock = OS_UNFAIR_LOCK_INIT; -void bsg_initialize_binary_images_array(BSG_Mach_Binary_Images *array, size_t initialSize); -void bsg_add_mach_binary_image(BSG_Mach_Binary_Images *array, BSG_Mach_Binary_Image_Info element); -void bsg_remove_mach_binary_image(BSG_Mach_Binary_Images *array, const char *element_name); -void bsg_free_binary_images_array(BSG_Mach_Binary_Images *array); +/** + * Provide external access to the array of binary image info + */ BSG_Mach_Binary_Images *bsg_get_mach_binary_images(void); /** diff --git a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m index 591bdffda..0be5d40fc 100644 --- a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m +++ b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m @@ -9,12 +9,16 @@ #import #import #import +#import "BSG_KSDynamicLinker.h" #import "BSG_KSMachHeaders.h" -void bsg_initialize_binary_images_array(BSG_Mach_Binary_Images *array, size_t initialSize) { - array->contents = (BSG_Mach_Binary_Image_Info *)malloc(initialSize * sizeof(BSG_Mach_Binary_Image_Info)); - array->used = 0; - array->size = initialSize; +// We expect <1000 items +static const BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE = 100; + +void bsg_initialize_binary_images_array(size_t initialSize) { + 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_Binary_Images *bsg_get_mach_binary_images() { @@ -24,20 +28,20 @@ void bsg_initialize_binary_images_array(BSG_Mach_Binary_Images *array, size_t in /** * 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. + * 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_Images *array, BSG_Mach_Binary_Image_Info element) { +void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element) { os_unfair_lock_lock(&bsg_mach_binary_images_access_lock); // Expand array if necessary - if (array->used == array->size) { - array->size *= 2; - array->contents = (BSG_Mach_Binary_Image_Info *)realloc(array->contents, array->size * sizeof(BSG_Mach_Binary_Image_Info)); + if (bsg_mach_binary_images.used == bsg_mach_binary_images.size) { + bsg_mach_binary_images.size *= 2; + bsg_mach_binary_images.contents = (BSG_Mach_Binary_Image_Info *)realloc(bsg_mach_binary_images.contents, bsg_mach_binary_images.size * sizeof(BSG_Mach_Binary_Image_Info)); } // Store the value, increment the number of used elements - array->contents[array->used++] = element; + bsg_mach_binary_images.contents[bsg_mach_binary_images.used++] = element; os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock); } @@ -47,38 +51,32 @@ void bsg_add_mach_binary_image(BSG_Mach_Binary_Images *array, BSG_Mach_Binary_Im * 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(BSG_Mach_Binary_Images *array, const char *element_name) { +void bsg_remove_mach_binary_image(const char *element_name) { os_unfair_lock_lock(&bsg_mach_binary_images_access_lock); - for (size_t i=0; iused; i++) { - BSG_Mach_Binary_Image_Info item = array->contents[i]; + for (size_t i=0; iused >= 2) { - array->contents[i] = array->contents[--array->used]; + // Note: removal of the last (ith) item involves a redundant copy from last->last. + if (bsg_mach_binary_images.used >= 2) { + bsg_mach_binary_images.contents[i] = bsg_mach_binary_images.contents[--bsg_mach_binary_images.used]; } else { - array->used = 0; + bsg_mach_binary_images.used = 0; } - break; // an image can only be loaded singularly + break; // an image can only be loaded singularly; exit loop once found } } os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock); } -void bsg_free_binary_images_array(BSG_Mach_Binary_Images *array) { - free(array->contents); - array->contents = NULL; - array->used = array->size = 0; -} - void bsg_initialise_mach_binary_headers() { - bsg_initialize_binary_images_array(&bsg_mach_binary_images, 100); + bsg_initialize_binary_images_array(BSG_INITIAL_MACH_BINARY_IMAGE_ARRAY_SIZE); } -uintptr_t bsg_ksdlfirstCmdAfterHeader(const struct mach_header *const header); - /** * Populate a Mach binary image info structure * @@ -88,7 +86,7 @@ void bsg_initialise_mach_binary_headers() { * * @returns a boolean indicating success */ -bool populate_info(const struct mach_header *header, BSG_Mach_Binary_Image_Info *info) { +bool bsg_populate_mach_image_info(const struct mach_header *header, BSG_Mach_Binary_Image_Info *info) { // Early exit conditions; this is not a valid/useful binary image // 1. We can't find a sensible Mach command @@ -100,8 +98,8 @@ bool populate_info(const struct mach_header *header, BSG_Mach_Binary_Image_Info // 2. The image doesn't have a name. Note: running with a debugger attached causes this condition to match. Dl_info DlInfo = (const Dl_info) { 0 }; dladdr(header, &DlInfo); - const char *image_name = DlInfo.dli_fname; - if (!image_name) { + const char *imageName = DlInfo.dli_fname; + if (!imageName) { return false; } @@ -147,7 +145,7 @@ bool populate_info(const struct mach_header *header, BSG_Mach_Binary_Image_Info info->imageSize = imageSize; info->imageVmAddr = imageVmAddr; info->uuid = uuid; - info->name = image_name; + info->name = imageName; return true; } @@ -164,8 +162,8 @@ bool populate_info(const struct mach_header *header, BSG_Mach_Binary_Image_Info void bsg_mach_binary_image_added(const struct mach_header *header, intptr_t slide) { BSG_Mach_Binary_Image_Info info = { 0 }; - if (populate_info(header, &info)) { - bsg_add_mach_binary_image(&bsg_mach_binary_images, info); + if (bsg_populate_mach_image_info(header, &info)) { + bsg_add_mach_binary_image(info); } } @@ -176,7 +174,7 @@ void bsg_mach_binary_image_removed(const struct mach_header *header, intptr_t sl { // Convert header and slide into an info struct BSG_Mach_Binary_Image_Info info; - if (populate_info(header, &info)) { - bsg_remove_mach_binary_image(&bsg_mach_binary_images, info.name); + if (bsg_populate_mach_image_info(header, &info)) { + bsg_remove_mach_binary_image(info.name); } } diff --git a/Tests/KSCrash/KSMachHeader_Tests.m b/Tests/KSCrash/KSMachHeader_Tests.m index 08a31a24d..7b3ecba02 100644 --- a/Tests/KSCrash/KSMachHeader_Tests.m +++ b/Tests/KSCrash/KSMachHeader_Tests.m @@ -10,6 +10,12 @@ #import "BSG_KSMachHeaders.h" #import +// Private methods +void bsg_initialize_binary_images_array(size_t initialSize); +void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element); +void bsg_remove_mach_binary_image(const char *element_name); +BSG_Mach_Binary_Images *bsg_get_mach_binary_images(void); + const struct mach_header mh = { .magic = 1, .cputype = 1, @@ -20,35 +26,10 @@ .flags = 1 }; -const BSG_Mach_Binary_Image_Info info1 = { - .mh = &mh, - .imageVmAddr = 12345, - .imageSize = 6789, - .uuid = (uint8_t *)123, - .name = "header the first", - .cputype = 42, - .cpusubtype = 27 -}; - -const BSG_Mach_Binary_Image_Info info2 = { - .mh = &mh, - .imageVmAddr = 12345, - .imageSize = 6789, - .uuid = (uint8_t *)123, - .name = "header the second", - .cputype = 42, - .cpusubtype = 27 -}; - -const BSG_Mach_Binary_Image_Info info3 = { - .mh = &mh, - .imageVmAddr = 12345, - .imageSize = 6789, - .uuid = (uint8_t *)123, - .name = "header the third", - .cputype = 42, - .cpusubtype = 27 -}; +const BSG_Mach_Binary_Image_Info info1 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the first", .cputype = 42, .cpusubtype = 27 }; +const BSG_Mach_Binary_Image_Info info2 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the second", .cputype = 42, .cpusubtype = 27 }; +const BSG_Mach_Binary_Image_Info info3 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the third", .cputype = 42, .cpusubtype = 27 }; +const BSG_Mach_Binary_Image_Info info4 = {.mh = &mh, .imageVmAddr = 12345, .imageSize = 6789, .uuid = (uint8_t *)123, .name = "header the fourth", .cputype = 42, .cpusubtype = 27 }; @interface KSMachHeader_Tests : XCTestCase @end @@ -56,56 +37,114 @@ @interface KSMachHeader_Tests : XCTestCase @implementation KSMachHeader_Tests - (void)testDynamicArray { - BSG_Mach_Binary_Images headers; - bsg_initialize_binary_images_array(&headers, 2); - XCTAssertEqual(headers.size, 2); - XCTAssertEqual(headers.used, 0); + + bsg_initialize_binary_images_array(2); + BSG_Mach_Binary_Images *headers = bsg_get_mach_binary_images(); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 0); // Add - bsg_add_mach_binary_image(&headers, info1); - XCTAssertEqual(headers.size, 2); - XCTAssertEqual(headers.used, 1); + bsg_add_mach_binary_image(info1); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 1); - bsg_add_mach_binary_image(&headers, info2); - XCTAssertEqual(headers.size, 2); - XCTAssertEqual(headers.used, 2); + bsg_add_mach_binary_image(info2); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 2); // Expand - double size - bsg_add_mach_binary_image(&headers, info3); - XCTAssertEqual(headers.size, 4); - XCTAssertEqual(headers.used, 3); + bsg_add_mach_binary_image(info3); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 3); // Delete - third will be copied - bsg_remove_mach_binary_image(&headers, "header the first"); - XCTAssertEqual(headers.size, 4); - XCTAssertEqual(headers.used, 2); + bsg_remove_mach_binary_image("header the first"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 2); - XCTAssertEqual(strcmp(headers.contents[0].name, "header the third"), 0); - XCTAssertEqual(strcmp(headers.contents[1].name, "header the second"), 0); - XCTAssertEqual(headers.size, 4); + XCTAssertEqual(strcmp(headers->contents[0].name, "header the third"), 0); + XCTAssertEqual(strcmp(headers->contents[1].name, "header the second"), 0); + XCTAssertEqual(headers->size, 4); // Nothing happens - bsg_remove_mach_binary_image(&headers, "header the first"); - XCTAssertEqual(headers.size, 4); - XCTAssertEqual(headers.used, 2); - - bsg_remove_mach_binary_image(&headers, "header the second"); - XCTAssertEqual(headers.size, 4); - XCTAssertEqual(headers.used, 1); - XCTAssertEqual(strcmp(headers.contents[0].name, "header the third"), 0); + bsg_remove_mach_binary_image("header the first"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 2); + + bsg_remove_mach_binary_image("header the second"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 1); + XCTAssertEqual(strcmp(headers->contents[0].name, "header the third"), 0); - bsg_remove_mach_binary_image(&headers, "header the third"); - XCTAssertEqual(headers.size, 4); - XCTAssertEqual(headers.used, 0); + bsg_remove_mach_binary_image("header the third"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 0); - bsg_remove_mach_binary_image(&headers, "header the third"); - XCTAssertEqual(headers.size, 4); - XCTAssertEqual(headers.used, 0); + bsg_remove_mach_binary_image("header the third"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 0); // Readd - bsg_add_mach_binary_image(&headers, info1); - XCTAssertEqual(headers.size, 4); - XCTAssertEqual(headers.used, 1); + bsg_add_mach_binary_image(info1); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 1); +} + +- (void)testRemoveLast1 { + bsg_initialize_binary_images_array(2); + BSG_Mach_Binary_Images *headers = bsg_get_mach_binary_images(); + bsg_add_mach_binary_image(info1); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 1); + + bsg_remove_mach_binary_image("header the first"); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 0); +} + +- (void)testRemoveLast2 { + bsg_initialize_binary_images_array(2); + BSG_Mach_Binary_Images *headers = bsg_get_mach_binary_images(); + bsg_add_mach_binary_image(info1); + bsg_add_mach_binary_image(info2); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 2); + + bsg_remove_mach_binary_image("header the second"); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 1); + + bsg_remove_mach_binary_image("header the first"); + XCTAssertEqual(headers->size, 2); + XCTAssertEqual(headers->used, 0); +} + +- (void)testRemoveLast3 { + bsg_initialize_binary_images_array(2); + BSG_Mach_Binary_Images *headers = bsg_get_mach_binary_images(); + + 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(headers->size, 4); + XCTAssertEqual(headers->used, 4); + + bsg_remove_mach_binary_image("header the fourth"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 3); + + bsg_remove_mach_binary_image("header the first"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 2); + + bsg_remove_mach_binary_image("header the first"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 2); + + bsg_remove_mach_binary_image("header the third"); + XCTAssertEqual(headers->size, 4); + XCTAssertEqual(headers->used, 1); } @end