Merge pull request #133 from DataDog/ncreated/RUMM-506-prevent-malfor…

…ming-payload-on-io-exceptions

RUMM-506 Prevent malforming payload due to I/O exception

DatadogSDK.podspec

@@ -1,7 +1,7 @@
 Pod::Spec.new do |s|
   s.name         = "DatadogSDK"
   s.module_name  = "Datadog"
-  s.version      = "1.2.1"
+  s.version      = "1.2.2"
   s.summary      = "Official Datadog Swift SDK for iOS."
 
   s.homepage     = "https://www.datadoghq.com"

DatadogSDKObjc.podspec

@@ -1,7 +1,7 @@
 Pod::Spec.new do |s|
   s.name         = "DatadogSDKObjc"
   s.module_name  = "DatadogObjc"
-  s.version      = "1.2.1"
+  s.version      = "1.2.2"
   s.summary      = "Official Datadog Objective-C SDK for iOS."
 
   s.homepage     = "https://www.datadoghq.com"

Sources/Datadog/Core/Persistence/FileWriter.swift

@@ -42,9 +42,9 @@ internal final class FileWriter {
                     try write(data)
                 }
             } else {
+                let atomicData = commaSeparatorData + data
                 try file.append { write in
-                    try write(commaSeparatorData)
-                    try write(data)
+                    try write(atomicData)
                 }
             }
         } catch {

Sources/Datadog/Datadog.swift

@@ -8,7 +8,7 @@ import Foundation
 
 /// SDK version associated with logs.
 /// Should be synced with SDK releases.
-internal let sdkVersion = "1.2.1"
+internal let sdkVersion = "1.2.2"
 
 /// Datadog SDK configuration object.
 public class Datadog {

Tests/DatadogTests/Datadog/Core/Persistence/FileWriterTests.swift

@@ -40,6 +40,74 @@ class FileWriterTests: XCTestCase {
         )
     }
 
+    /// NOTE: Test added after incident-4797
+    func testGivenFileContainingData_whenNextDataFails_itDoesNotMalformTheEndOfTheFile() throws {
+        let previousObjcExceptionHandler = objcExceptionHandler
+        defer { objcExceptionHandler = previousObjcExceptionHandler }
+
+        let expectation = self.expectation(description: "write completed")
+        let writer = FileWriter(
+            orchestrator: .mockWriteToSingleFile(in: temporaryDirectory),
+            queue: queue
+        )
+
+        objcExceptionHandler = ObjcExceptionHandlerDeferredMock(
+            throwingError: ErrorMock("I/O exception"),
+            /*
+                Following the logic in `FileWriter` and `File`, the 3 comes from:
+                - succeed on `fileHandle.seekToEndOfFile()` to prepare the file for the first write
+                - succeed on `fileHandle.write(_:)` for `writer.write(value: ["key1": "value1"])`
+                - succeed on `fileHandle.seekToEndOfFile()` to prepare the file for the second `writer.write(value:)`
+                - throw an `I/O exception` for `fileHandle.write(_:)` for the second write
+             */
+            afterSucceedingCallsCounts: 3
+        )
+
+        writer.write(value: ["key1": "value1"]) // first write (2 calls to `ObjcExceptionHandler`)
+        writer.write(value: ["key2": "value2"]) // second write (2 calls to `ObjcExceptionHandler`, where the latter fails)
+
+        waitForWritesCompletion(on: queue, thenFulfill: expectation)
+        waitForExpectations(timeout: 1, handler: nil)
+        XCTAssertEqual(try temporaryDirectory.files().count, 1)
+
+        XCTAssertEqual(
+            try temporaryDirectory.files()[0].read().utf8String,
+            #"{"key1":"value1"}"# // second write should be ignored due to `I/O exception`
+        )
+    }
+
+    /// NOTE: Test added after incident-4797
+    func testWhenIOExceptionsHappenRandomly_theFileIsNeverMalformed() throws {
+        let previousObjcExceptionHandler = objcExceptionHandler
+        defer { objcExceptionHandler = previousObjcExceptionHandler }
+
+        let expectation = self.expectation(description: "write completed")
+        let writer = FileWriter(
+            orchestrator: .mockWriteToSingleFile(in: temporaryDirectory),
+            queue: queue
+        )
+
+        objcExceptionHandler = ObjcExceptionHandlerNonDeterministicMock(
+            throwingError: ErrorMock("I/O exception"),
+            withProbability: 0.2
+        )
+
+        struct Foo: Codable {
+            let foo = "bar"
+        }
+
+        (0...500).forEach { _ in writer.write(value: Foo()) }
+
+        waitForWritesCompletion(on: queue, thenFulfill: expectation)
+        waitForExpectations(timeout: 5, handler: nil)
+        XCTAssertEqual(try temporaryDirectory.files().count, 1)
+
+        let fileData = try temporaryDirectory.files()[0].read()
+        let jsonDecoder = JSONDecoder()
+
+        XCTAssertNoThrow(try jsonDecoder.decode([Foo].self, from: "[".utf8Data + fileData + "]".utf8Data))
+    }
+
     func testGivenErrorVerbosity_whenIndividualDataExceedsMaxWriteSize_itDropsDataAndPrintsError() throws {
         let expectation1 = self.expectation(description: "write completed")
         let expectation2 = self.expectation(description: "second write completed")

Tests/DatadogTests/Datadog/Mocks/DatadogPrivateMocks.swift

@@ -21,3 +21,46 @@ class ObjcExceptionHandlerMock: ObjcExceptionHandler {
         throw error
     }
 }
+
+/// An `ObjcExceptionHandler` which results with no error for the first `afterTimes` number of calls.
+/// Throws given `throwingError` for all other calls.
+class ObjcExceptionHandlerDeferredMock: ObjcExceptionHandler {
+    private let succeedingCallsCounts: Int
+    private var currentCallsCount = 0
+
+    let error: Error
+
+    init(throwingError: Error, afterSucceedingCallsCounts succeedingCallsCounts: Int) {
+        self.error = throwingError
+        self.succeedingCallsCounts = succeedingCallsCounts
+    }
+
+    override func rethrowToSwift(tryBlock: @escaping () -> Void) throws {
+        if currentCallsCount >= succeedingCallsCounts {
+            throw error
+        } else {
+            tryBlock()
+        }
+        currentCallsCount += 1
+    }
+}
+
+/// An `ObjcExceptionHandler` which throws given error with given probability.
+class ObjcExceptionHandlerNonDeterministicMock: ObjcExceptionHandler {
+    private let probability: Int
+    let error: Error
+
+    /// Probability should be described as a number between `0` and `1`
+    init(throwingError: Error, withProbability probability: Double) {
+        self.error = throwingError
+        self.probability = Int(probability * 1_000)
+    }
+
+    override func rethrowToSwift(tryBlock: @escaping () -> Void) throws {
+        if Int.random(in: 0...1_000) < probability {
+            throw error
+        } else {
+            tryBlock()
+        }
+    }
+}