Add more test coverage of RESPTranslator

Motivation:

Diagnostics for why `.bulkString` parses might fail were weak, and edge cases fell through gaps in coverage were found.

Modifications:

Added new cases to `RESPTranslator.ParsingError` for `.bulkString` parsing with additional test coverage.

Result:

Users should have better diagnostics for bogus data or failed parsing state.

Author: Mordil

Sources/RediStack/RESP/RESPTranslator.swift

@@ -102,11 +102,15 @@ extension RESPTranslator {
     /// [https://www.gitlab.com/mordil/swift-redis-nio-client/issues](https://www.gitlab.com/mordil/swift-redis-nio-client/issues)
     public enum ParsingError: LocalizedError {
         case invalidToken
+        case invalidBulkStringSize
+        case bulkStringSizeMismatch
 
         /// See `LocalizedError.errorDescription`
         public var errorDescription: String? {
             switch self {
             case .invalidToken: return "Cannot parse RESP: Invalid Token"
+            case .invalidBulkStringSize: return "Cannot parse RESP Bulk String: Received invalid size."
+            case .bulkStringSizeMismatch: return "Cannot parse RESP Bulk String: Declared Size and Content Size do not match."
             }
         }
     }
@@ -133,7 +137,7 @@ extension RESPTranslator {
             result = .integer(value)
 
         case .dollar:
-            result = self.parseBulkString(from: &copy)
+            result = try self.parseBulkString(from: &copy)
             break
 
         case .asterisk:
@@ -187,8 +191,19 @@ extension RESPTranslator {
     }
 
     /// See [https://redis.io/topics/protocol#resp-bulk-strings](https://redis.io/topics/protocol#resp-bulk-strings)
-    internal func parseBulkString(from buffer: inout ByteBuffer) -> RESPValue? {
-        guard let size = self.parseInteger(from: &buffer) else { return nil }
+    internal func parseBulkString(from buffer: inout ByteBuffer) throws -> RESPValue? {
+        let startingReaderIndex = buffer.readerIndex
+        
+        guard let size = self.parseInteger(from: &buffer) else {
+            // if the reader index changed, that means that a valid string parse happened, but it's not a number.
+            guard startingReaderIndex == buffer.readerIndex else {
+                throw ParsingError.invalidBulkStringSize
+            }
+            return nil
+        }
+        
+        // only -1 is the only valid negative value for a size
+        guard size >= -1 else { throw ParsingError.invalidBulkStringSize }
 
         // Redis sends '$-1\r\n' to represent a null bulk string
         guard size > -1 else { return .null }
@@ -199,8 +214,10 @@ extension RESPTranslator {
         let expectedRemainingMessageSize = size + 2
         guard buffer.readableBytes >= expectedRemainingMessageSize else { return nil }
 
-        // sanity check assert that the position of the newline ending is the remaining size
-        assert(buffer.getBytes(at: expectedRemainingMessageSize + buffer.readerIndex - 1, length: 1)?.first == .newline)
+        // sanity check that the declared content size matches the actual size.
+        guard
+            buffer.viewBytes(at: buffer.readerIndex + expectedRemainingMessageSize - 1, length: 1)?.first == .newline
+        else { throw ParsingError.bulkStringSizeMismatch }
 
         // empty content bulk strings are different from null, and represented as .bulkString(nil)
         guard size > 0 else {

Tests/RediStackTests/RESPTranslatorTests.swift

@@ -104,6 +104,14 @@ fileprivate extension ByteBuffer {
 // MARK: Parse
 
 extension RESPTranslatorTests {
+    func testParsing_invalidToken() {
+        var buffer = self.allocator.buffer(capacity: 128)
+        buffer.writeString("!!!!")
+        XCTAssertThrowsError(try self.parser.parseBytes(from: &buffer)) { error in
+            XCTAssertEqual(error as? RESPTranslator.ParsingError, .invalidToken)
+        }
+    }
+    
     func testParsing_invalidSymbols() {
         let testRESP = "&3\r\n"
         var buffer = allocator.buffer(capacity: testRESP.count)
@@ -337,6 +345,33 @@ extension RESPTranslatorTests {
 // MARK: Bulk Strings
 
 extension RESPTranslatorTests {
+    func testParsing_bulkString_sizeMismatch() {
+        var buffer = self.allocator.buffer(capacity: 128)
+        buffer.writeString("$2\r\ntoo long\r\n")
+        buffer.mimicTokenParse()
+        XCTAssertThrowsError(try self.parser.parseBulkString(from: &buffer)) { error in
+            XCTAssertEqual(error as? RESPTranslator.ParsingError, .bulkStringSizeMismatch)
+        }
+    }
+    
+    func testParsing_bulkString_invalidNegativeSize() {
+        var buffer = self.allocator.buffer(capacity: 128)
+        buffer.writeString("$-4\r\nwhat\r\n")
+        buffer.mimicTokenParse()
+        XCTAssertThrowsError(try self.parser.parseBulkString(from: &buffer)) { error in
+            XCTAssertEqual(error as? RESPTranslator.ParsingError, .invalidBulkStringSize)
+        }
+    }
+    
+    func testParsing_bulkString_SizeIsNaN() {
+        var buffer = self.allocator.buffer(capacity: 128)
+        buffer.writeString("$FOO\r\nwhat\r\n")
+        buffer.mimicTokenParse()
+        XCTAssertThrowsError(try self.parser.parseBulkString(from: &buffer)) { error in
+            XCTAssertEqual(error as? RESPTranslator.ParsingError, .invalidBulkStringSize)
+        }
+    }
+    
     func testParsing_bulkString_missingEndings() {
         for message in ["$6", "$6\r\n", "$6\r\nabcdef", "$0\r\n"] {
             XCTAssertNil(bulkStringParseTest(inputRESP: message))
@@ -374,11 +409,11 @@ extension RESPTranslatorTests {
         buffer.writeString(testString)
 
         buffer.mimicTokenParse()
-        let first = parser.parseBulkString(from: &buffer)
+        let first = try? parser.parseBulkString(from: &buffer)
         XCTAssertEqual(buffer.readerIndex, 6) // position of the 2nd '$'
 
         buffer.mimicTokenParse()
-        let second = parser.parseBulkString(from: &buffer)
+        let second = try? parser.parseBulkString(from: &buffer)
         XCTAssertEqual(buffer.readerIndex, 17)
 
         XCTAssertEqual(first?.string, "")
@@ -395,7 +430,7 @@ extension RESPTranslatorTests {
 
         buffer.mimicTokenParse()
 
-        guard let result = parser.parseBulkString(from: &buffer) else { return nil }
+        guard let result = try? parser.parseBulkString(from: &buffer) else { return nil }
         return result
     }
 }

Tests/RediStackTests/XCTestManifests.swift

@@ -14,10 +14,13 @@ extension RESPTranslatorTests {
         ("testParsing_array_withNullElements", testParsing_array_withNullElements),
         ("testParsing_arrays_chunked", testParsing_arrays_chunked),
         ("testParsing_arrays", testParsing_arrays),
+        ("testParsing_bulkString_invalidNegativeSize", testParsing_bulkString_invalidNegativeSize),
         ("testParsing_bulkString_missingEndings", testParsing_bulkString_missingEndings),
         ("testParsing_bulkString_null", testParsing_bulkString_null),
         ("testParsing_bulkString_rawBytes", testParsing_bulkString_rawBytes),
         ("testParsing_bulkString_recursively", testParsing_bulkString_recursively),
+        ("testParsing_bulkString_SizeIsNaN", testParsing_bulkString_SizeIsNaN),
+        ("testParsing_bulkString_sizeMismatch", testParsing_bulkString_sizeMismatch),
         ("testParsing_bulkString_withContent", testParsing_bulkString_withContent),
         ("testParsing_bulkString_withNoSize", testParsing_bulkString_withNoSize),
         ("testParsing_bulkStrings_chunked", testParsing_bulkStrings_chunked),
@@ -29,6 +32,7 @@ extension RESPTranslatorTests {
         ("testParsing_integer_withAllBytes", testParsing_integer_withAllBytes),
         ("testParsing_integer", testParsing_integer),
         ("testParsing_invalidSymbols", testParsing_invalidSymbols),
+        ("testParsing_invalidToken", testParsing_invalidToken),
         ("testParsing_simpleString_chunked", testParsing_simpleString_chunked),
         ("testParsing_simpleString_handlesRecursion", testParsing_simpleString_handlesRecursion),
         ("testParsing_simpleString_missingNewline", testParsing_simpleString_missingNewline),