From b87b4f34f71e983966934737fec37d8b62e48a43 Mon Sep 17 00:00:00 2001 From: Dan Federman Date: Tue, 8 Mar 2022 11:31:28 -0800 Subject: [PATCH] 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 * Better comment Co-authored-by: Francisco Diaz * Document that value is random Co-authored-by: Steven Hepting Co-authored-by: Francisco Diaz --- CacheAdvance.podspec | 2 +- Sources/CacheAdvance/CacheAdvance.swift | 47 +++++++++++++++---- Sources/CacheAdvance/CacheReader.swift | 10 +++- .../CacheAdvanceTests/CacheAdvanceTests.swift | 29 ++++++++++++ 4 files changed, 76 insertions(+), 12 deletions(-) diff --git a/CacheAdvance.podspec b/CacheAdvance.podspec index 7a4ede94..2fac5f4e 100644 --- a/CacheAdvance.podspec +++ b/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' diff --git a/Sources/CacheAdvance/CacheAdvance.swift b/Sources/CacheAdvance/CacheAdvance.swift index bb870345..3581cc27 100644 --- a/Sources/CacheAdvance/CacheAdvance.swift +++ b/Sources/CacheAdvance/CacheAdvance.swift @@ -36,7 +36,7 @@ public final class CacheAdvance { /// - 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, @@ -44,12 +44,31 @@ public final class CacheAdvance { 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 { 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 { // 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 diff --git a/Sources/CacheAdvance/CacheReader.swift b/Sources/CacheAdvance/CacheReader.swift index 5ea9afbf..8d598c28 100644 --- a/Sources/CacheAdvance/CacheReader.swift +++ b/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 diff --git a/Tests/CacheAdvanceTests/CacheAdvanceTests.swift b/Tests/CacheAdvanceTests/CacheAdvanceTests.swift index 6820b838..f833393c 100644 --- a/Tests/CacheAdvanceTests/CacheAdvanceTests.swift +++ b/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( + 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())