impr: Avoid work when storing invalid envelopes

CHANGELOG.md

@@ -20,6 +20,10 @@
 - Session replay for crash not created because of a race condition (#4314)
 - Double-quoted include, expected angle-bracketed instead (#4298)
 
+### Improvements
+
+- Avoid extra work when storing invalid envelopes (#4337)
+
 ## 8.36.0
 
 ### Features

Sentry.xcodeproj/project.pbxproj

@@ -133,6 +133,10 @@
 		62C316812B1F2E93000D7031 /* SentryDelayedFramesTracker.h in Headers */ = {isa = PBXBuildFile; fileRef = 62C316802B1F2E93000D7031 /* SentryDelayedFramesTracker.h */; };
 		62C316832B1F2EA1000D7031 /* SentryDelayedFramesTracker.m in Sources */ = {isa = PBXBuildFile; fileRef = 62C316822B1F2EA1000D7031 /* SentryDelayedFramesTracker.m */; };
 		62C3168B2B1F865A000D7031 /* SentryTimeSwiftTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */; };
+		62CFD9A92C99741100834E1B /* SentryInvalidJSONStringTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62CFD9A82C99741100834E1B /* SentryInvalidJSONStringTests.swift */; };
+		62CFD9AB2C9976E700834E1B /* SentryInvalidJSONString.h in Headers */ = {isa = PBXBuildFile; fileRef = 62CFD9AA2C9976E700834E1B /* SentryInvalidJSONString.h */; };
+		62CFD9AD2C99770B00834E1B /* SentryInvalidJSONString.m in Sources */ = {isa = PBXBuildFile; fileRef = 62CFD9AC2C99770B00834E1B /* SentryInvalidJSONString.m */; };
+		62CFD9AE2C99770B00834E1B /* SentryInvalidJSONString.m in Sources */ = {isa = PBXBuildFile; fileRef = 62CFD9AC2C99770B00834E1B /* SentryInvalidJSONString.m */; };
 		62E081A929ED4260000F69FC /* SentryBreadcrumbDelegate.h in Headers */ = {isa = PBXBuildFile; fileRef = 62E081A829ED4260000F69FC /* SentryBreadcrumbDelegate.h */; };
 		62E081AB29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62E081AA29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift */; };
 		62E146D02BAAE47600ED34FD /* LocalMetricsAggregator.swift in Sources */ = {isa = PBXBuildFile; fileRef = 62E146CF2BAAE47600ED34FD /* LocalMetricsAggregator.swift */; };
@@ -1121,6 +1125,9 @@
 		62C316802B1F2E93000D7031 /* SentryDelayedFramesTracker.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryDelayedFramesTracker.h; path = include/SentryDelayedFramesTracker.h; sourceTree = "<group>"; };
 		62C316822B1F2EA1000D7031 /* SentryDelayedFramesTracker.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryDelayedFramesTracker.m; sourceTree = "<group>"; };
 		62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryTimeSwiftTests.swift; sourceTree = "<group>"; };
+		62CFD9A82C99741100834E1B /* SentryInvalidJSONStringTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryInvalidJSONStringTests.swift; sourceTree = "<group>"; };
+		62CFD9AA2C9976E700834E1B /* SentryInvalidJSONString.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SentryInvalidJSONString.h; sourceTree = "<group>"; };
+		62CFD9AC2C99770B00834E1B /* SentryInvalidJSONString.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryInvalidJSONString.m; sourceTree = "<group>"; };
 		62E081A829ED4260000F69FC /* SentryBreadcrumbDelegate.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryBreadcrumbDelegate.h; path = include/SentryBreadcrumbDelegate.h; sourceTree = "<group>"; };
 		62E081AA29ED4322000F69FC /* SentryBreadcrumbTestDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryBreadcrumbTestDelegate.swift; sourceTree = "<group>"; };
 		62E146CF2BAAE47600ED34FD /* LocalMetricsAggregator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocalMetricsAggregator.swift; sourceTree = "<group>"; };
@@ -3099,6 +3106,7 @@
 				62C3168A2B1F865A000D7031 /* SentryTimeSwiftTests.swift */,
 				33042A1629DC2C4300C60085 /* SentryExtraContextProviderTests.swift */,
 				62375FB82B47F9F000CC55F1 /* SentryDependencyContainerTests.swift */,
+				62CFD9A82C99741100834E1B /* SentryInvalidJSONStringTests.swift */,
 			);
 			path = Helper;
 			sourceTree = "<group>";
@@ -3362,6 +3370,8 @@
 				62F226B829A37C270038080D /* SentryBooleanSerialization.h */,
 				62F226B629A37C120038080D /* SentryBooleanSerialization.m */,
 				62B86CFB29F052BB008F3947 /* SentryTestLogConfig.m */,
+				62CFD9AA2C9976E700834E1B /* SentryInvalidJSONString.h */,
+				62CFD9AC2C99770B00834E1B /* SentryInvalidJSONString.m */,
 			);
 			path = TestUtils;
 			sourceTree = "<group>";
@@ -4262,6 +4272,7 @@
 			buildActionMask = 2147483647;
 			files = (
 				841325C52BF49EC40029228F /* SentryLaunchProfiling+Tests.h in Headers */,
+				62CFD9AB2C9976E700834E1B /* SentryInvalidJSONString.h in Headers */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -4886,6 +4897,7 @@
 				7B6D98EB24C6E84F005502FA /* SentryCrashInstallationReporterTests.swift in Sources */,
 				7BA61EA625F21E660008CAA2 /* SentryLogTests.swift in Sources */,
 				7B6D1263265F7CC600C9BE4B /* PrivateSentrySDKOnlyTests.swift in Sources */,
+				62CFD9A92C99741100834E1B /* SentryInvalidJSONStringTests.swift in Sources */,
 				7BB42EF124F3B7B700D7B39A /* SentrySession+Equality.m in Sources */,
 				7BF9EF8B2722D58700B5BBEF /* SentryInitializeForGettingSubclassesNotCalled.m in Sources */,
 				33042A1729DC2C4300C60085 /* SentryExtraContextProviderTests.swift in Sources */,
@@ -4942,6 +4954,7 @@
 				D8F6A24E288553A800320515 /* SentryPredicateDescriptorTests.swift in Sources */,
 				7B18DE4A28DA0C8B004845C6 /* SentryNSNotificationCenterWrapperTests.swift in Sources */,
 				7B0002322477F0520035FEF1 /* SentrySessionTests.m in Sources */,
+				62CFD9AD2C99770B00834E1B /* SentryInvalidJSONString.m in Sources */,
 				62375FB92B47F9F000CC55F1 /* SentryDependencyContainerTests.swift in Sources */,
 				7BC6EC08255C36DE0059822A /* SentryStacktraceTests.swift in Sources */,
 				D88817DD26D72BA500BF2251 /* SentryTraceStateTests.swift in Sources */,
@@ -5127,6 +5140,7 @@
 				84B7FA3C29B2876F00AD93B1 /* TestConstants.swift in Sources */,
 				8431F01A29B2852D00D8DC56 /* Dynamic.swift in Sources */,
 				84B7FA4329B28D8C00AD93B1 /* Invocations.swift in Sources */,
+				62CFD9AE2C99770B00834E1B /* SentryInvalidJSONString.m in Sources */,
 				84B7FA4029B28BAD00AD93B1 /* TestTransportAdapter.swift in Sources */,
 				84B7FA4429B2924000AD93B1 /* TestRandom.swift in Sources */,
 				8431F01B29B2852D00D8DC56 /* Logger.swift in Sources */,

Sources/Sentry/SentryFileManager.m

@@ -310,10 +310,15 @@ - (void)removeFileAtPath:(NSString *)path
     }
 }
 
-- (NSString *)storeEnvelope:(SentryEnvelope *)envelope
+- (nullable NSString *)storeEnvelope:(SentryEnvelope *)envelope
 {
     NSData *envelopeData = [SentrySerialization dataWithEnvelope:envelope];
 
+    if (envelopeData == nil) {
+        SENTRY_LOG_ERROR(@"Serialization of envelope failed. Can't store envelope.");
+        return nil;
+    }
+
     @synchronized(self) {
         NSString *path =
             [self.envelopesPath stringByAppendingPathComponent:[self uniqueAscendingJsonName]];

Sources/Sentry/include/SentryFileManager.h

@@ -38,7 +38,7 @@ SENTRY_NO_INIT
 
 - (void)setDelegate:(id<SentryFileManagerDelegate>)delegate;
 
-- (NSString *)storeEnvelope:(SentryEnvelope *)envelope;
+- (nullable NSString *)storeEnvelope:(SentryEnvelope *)envelope;
 
 - (void)storeCurrentSession:(SentrySession *)session;
 - (void)storeCrashedSession:(SentrySession *)session;

Tests/SentryTests/Helper/SentryFileManagerTests.swift

@@ -134,6 +134,15 @@ class SentryFileManagerTests: XCTestCase {
         XCTAssertEqual(expectedData, actualData as Data)
     }
 
+    func testStoreInvalidEnvelope_ReturnsNil() {
+        let sdkInfoWithInvalidJSON = SentrySdkInfo(name: SentryInvalidJSONString() as String, andVersion: "8.0.0")
+        let headerWithInvalidJSON = SentryEnvelopeHeader(id: nil, sdkInfo: sdkInfoWithInvalidJSON, traceContext: nil)
+
+        let envelope = SentryEnvelope(header: headerWithInvalidJSON, items: [])
+
+        XCTAssertNil(sut.store(envelope))
+    }
+
     func testDeleteOldEnvelopes() throws {
         try givenOldEnvelopes()
 
@@ -220,7 +229,7 @@ class SentryFileManagerTests: XCTestCase {
 
     func testDontDeleteYoungEnvelopes() throws {
         let envelope = TestConstants.envelope
-        let path = sut.store(envelope)
+        let path = try XCTUnwrap(sut.store(envelope))
 
         let timeIntervalSince1970 = fixture.currentDateProvider.date().timeIntervalSince1970 - (90 * 24 * 60 * 60)
         let date = Date(timeIntervalSince1970: timeIntervalSince1970)
@@ -410,7 +419,7 @@ class SentryFileManagerTests: XCTestCase {
         }
 
         // Trying to load the file content of a directory is going to return nil for the envelope.
-        let envelopePath = sut.store(TestConstants.envelope)
+        let envelopePath = try XCTUnwrap(sut.store(TestConstants.envelope))
         let fileManager = FileManager.default
         try! fileManager.removeItem(atPath: envelopePath)
         try! fileManager.createDirectory(atPath: envelopePath, withIntermediateDirectories: false, attributes: nil)
@@ -854,7 +863,7 @@ private extension SentryFileManagerTests {
 
     func givenOldEnvelopes() throws {
         let envelope = TestConstants.envelope
-        let path = sut.store(envelope)
+        let path = try XCTUnwrap(sut.store(envelope))
 
         let timeIntervalSince1970 = fixture.currentDateProvider.date().timeIntervalSince1970 - (90 * 24 * 60 * 60)
         let date = Date(timeIntervalSince1970: timeIntervalSince1970 - 1)

Tests/SentryTests/Helper/SentryInvalidJSONStringTests.swift

@@ -0,0 +1,22 @@
+import XCTest
+
+final class SentryInvalidJSONStringTests: XCTestCase {
+
+    func testDefaultInit_ReturnsInvalidJSONObject() throws {
+        let sut = SentryInvalidJSONString()
+
+        let array = [sut]
+        XCTAssertFalse(JSONSerialization.isValidJSONObject(array))
+        XCTAssertFalse(JSONSerialization.isValidJSONObject(array))
+    }
+
+    func testInitWithInvocations_ReturnsValidJSONUntilInvocationsReached() {
+        let sut = SentryInvalidJSONString(lengthInvocationsToBeInvalid: 2)
+
+        let array = [sut]
+        XCTAssertTrue(JSONSerialization.isValidJSONObject(array))
+        XCTAssertTrue(JSONSerialization.isValidJSONObject(array))
+        XCTAssertFalse(JSONSerialization.isValidJSONObject(array))
+        XCTAssertFalse(JSONSerialization.isValidJSONObject(array))
+    }
+}

Tests/SentryTests/Helper/SentrySerializationNilTests.m

@@ -1,3 +1,4 @@
+#import "SentryEnvelope.h"
 #import "SentrySerialization.h"
 #import <XCTest/XCTest.h>
 

Tests/SentryTests/Helper/SentrySerializationTests.swift

@@ -17,6 +17,30 @@ class SentrySerializationTests: XCTestCase {
         XCTAssertNil(data)
     }
 
+    func testSerializationFailsWithFirstValidAndThenInvalidJSONObject() {
+        let json = [ SentryInvalidJSONString(lengthInvocationsToBeInvalid: 1)]
+        let data = SentrySerialization.data(withJSONObject: json)
+        XCTAssertNil(data)
+    }
+
+    func testDataWithEnvelope_InvalidEnvelopeHeaderJSON_ReturnsNil() {
+        let sdkInfoWithInvalidJSON = SentrySdkInfo(name: SentryInvalidJSONString() as String, andVersion: "8.0.0")
+        let headerWithInvalidJSON = SentryEnvelopeHeader(id: nil, sdkInfo: sdkInfoWithInvalidJSON, traceContext: nil)
+
+        let envelope = SentryEnvelope(header: headerWithInvalidJSON, items: [])
+
+        XCTAssertNil(SentrySerialization.data(with: envelope))
+    }
+
+    func testDataWithEnvelope_InvalidEnvelopeItemHeaderJSON_ReturnsNil() throws {
+        let envelopeItemHeader = SentryEnvelopeItemHeader(type: SentryInvalidJSONString() as String, length: 0)
+        let envelopeItem = SentryEnvelopeItem(header: envelopeItemHeader, data: Data())
+
+        let envelope = SentryEnvelope(header: SentryEnvelopeHeader(id: SentryId()), singleItem: envelopeItem)
+
+        XCTAssertNil(SentrySerialization.data(with: envelope))
+    }
+
     func testSentryEnvelopeSerializer_WithSingleEvent() throws {
         // Arrange
         let event = Event()

Tests/SentryTests/Networking/SentryHttpTransportTests.swift

@@ -470,7 +470,7 @@ class SentryHttpTransportTests: XCTestCase {
     }
 
     func testAllCachedEnvelopesCantDeserializeEnvelope() throws {
-        let path = fixture.fileManager.store(TestConstants.envelope)
+        let path = try XCTUnwrap( fixture.fileManager.store(TestConstants.envelope))
         let faultyEnvelope = Data([0x70, 0xa3, 0x10, 0x45])
         try faultyEnvelope.write(to: URL(fileURLWithPath: path))
 

Tests/SentryTests/SentryTests-Bridging-Header.h

@@ -134,6 +134,7 @@
 #import "SentryInstallation+Test.h"
 #import "SentryInstallation.h"
 #import "SentryInternalNotificationNames.h"
+#import "SentryInvalidJSONString.h"
 #import "SentryLevelMapper.h"
 #import "SentryLog.h"
 #import "SentryLogTestHelper.h"

Tests/SentryTests/TestUtils/SentryInvalidJSONString.h

@@ -0,0 +1,17 @@
+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+/**
+ * A subclass of NSString containing an invalid JSON string. This class is helpful in writing tests
+ * for NSJSONSerialization to create invalid JSON objects. Furthermore, you can use
+ * initWithLengthInvocationsToBeInvalid to specify after how many NSString.length invocations, the
+ * string should become an invalid JSON object.
+ */
+@interface SentryInvalidJSONString : NSString
+
+- (instancetype)initWithLengthInvocationsToBeInvalid:(NSInteger)lengthInvocationsToBeInvalid;
+
+@end
+
+NS_ASSUME_NONNULL_END

Tests/SentryTests/TestUtils/SentryInvalidJSONString.m

@@ -0,0 +1,60 @@
+#import "SentryInvalidJSONString.h"
+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface
+SentryInvalidJSONString ()
+
+@property (nonatomic, strong) NSString *stringHolder;
+@property (nonatomic, assign) NSUInteger lengthInvocations;
+@property (nonatomic, assign) NSUInteger lengthInvocationsToBeInvalid;
+
+@end
+
+@implementation SentryInvalidJSONString
+
+- (instancetype)initWithCharactersNoCopy:(unichar *)characters
+                                  length:(NSUInteger)length
+                            freeWhenDone:(BOOL)freeBuffer
+{
+    if (self = [super init]) {
+        // Empty on purpose
+    }
+    return self;
+}
+
+- (instancetype)initWithLengthInvocationsToBeInvalid:(NSInteger)lengthInvocationsToBeInvalid
+{
+
+    if (self = [super init]) {
+        self.lengthInvocations = 0;
+        self.lengthInvocationsToBeInvalid = lengthInvocationsToBeInvalid;
+    }
+    return self;
+}
+
+- (NSUInteger)length
+{
+    self.lengthInvocations++;
+
+    if (self.lengthInvocations > self.lengthInvocationsToBeInvalid) {
+        NSMutableString *invalidString = [NSMutableString stringWithString:@"invalid string"];
+        [invalidString appendFormat:@"%C", 0xD800]; // Invalid UTF-16 surrogate pair
+
+        _stringHolder = invalidString;
+
+    } else {
+        _stringHolder = @"valid string";
+    }
+
+    return self.stringHolder.length;
+}
+
+- (unichar)characterAtIndex:(NSUInteger)index
+{
+    return [self.stringHolder characterAtIndex:index];
+}
+
+@end
+
+NS_ASSUME_NONNULL_END