Prevent infinite recursion loop while reading messages (#52)

* Create internal initializer for CacheAdvance to enable testing pathological test cases

* Write test that will fail

* Fix test

* Attempt to prevent the test scenario from happening again

* Bugfix version bump

* Dan learns to type

Co-authored-by: Steven Hepting <shepting@gmail.com>

* Better comment

Co-authored-by: Francisco Diaz <fdiaz@users.noreply.github.com>

* Document that value is random

Co-authored-by: Steven Hepting <shepting@gmail.com>
Co-authored-by: Francisco Diaz <fdiaz@users.noreply.github.com>

CacheAdvance.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
   s.name     = 'CacheAdvance'
-  s.version  = '1.2.0'
+  s.version  = '1.2.1'
   s.license  = 'Apache License, Version 2.0'
   s.summary  = 'A performant cache for logging systems. CacheAdvance persists log events 30x faster than SQLite.'
   s.homepage = 'https://github.com/dfed/CacheAdvance'

Sources/CacheAdvance/CacheAdvance.swift

@@ -36,20 +36,39 @@ public final class CacheAdvance<T: Codable> {
     /// - Warning: `shouldOverwriteOldMessages` must be consistent for the life of a cache. Changing this value after logs have been persisted to a cache will prevent appending new messages to this cache.
     /// - Warning: `decoder` must have a consistent implementation for the life of a cache. Changing this value after logs have been persisted to a cache may prevent reading messages from this cache.
     /// - Warning: `encoder` must have a consistent implementation for the life of a cache. Changing this value after logs have been persisted to a cache may prevent reading messages from this cache.
-    public init(
+    public convenience init(
         fileURL: URL,
         maximumBytes: Bytes,
         shouldOverwriteOldMessages: Bool,
         decoder: MessageDecoder = JSONDecoder(),
         encoder: MessageEncoder = JSONEncoder())
         throws
     {
-        self.fileURL = fileURL
-
-        writer = try FileHandle(forWritingTo: fileURL)
-        reader = try CacheReader(forReadingFrom: fileURL)
-        header = try CacheHeaderHandle(forReadingFrom: fileURL, maximumBytes: maximumBytes, overwritesOldMessages: shouldOverwriteOldMessages)
+        self.init(
+            fileURL: fileURL,
+            writer: try FileHandle(forWritingTo: fileURL),
+            reader: try CacheReader(forReadingFrom: fileURL),
+            header: try CacheHeaderHandle(
+                forReadingFrom: fileURL,
+                maximumBytes: maximumBytes,
+                overwritesOldMessages: shouldOverwriteOldMessages),
+            decoder: decoder,
+            encoder: encoder)
+    }
 
+    /// An internal initializer with no logic. Can be used to create pathological test cases.
+    required init(
+        fileURL: URL,
+        writer: FileHandle,
+        reader: CacheReader,
+        header: CacheHeaderHandle,
+        decoder: MessageDecoder,
+        encoder: MessageEncoder)
+    {
+        self.fileURL = fileURL
+        self.writer = writer
+        self.reader = reader
+        self.header = header
         self.decoder = decoder
         self.encoder = encoder
     }
@@ -93,12 +112,16 @@ public final class CacheAdvance<T: Codable> {
 
         let cacheHasSpaceForNewMessageBeforeEndOfFile = writer.offsetInFile + bytesNeededToStoreMessage <= header.maximumBytes
         if header.overwritesOldMessages {
-            if !cacheHasSpaceForNewMessageBeforeEndOfFile {
+            let truncateAtOffset: UInt64?
+            if cacheHasSpaceForNewMessageBeforeEndOfFile {
+                // We have room for this message. No need to truncate.
+                truncateAtOffset = nil
+            } else {
                 // This message can't be written without exceeding our maximum file length.
                 // We'll need to start writing the file from the beginning of the file.
 
-                // Trim the file to the current position to remove soon-to-be-abandoned data from the file.
-                try writer.truncate(at: writer.offsetInFile)
+                // Trim the file to the current writer position to remove soon-to-be-abandoned data from the file.
+                truncateAtOffset = writer.offsetInFile
 
                 // Set the offset back to the beginning of the file.
                 try writer.seek(to: FileHeader.expectedEndOfHeaderInFile)
@@ -121,6 +144,12 @@ public final class CacheAdvance<T: Codable> {
             // If the application crashes between writing the header and writing the message data, we'll have lost the messages between the previous offsetInFileOfOldestMessage and the new offsetInFileOfOldestMessage.
             try header.updateOffsetInFileOfOldestMessage(to: offsetInFileOfOldestMessage)
 
+            // Truncate the file if it needs truncation before we write the next message, and after we update our header.
+            // If the application crashes between truncating this message data and writing the next message, our file will still be consistent.
+            if let truncateAtOffset = truncateAtOffset {
+                try writer.truncate(at: truncateAtOffset)
+            }
+
             // Let the reader know where the oldest message begins.
             reader.offsetInFileOfOldestMessage = offsetInFileOfOldestMessage
 

Sources/CacheAdvance/CacheReader.swift

@@ -42,7 +42,7 @@ final class CacheReader {
     }
 
     /// Returns the next encodable message, seeking to the beginning of the next message.
-    func nextEncodedMessage() throws -> Data? {
+    func nextEncodedMessage(previousReadWasEmpty: Bool = false) throws -> Data? {
         let startingOffset = offsetInFile
 
         guard startingOffset != offsetInFileAtEndOfNewestMessage else {
@@ -60,11 +60,17 @@ final class CacheReader {
             return message
 
         case .emptyRead:
+            guard !previousReadWasEmpty else {
+                // If the previous read was also empty, then the file has been corrupted.
+                // Two empty reads in a row means that offsetInFileAtEndOfNewestMessage is incorrect.
+                // This inconsistency likely is likely due to a crash occurring during a message write.
+                throw CacheAdvanceError.fileCorrupted
+            }
             // We know the next message is at the end of the file header. Let's seek to it.
             try reader.seek(to: FileHeader.expectedEndOfHeaderInFile)
 
             // We know there's a message to read now that we're at the start of the file.
-            return try nextEncodedMessage()
+            return try nextEncodedMessage(previousReadWasEmpty: true)
 
         case .invalidFormat:
             throw CacheAdvanceError.fileCorrupted

Tests/CacheAdvanceTests/CacheAdvanceTests.swift

@@ -96,6 +96,35 @@ final class CacheAdvanceTests: XCTestCase {
         XCTAssertEqual(messages, [])
     }
 
+  func test_messages_throwsFileCorruptedWhenOffsetInFileAtEndOfNewsetMessageOutOfSync() throws {
+      let randomHighValue: UInt64 = 10_1000
+      let header = try CacheHeaderHandle(
+        forReadingFrom: testFileLocation,
+        maximumBytes: randomHighValue,
+        overwritesOldMessages: true)
+      let cache = CacheAdvance<TestableMessage>(
+        fileURL: testFileLocation,
+        writer: try FileHandle(forWritingTo: testFileLocation),
+        reader: try CacheReader(forReadingFrom: testFileLocation),
+        header: try CacheHeaderHandle(
+            forReadingFrom: testFileLocation,
+            maximumBytes: header.maximumBytes,
+            overwritesOldMessages: header.overwritesOldMessages),
+        decoder: JSONDecoder(),
+        encoder: JSONEncoder())
+
+      // Make sure the header data is persisted before we read it as part of the `messages()` call below.
+      try header.synchronizeHeaderData()
+      // Our file is empty. Make the file corrupted by setting the offset at end of newest message to be further in the file.
+      // This should never happen, but past versions of this repo could lead to a file having this kind of inconsistency if a crash occurred at the wrong time.
+      try header.updateOffsetInFileAtEndOfNewestMessage(
+        to: FileHeader.expectedEndOfHeaderInFile + UInt64(MessageSpan.storageLength) + 1)
+
+      XCTAssertThrowsError(try cache.messages()) {
+          XCTAssertEqual($0 as? CacheAdvanceError, CacheAdvanceError.fileCorrupted)
+      }
+  }
+
     func test_isWritable_returnsTrueWhenStaticHeaderMetadataMatches() throws {
         let originalCache = try createCache(overwritesOldMessages: false)
         XCTAssertTrue(try originalCache.isWritable())