fix: Check for NULL in binary image cache (#3469)

Properly validate the binary image name when converting it to an NSString to avoid crashes.
getsentry · Dec 1, 2023 · bfe863d · bfe863d
1 parent 5ab8ec2
commit bfe863d
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 
 - Crash when UINavigationController doesn't have rootViewController (#3455)
 - Crash when synchronizing invalid JSON breadcrumbs to SentryWatchdogTermination (#3458)
+- Check for NULL in binary image cache (#3469)
 - Threading issues in binary image cache (#3468)
 - Finish transaction for external view controllers (#3440)
 

diff --git a/Sources/Sentry/SentryBinaryImageCache.m b/Sources/Sentry/SentryBinaryImageCache.m
@@ -2,6 +2,7 @@
 #import "SentryCrashBinaryImageCache.h"
 #import "SentryDependencyContainer.h"
 #import "SentryInAppLogic.h"
+#import "SentryLog.h"
 
 static void binaryImageWasAdded(const SentryCrashBinaryImage *image);
 
@@ -39,8 +40,26 @@ - (void)stop
 
 - (void)binaryImageAdded:(const SentryCrashBinaryImage *)image
 {
+    if (image == NULL) {
+        SENTRY_LOG_WARN(@"The image is NULL. Can't add NULL to cache.");
+        return;
+    }
+
+    if (image->name == NULL) {
+        SENTRY_LOG_WARN(@"The image name was NULL. Can't add image to cache.");
+        return;
+    }
+
+    NSString *imageName = [NSString stringWithCString:image->name encoding:NSUTF8StringEncoding];
+
+    if (imageName == nil) {
+        SENTRY_LOG_WARN(@"Couldn't convert the cString image name to an NSString. This could be "
+                        @"due to a different encoding than NSUTF8StringEncoding of the cString..");
+        return;
+    }
+
     SentryBinaryImageInfo *newImage = [[SentryBinaryImageInfo alloc] init];
-    newImage.name = [NSString stringWithCString:image->name encoding:NSUTF8StringEncoding];
+    newImage.name = imageName;
     newImage.address = image->address;
     newImage.size = image->size;
 
@@ -64,6 +83,11 @@ - (void)binaryImageAdded:(const SentryCrashBinaryImage *)image
 
 - (void)binaryImageRemoved:(const SentryCrashBinaryImage *)image
 {
+    if (image == NULL) {
+        SENTRY_LOG_WARN(@"The image is NULL. Can't remove it from the cache.");
+        return;
+    }
+
     @synchronized(self) {
         NSInteger index = [self indexOfImage:image->address];
         if (index >= 0) {

diff --git a/Tests/SentryTests/SentryBinaryImageCacheTests.swift b/Tests/SentryTests/SentryBinaryImageCacheTests.swift
@@ -1,4 +1,5 @@
 import Nimble
+import SentryTestUtils
 import XCTest
 
 class SentryBinaryImageCacheTests: XCTestCase {
@@ -41,6 +42,12 @@ class SentryBinaryImageCacheTests: XCTestCase {
         XCTAssertEqual(sut.cache.first?.name, "Expected Name at 0")
         XCTAssertEqual(sut.cache[1].name, "Expected Name at 100")
     }
+
+    func testBinaryImageAdded_IsNull() {
+        sut.binaryImageAdded(nil)
+
+        expect(self.sut.cache.count) == 0
+    }
 
     func testBinaryImageRemoved() {
         var binaryImage0 = createCrashBinaryImage(0)
@@ -74,6 +81,15 @@ class SentryBinaryImageCacheTests: XCTestCase {
         XCTAssertEqual(sut.cache.count, 0)
         XCTAssertNil(sut.image(byAddress: 240))
     }
+
+    func testBinaryImageRemoved_IsNull() {
+        var binaryImage = createCrashBinaryImage(0)
+        sut.binaryImageAdded(&binaryImage)
+
+        sut.binaryImageRemoved(nil)
+
+        expect(self.sut.cache.count) == 1
+    }
 
     func testImageNameByAddress() {
         var binaryImage0 = createCrashBinaryImage(0)
@@ -116,6 +132,56 @@ class SentryBinaryImageCacheTests: XCTestCase {
         expect(didNotFind) == nil
     }
 
+    func testBinaryImageWithNULLName_DoesNotAddImage() {
+        let address = UInt64(100)
+
+        var binaryImage = SentryCrashBinaryImage(
+            address: address,
+            vmAddress: 0,
+            size: 100,
+            name: nil,
+            uuid: nil,
+            cpuType: 1,
+            cpuSubType: 1,
+            majorVersion: 1,
+            minorVersion: 0,
+            revisionVersion: 0,
+            crashInfoMessage: nil,
+            crashInfoMessage2: nil
+        )
+
+        sut.binaryImageAdded(&binaryImage)
+        expect(self.sut.image(byAddress: address)) == nil
+        expect(self.sut.cache.count) == 0
+    }
+
+    func testBinaryImageNameDifferentEncoding_DoesNotAddImage() {
+        let name = NSString(string: "こんにちは") // "Hello" in Japanese
+        // 8 = NSShiftJISStringEncoding
+        // Passing NSShiftJISStringEncoding directly doesn't work on older Xcode versions.
+        let nameCString = name.cString(using: UInt(8))
+        let address = UInt64(100)
+
+        var binaryImage = SentryCrashBinaryImage(
+            address: address,
+            vmAddress: 0,
+            size: 100,
+            name: nameCString,
+            uuid: nil,
+            cpuType: 1,
+            cpuSubType: 1,
+            majorVersion: 1,
+            minorVersion: 0,
+            revisionVersion: 0,
+            crashInfoMessage: nil,
+            crashInfoMessage2: nil
+        )
+
+        sut.binaryImageAdded(&binaryImage)
+        expect(self.sut.image(byAddress: address)) == nil
+        expect(self.sut.cache.count) == 0
+    }
+
     func testAddingImagesWhileStoppingAndStartingOnDifferentThread() {
         let count = 1_000