From 84663b55cbd81f24445b3879a822476dd385a800 Mon Sep 17 00:00:00 2001
From: Robin Macharg <robin@bugsnag.com>
Date: Wed, 6 May 2020 13:34:46 +0100
Subject: [PATCH] Deadlock: review feedback, specifically: realloc safety and
 locking

---
 .../Recording/Tools/BSG_KSMachHeaders.h       | 11 +++-
 .../Recording/Tools/BSG_KSMachHeaders.m       | 50 ++++++-------------
 2 files changed, 25 insertions(+), 36 deletions(-)

diff --git a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h
index bd35813db..f6c739db0 100644
--- a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h
+++ b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.h
@@ -40,14 +40,23 @@ static BSG_Mach_Binary_Images bsg_mach_binary_images;
 
 /**
  * An OS-version-specific lock, used to synchronise access to the array of binary image info.
+ *
+ * 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
  */
 #if defined(__IPHONE_10_0) || defined(__MAC_10_12) || defined(__TVOS_10_0) || defined(__WATCHOS_3_0)
     #import <os/lock.h>
     static os_unfair_lock bsg_mach_binary_images_access_lock = OS_UNFAIR_LOCK_INIT;
+    #define bsg_lock_mach_binary_image_access os_unfair_lock_lock
+    #define bsg_unlock_mach_binary_image_access os_unfair_lock_unlock
 #else
     #import <libkern/OSAtomic.h>
-    #define OS_SPINLOCK_INIT 0
     static OSSpinLock bsg_mach_binary_images_access_lock = OS_SPINLOCK_INIT;
+    #define bsg_lock_mach_binary_image_access OSSpinLockLock
+    #define bsg_unlock_mach_binary_image_access OSSpinLockUnlock
 #endif
 
 /**
diff --git a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m
index d1c43944b..da55035a6 100644
--- a/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m
+++ b/Source/KSCrash/Source/KSCrash/Recording/Tools/BSG_KSMachHeaders.m
@@ -16,30 +16,6 @@
     return &bsg_mach_binary_images;
 }
 
-// MARK: - Code synchronisation wrappers
-
-// 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
-
-void lock() {
-#if defined(__IPHONE_10_0) || defined(__MAC_10_12) || defined(__TVOS_10_0) || defined(__WATCHOS_3_0)
-    os_unfair_lock_lock(&bsg_mach_binary_images_access_lock);
-#else
-    OSSpinLockLock(&bsg_mach_binary_images_access_lock);
-#endif
-}
-
-void unlock() {
-#if defined(__IPHONE_10_0) || defined(__MAC_10_12) || defined(__TVOS_10_0) || defined(__WATCHOS_3_0)
-    os_unfair_lock_unlock(&bsg_mach_binary_images_access_lock);
-#else
-    OSSpinLockUnlock(&bsg_mach_binary_images_access_lock);
-#endif
-}
-
 /**
  * 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)
@@ -47,27 +23,31 @@ void unlock() {
  */
 void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element) {
     
-    lock();
+    bsg_lock_mach_binary_image_access(&bsg_mach_binary_images_access_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) {
-        bsg_mach_binary_images.size *= 2;
-        size_t newSize = bsg_mach_binary_images.size * sizeof(BSG_Mach_Binary_Image_Info);
-        void **newData = realloc(bsg_mach_binary_images.contents, newSize);
+        size_t newSize = bsg_mach_binary_images.size *= 2;
+        size_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);
         
-        // Exit early if we can't allocate memory
-        if (newData == NULL) {
+        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_unlock_mach_binary_image_access(&bsg_mach_binary_images_access_lock);
             return;
         }
-        
-        bsg_mach_binary_images.contents = (BSG_Mach_Binary_Image_Info *)newData;
     }
     
     // Store the value, increment the number of used elements
     bsg_mach_binary_images.contents[bsg_mach_binary_images.used++] = element;
     
-    unlock();
+    bsg_unlock_mach_binary_image_access(&bsg_mach_binary_images_access_lock);
 }
 
 /**
@@ -77,7 +57,7 @@ void bsg_add_mach_binary_image(BSG_Mach_Binary_Image_Info element) {
  */
 void bsg_remove_mach_binary_image(uint64_t imageVmAddr) {
     
-    lock();
+    bsg_lock_mach_binary_image_access(&bsg_mach_binary_images_access_lock);
     
     for (size_t i=0; i<bsg_mach_binary_images.used; i++) {
         BSG_Mach_Binary_Image_Info item = bsg_mach_binary_images.contents[i];
@@ -94,7 +74,7 @@ void bsg_remove_mach_binary_image(uint64_t imageVmAddr) {
         }
     }
     
-    unlock();
+    bsg_unlock_mach_binary_image_access(&bsg_mach_binary_images_access_lock);
 }
 
 void bsg_initialise_mach_binary_headers(size_t initialSize) {