Fix Crashlytics abort on binary image records with a non-numeric base or size

Crashlytics/CHANGELOG.md

@@ -1,3 +1,7 @@
+# Unreleased
+- [fixed] Fixed a crash when processing a report whose binary image records
+  contain a non-numeric `base` or `size`. (#16519)
+
 # 12.18.0
 - [removed] Removes unused integration with the now deprecated ObjC MetricKit API.
 

Crashlytics/Crashlytics/Models/FIRCLSSymbolResolver.m

@@ -60,14 +60,11 @@ - (BOOL)loadBinaryImagesFromFile:(NSString*)path {
       continue;
     }
 
-    // This does happen occasionally and causes a crash. I'm really not sure there
-    // is anything sane we can do in this case.
-    if (![details objectForKey:@"base"] || ![details objectForKey:@"size"]) {
-      continue;
-    }
-
-    if ([details objectForKey:@"base"] == (id)[NSNull null] ||
-        [details objectForKey:@"size"] == (id)[NSNull null]) {
+    // base and size come from JSON and are occasionally missing, NSNull, or some other
+    // non-numeric type, which would crash the sort comparator below.
+    id base = [details objectForKey:@"base"];
+    id size = [details objectForKey:@"size"];
+    if (![base isKindOfClass:[NSNumber class]] || ![size isKindOfClass:[NSNumber class]]) {
       continue;
     }
 

Crashlytics/UnitTests/Data/binary_images_with_string_base_entry.clsrecord

@@ -0,0 +1,6 @@
+{"load":{"path":"/Users/mmassicotte/Library/Developer/CoreSimulator/Devices/5FC72D9A-2048-482A-9A79-2CA9F1FC18FB/data/Containers/Bundle/Application/788D77C9-161A-4021-9774-7E3C9393AD45/CrashTest-iOS.app/CrashTest-iOS","uuid":"e07d35c9f99a3a82a3638faf8775eccc","base":4389027840,"size":598016}}
+{"load":{"path":"/Applications/Xcode 6.2.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator8.2.sdk/Developer/Library/PrivateFrameworks/DTDDISupport.framework/libViewDebuggerSupport.dylib","bundle_id":null,"build_version":null,"display_version":null,"uuid":"7f81304e1e243c9c928a1cae4217e116","base":4390633472,"size":24576}}
+{"load":{"path":"/Applications/Xcode 6.2.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/System/Library/Frameworks/AdSupport.framework/AdSupport","bundle_id":null,"build_version":null,"display_version":null,"uuid":"be521a1e2d003fa8a1e84deb6c3ebfe5","base":4390678528,"size":4096}}
+{"load":{"path":"/Applications/Xcode 6.2.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/usr/lib/libz.1.dylib","uuid":"7cca2d44f20a35a59b7cb026a188f4f5","base":4390699008,"size":77824}}
+{"load":{"path":"/Applications/Xcode 6.2.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/System/Library/Frameworks/CoreGraphics.framework/CoreGraphics","bundle_id":null,"build_version":null,"display_version":null,"uuid":"9a455d304f0735be8a03253c5b17aecd","base":4390797312,"size":1884160}}
+{"load":{"path":"/Applications/Xcode 6.2.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/System/Library/Frameworks/UIKit.framework/UIKit","bundle_id":null,"build_version":null,"display_version":null,"uuid":"cdd3270b77223f4f933368746205a9cf","base":"0x105a3c000","size":11964416}}

Crashlytics/UnitTests/FIRCLSSymbolResolverTests.m

@@ -16,6 +16,12 @@
 
 #import <XCTest/XCTest.h>
 
+// -loadedBinaryImageForPC: is internal to FIRCLSSymbolResolver, but it is the only way to
+// observe which records survived filtering.
+@interface FIRCLSSymbolResolver (Testing)
+- (NSDictionary*)loadedBinaryImageForPC:(uintptr_t)pc;
+@end
+
 @interface FIRCLSSymbolResolverTests : XCTestCase
 
 @end
@@ -68,4 +74,18 @@ - (void)testLoadingBinaryImagesWithMissingBaseValue {
   XCTAssert([resolver loadBinaryImagesFromFile:binaryImagePath]);
 }
 
+- (void)testLoadingBinaryImagesWithStringBaseValue {
+  FIRCLSSymbolResolver* resolver = [[FIRCLSSymbolResolver alloc] init];
+
+  NSString* binaryImagePath =
+      [self pathForResource:@"binary_images_with_string_base_entry.clsrecord"];
+
+  XCTAssert([resolver loadBinaryImagesFromFile:binaryImagePath]);
+
+  // The record with the string base covers 0x105a3c000 and up, it should have been skipped.
+  XCTAssertNil([resolver loadedBinaryImageForPC:4395000000]);
+
+  XCTAssertNotNil([resolver loadedBinaryImageForPC:4389027840]);
+}
+
 @end