From c69c730aace979fffe907426f470ed41b5826f56 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Fri, 18 Aug 2023 14:23:18 -0400 Subject: [PATCH 1/3] Use distinct errors for errors during delimited parsing. This allows the caller to tell the difference between when a message *within* the delimited stream was bad vs. when the framing on stream itself was bad. --- .../SwiftProtobuf/AsyncMessageSequence.swift | 14 +-- Sources/SwiftProtobuf/BinaryDelimited.swift | 6 +- .../Test_AsyncMessageSequence.swift | 12 +- .../Test_BinaryDelimited.swift | 112 +++++++++++++++--- 4 files changed, 111 insertions(+), 33 deletions(-) diff --git a/Sources/SwiftProtobuf/AsyncMessageSequence.swift b/Sources/SwiftProtobuf/AsyncMessageSequence.swift index d49d2c398..d65da443b 100644 --- a/Sources/SwiftProtobuf/AsyncMessageSequence.swift +++ b/Sources/SwiftProtobuf/AsyncMessageSequence.swift @@ -30,9 +30,9 @@ extension AsyncSequence where Element == UInt8 { /// - Returns: An asynchronous sequence of messages read from the `AsyncSequence` of bytes. /// - Throws: `BinaryDecodingError` if decoding fails, throws /// `BinaryDelimited.Error` for some reading errors, - /// `BinaryDecodingError.truncated` if the stream ends before fully decoding a + /// `BinaryDelimited.Error.truncated` if the stream ends before fully decoding a /// message or a delimiter, - /// `BinaryDecodingError.malformedProtobuf`if a delimiter could not be read and + /// `BinaryDelimited.Error.malformedLength`if a delimiter could not be read and /// `BinaryDecodingError.tooLarge` if a size delimiter of 2GB or greater is found. @inlinable public func binaryProtobufDelimitedMessages( @@ -80,9 +80,9 @@ public struct AsyncMessageSequence< /// - Returns: An asynchronous sequence of messages read from the `AsyncSequence` of bytes. /// - Throws: `BinaryDecodingError` if decoding fails, throws /// `BinaryDelimited.Error` for some reading errors, - /// `BinaryDecodingError.truncated` if the stream ends before fully decoding a + /// `BinaryDelimited.Error.truncated` if the stream ends before fully decoding a /// message or a delimiter, - /// `BinaryDecodingError.malformedProtobuf`if a delimiter could not be read and + /// `BinaryDelimited.Error.malformedLength`if a delimiter could not be read and /// `BinaryDecodingError.tooLarge` if a size delimiter of 2GB or greater is found. public init( base: Base, @@ -130,7 +130,7 @@ public struct AsyncMessageSequence< shift += UInt64(7) if shift > 35 { iterator = nil - throw BinaryDecodingError.malformedProtobuf + throw BinaryDelimited.Error.malformedLength } if (byte & 0x80 == 0) { return messageSize @@ -139,7 +139,7 @@ public struct AsyncMessageSequence< if (shift > 0) { // The stream has ended inside a varint. iterator = nil - throw BinaryDecodingError.truncated + throw BinaryDelimited.Error.truncated } return nil // End of stream reached. } @@ -161,7 +161,7 @@ public struct AsyncMessageSequence< guard let byte = try await iterator?.next() else { // The iterator hit the end, but the chunk wasn't filled, so the full // payload wasn't read. - throw BinaryDecodingError.truncated + throw BinaryDelimited.Error.truncated } chunk[consumedBytes] = byte consumedBytes += 1 diff --git a/Sources/SwiftProtobuf/BinaryDelimited.swift b/Sources/SwiftProtobuf/BinaryDelimited.swift index c9d975c52..edec1a9a1 100644 --- a/Sources/SwiftProtobuf/BinaryDelimited.swift +++ b/Sources/SwiftProtobuf/BinaryDelimited.swift @@ -32,6 +32,10 @@ public enum BinaryDelimited { /// Messages are limited by the protobuf spec to 2GB; when decoding, if the /// length says the payload is over 2GB, this error is raised. case tooLarge + + /// While attempting to read the length of a message on the stream, the + /// bytes were malformed for the protobuf format. + case malformedLength } /// Serialize a single size-delimited message to the given stream. Delimited @@ -246,7 +250,7 @@ internal func decodeVarint(_ stream: InputStream) throws -> UInt64 { } shift += 7 if shift > 63 { - throw BinaryDecodingError.malformedProtobuf + throw BinaryDelimited.Error.malformedLength } } } diff --git a/Tests/SwiftProtobufTests/Test_AsyncMessageSequence.swift b/Tests/SwiftProtobufTests/Test_AsyncMessageSequence.swift index 91a27ac47..8f2727363 100644 --- a/Tests/SwiftProtobufTests/Test_AsyncMessageSequence.swift +++ b/Tests/SwiftProtobufTests/Test_AsyncMessageSequence.swift @@ -126,11 +126,11 @@ final class Test_AsyncMessageSequence: XCTestCase { XCTFail("Shouldn't have returned a value for an empty stream.") } } catch { - if error as! BinaryDecodingError == .truncated { + if error as! BinaryDelimited.Error == .truncated { truncatedThrown = true } } - XCTAssertTrue(truncatedThrown, "Should throw a BinaryDecodingError.truncated") + XCTAssertTrue(truncatedThrown, "Should throw a BinaryDelimited.Error.truncated") } // Single varint describing a 2GB message @@ -161,11 +161,11 @@ final class Test_AsyncMessageSequence: XCTestCase { XCTFail("Shouldn't have returned a value for an empty stream.") } } catch { - if error as! BinaryDecodingError == .truncated { + if error as! BinaryDelimited.Error == .truncated { truncatedThrown = true } } - XCTAssertTrue(truncatedThrown, "Should throw a BinaryDecodingError.truncated") + XCTAssertTrue(truncatedThrown, "Should throw a BinaryDelimited.Error.truncated") } // Stream with a valid varint and message, but the following varint is truncated @@ -193,11 +193,11 @@ final class Test_AsyncMessageSequence: XCTestCase { } XCTAssertEqual(count, 1, "One message should be deserialized") } catch { - if error as! BinaryDecodingError == .truncated { + if error as! BinaryDelimited.Error == .truncated { truncatedThrown = true } } - XCTAssertTrue(truncatedThrown, "Should throw a BinaryDecodingError.truncated") + XCTAssertTrue(truncatedThrown, "Should throw a BinaryDelimited.Error.truncated") } fileprivate func asyncByteStream(bytes: [UInt8]) -> AsyncStream { diff --git a/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift b/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift index c74513665..8c34f1b9a 100644 --- a/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift +++ b/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift @@ -12,12 +12,98 @@ import Foundation import XCTest import SwiftProtobuf +fileprivate func openInputStream(_ bytes: [UInt8]) -> InputStream { + let istream = InputStream(data: Data(bytes)) + istream.open() + return istream +} + class Test_BinaryDelimited: XCTestCase { - func testEverything() { - // Don't need to test encode/decode since there are plenty of tests specific to that, - // just test the delimited behaviors. + /// Helper to assert the next message read matches and expected one. + func assertParse(expected: M, onStream istream: InputStream) { + do { + let msg = try BinaryDelimited.parse( + messageType: M.self, + from: istream) + XCTAssertEqual(msg, expected) + } catch let e { + XCTFail("Unexpected failure: \(e)") + } + } + + /// Helper to assert we're at the end of the stream. + /// + /// `hasBytesAvailable` is documented as maybe returning True and a read + /// has to happen to really know if ones at the end. This is especially + /// true with file based streams. + func assertParseFails(atEndOfStream istream: InputStream) { + XCTAssertThrowsError(try BinaryDelimited.parse(messageType: SwiftProtoTesting_TestAllTypes.self, + from: istream)) { error in + XCTAssertEqual(error as? BinaryDelimited.Error, BinaryDelimited.Error.truncated) + } + } + + func assertParsing(failsWithTruncatedStream istream: InputStream) { + XCTAssertThrowsError(try BinaryDelimited.parse(messageType: SwiftProtoTesting_TestAllTypes.self, + from: istream)) { error in + XCTAssertEqual(error as? BinaryDelimited.Error, BinaryDelimited.Error.truncated) + } + } + + func testNoData() { + let istream = openInputStream([]) + + assertParseFails(atEndOfStream: istream) + } + + func testZeroLengthMessage() { + let istream = openInputStream([0]) + + assertParse(expected: SwiftProtoTesting_TestAllTypes(), onStream: istream) + + assertParseFails(atEndOfStream: istream) + } + + func testNoDataForMessage() { + let istream = openInputStream([0x96, 0x01]) + + // Length will be read, then the no data for the message, so .truncated. + assertParsing(failsWithTruncatedStream: istream) + } + func testNotEnoughDataForMessage() { + let istream = openInputStream([0x96, 0x01, 0x01, 0x02, 0x03]) + + // Length will be read, but not enought data, so .truncated + assertParsing(failsWithTruncatedStream: istream) + } + + func testTruncatedLength() { + let istream = openInputStream([0x96]) // Needs something like `, 0x01` + + assertParsing(failsWithTruncatedStream: istream) + } + + func testTooLarge() { + let istream = openInputStream([0x80, 0x80, 0x80, 0x80, 0x08]) // 2GB + + XCTAssertThrowsError(try BinaryDelimited.parse(messageType: SwiftProtoTesting_TestAllTypes.self, + from: istream)) { error in + XCTAssertEqual(error as? BinaryDelimited.Error, BinaryDelimited.Error.tooLarge) + } + } + + func testOverEncodedLength() { + let istream = openInputStream([0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80 ,0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x08]) + + XCTAssertThrowsError(try BinaryDelimited.parse(messageType: SwiftProtoTesting_TestAllTypes.self, + from: istream)) { error in + XCTAssertEqual(error as? BinaryDelimited.Error, BinaryDelimited.Error.malformedLength) + } + } + + func testTwoMessages() { let stream1 = OutputStream.toMemory() stream1.open() @@ -48,27 +134,15 @@ class Test_BinaryDelimited: XCTestCase { let stream2 = InputStream(data: data) stream2.open() + // Test using `merge` var msg1a = SwiftProtoTesting_TestAllTypes() XCTAssertNoThrow(try BinaryDelimited.merge(into: &msg1a, from: stream2)) XCTAssertEqual(msg1, msg1a) - do { - let msg2a = try BinaryDelimited.parse( - messageType: SwiftProtoTesting_TestPackedTypes.self, - from: stream2) - XCTAssertEqual(msg2, msg2a) - } catch let e { - XCTFail("Unexpected failure: \(e)") - } + // Test using `parse` + assertParse(expected: msg2, onStream: stream2) - do { - _ = try BinaryDelimited.parse(messageType: SwiftProtoTesting_TestAllTypes.self, from: stream2) - XCTFail("Should not have gotten here") - } catch BinaryDelimited.Error.truncated { - // Nothing, this is what we expect since there is nothing left to read. - } catch let e { - XCTFail("Unexpected failure: \(e)") - } + assertParseFails(atEndOfStream: stream2) } } From 02df4762ad4f62e5369e6d810e6f673242c0ace7 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Mon, 21 Aug 2023 10:14:49 -0400 Subject: [PATCH 2/3] Add a new "error" for no bytes available. `InputStream` documents that `hasBytesAvailable` might return `True` when a `read` is needed to determine the real status. This case caused some attempts to use the delimited api then fall into seeing "end" as an error because it caused folks to see `.truncated` without really knowing nothing was there to read. This provide a distinct case for this condition so callers then have the option to gracefully handle reaching the end. Closed #1410 --- Sources/SwiftProtobuf/BinaryDelimited.swift | 36 +++++++++++++------ .../Test_BinaryDelimited.swift | 2 +- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/Sources/SwiftProtobuf/BinaryDelimited.swift b/Sources/SwiftProtobuf/BinaryDelimited.swift index edec1a9a1..43cea1187 100644 --- a/Sources/SwiftProtobuf/BinaryDelimited.swift +++ b/Sources/SwiftProtobuf/BinaryDelimited.swift @@ -36,6 +36,15 @@ public enum BinaryDelimited { /// While attempting to read the length of a message on the stream, the /// bytes were malformed for the protobuf format. case malformedLength + + /// This isn't really an "error". `InputStream` documents that + /// `hasBytesAvailable` _may_ return `True` if a read is needed to + /// determine if there really are bytes available. So this "error" is throw + /// when a `parse` or `merge` fails because there were no bytes available. + /// If this is rasied, the callers should decides via what ever other means + /// are correct if the stream has completely ended or if more bytes might + /// eventually show up. + case noBytesAvailable } /// Serialize a single size-delimited message to the given stream. Delimited @@ -226,24 +235,31 @@ internal func decodeVarint(_ stream: InputStream) throws -> UInt64 { let readBuffer = UnsafeMutablePointer.allocate(capacity: 1) defer { readBuffer.deallocate() } - func nextByte() throws -> UInt8 { + func nextByte() throws -> UInt8? { let bytesRead = stream.read(readBuffer, maxLength: 1) - if bytesRead != 1 { - if bytesRead == -1 { - if let streamError = stream.streamError { - throw streamError - } - throw BinaryDelimited.Error.unknownStreamError + switch bytesRead { + case 1: + return readBuffer[0] + case 0: + return nil + default: + precondition(bytesRead == -1) + if let streamError = stream.streamError { + throw streamError } - throw BinaryDelimited.Error.truncated + throw BinaryDelimited.Error.unknownStreamError } - return readBuffer[0] } var value: UInt64 = 0 var shift: UInt64 = 0 while true { - let c = try nextByte() + guard let c = try nextByte() else { + if shift == 0 { + throw BinaryDelimited.Error.noBytesAvailable + } + throw BinaryDelimited.Error.truncated + } value |= UInt64(c & 0x7f) << shift if c & 0x80 == 0 { return value diff --git a/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift b/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift index 8c34f1b9a..dc23d6003 100644 --- a/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift +++ b/Tests/SwiftProtobufTests/Test_BinaryDelimited.swift @@ -40,7 +40,7 @@ class Test_BinaryDelimited: XCTestCase { func assertParseFails(atEndOfStream istream: InputStream) { XCTAssertThrowsError(try BinaryDelimited.parse(messageType: SwiftProtoTesting_TestAllTypes.self, from: istream)) { error in - XCTAssertEqual(error as? BinaryDelimited.Error, BinaryDelimited.Error.truncated) + XCTAssertEqual(error as? BinaryDelimited.Error, BinaryDelimited.Error.noBytesAvailable) } } From 27414bb0c91bd416be78939af1005280c318a364 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Thu, 24 Aug 2023 10:53:32 -0400 Subject: [PATCH 3/3] Tweak the docs on the errors throw. Don't call out too specific of things and instead just mention the Errors types that are possible to avoid being overly specific and leading folks in the wrong direction. --- .../SwiftProtobuf/AsyncMessageSequence.swift | 18 ++++++------------ Sources/SwiftProtobuf/BinaryDelimited.swift | 4 ++-- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/Sources/SwiftProtobuf/AsyncMessageSequence.swift b/Sources/SwiftProtobuf/AsyncMessageSequence.swift index d65da443b..3c88aae75 100644 --- a/Sources/SwiftProtobuf/AsyncMessageSequence.swift +++ b/Sources/SwiftProtobuf/AsyncMessageSequence.swift @@ -28,12 +28,9 @@ extension AsyncSequence where Element == UInt8 { /// `BinaryDecodingError.missingRequiredFields` will be thrown. /// - options: The BinaryDecodingOptions to use. /// - Returns: An asynchronous sequence of messages read from the `AsyncSequence` of bytes. - /// - Throws: `BinaryDecodingError` if decoding fails, throws - /// `BinaryDelimited.Error` for some reading errors, - /// `BinaryDelimited.Error.truncated` if the stream ends before fully decoding a - /// message or a delimiter, - /// `BinaryDelimited.Error.malformedLength`if a delimiter could not be read and - /// `BinaryDecodingError.tooLarge` if a size delimiter of 2GB or greater is found. + /// - Throws: `BinaryDelimited.Error` for errors in the framing of the messages + /// in the sequence, `BinaryDecodingError` for errors while decoding + /// messages. @inlinable public func binaryProtobufDelimitedMessages( of messageType: M.Type = M.self, @@ -78,12 +75,9 @@ public struct AsyncMessageSequence< /// `BinaryDecodingError.missingRequiredFields` will be thrown. /// - options: The BinaryDecodingOptions to use. /// - Returns: An asynchronous sequence of messages read from the `AsyncSequence` of bytes. - /// - Throws: `BinaryDecodingError` if decoding fails, throws - /// `BinaryDelimited.Error` for some reading errors, - /// `BinaryDelimited.Error.truncated` if the stream ends before fully decoding a - /// message or a delimiter, - /// `BinaryDelimited.Error.malformedLength`if a delimiter could not be read and - /// `BinaryDecodingError.tooLarge` if a size delimiter of 2GB or greater is found. + /// - Throws: `BinaryDelimited.Error` for errors in the framing of the messages + /// in the sequence, `BinaryDecodingError` for errors while decoding + /// messages. public init( base: Base, extensions: ExtensionMap? = nil, diff --git a/Sources/SwiftProtobuf/BinaryDelimited.swift b/Sources/SwiftProtobuf/BinaryDelimited.swift index 43cea1187..dd7a6ce0e 100644 --- a/Sources/SwiftProtobuf/BinaryDelimited.swift +++ b/Sources/SwiftProtobuf/BinaryDelimited.swift @@ -123,7 +123,7 @@ public enum BinaryDelimited { /// - Returns: The message read. /// - Throws: `BinaryDecodingError` if decoding fails, throws /// `BinaryDelimited.Error` for some reading errors, and the - /// underlying InputStream.streamError for a stream error. + /// underlying `InputStream.streamError` for a stream error. public static func parse( messageType: M.Type, from stream: InputStream, @@ -164,7 +164,7 @@ public enum BinaryDelimited { /// - options: The BinaryDecodingOptions to use. /// - Throws: `BinaryDecodingError` if decoding fails, throws /// `BinaryDelimited.Error` for some reading errors, and the - /// underlying InputStream.streamError for a stream error. + /// underlying `InputStream.streamError` for a stream error. public static func merge( into message: inout M, from stream: InputStream,