Skip to content

Commit

Permalink
Add a fail out if raw field numbers are too long.
Browse files Browse the repository at this point in the history
Follow up from the comment on adding unknown field skipping.

Since there is a max field length, we can fail out if the field number is too
many digits.

Added tests to also cover this.
  • Loading branch information
thomasvl committed May 2, 2024
1 parent 1804a61 commit 0d5414d
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 2 deletions.
30 changes: 28 additions & 2 deletions Sources/SwiftProtobuf/TextFormatScanner.swift
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ private let asciiLowerY = UInt8(ascii: "y")
private let asciiLowerZ = UInt8(ascii: "z")
private let asciiUpperZ = UInt8(ascii: "Z")

// https://protobuf.dev/programming-guides/proto2/#assigning
// Fields can be between 1 and 536,870,911. So we can stop parsing
// a raw number if we go over this (it also avoid rollover).
private let maxFieldNumLength: Int = 9

private func fromHexDigit(_ c: UInt8) -> UInt8? {
if c >= asciiZero && c <= asciiNine {
return c - asciiZero
Expand Down Expand Up @@ -1094,9 +1099,26 @@ internal struct TextFormatScanner {
}
throw TextFormatDecodingError.unknownField
case asciiLowerA...asciiLowerZ,
asciiUpperA...asciiUpperZ,
asciiOne...asciiNine: // a...z, A...Z, 1...9
asciiUpperA...asciiUpperZ: // a...z, A...Z
return parseIdentifier()
case asciiOne...asciiNine: // 1...9 (field numbers are 123, not 0123)
let start = p
p += 1
while p != end {
let c = p[0]
if c < asciiZero || c > asciiNine {
break
}
p += 1
if p - start > maxFieldNumLength {
throw TextFormatDecodingError.malformedText
}
}
let buff = UnsafeRawBufferPointer(start: start, count: p - start)
skipWhitespace()
let s = utf8ToString(bytes: buff.baseAddress!, count: buff.count)
// Safe, can't be invalid UTF-8 given the input.
return s!
default:
throw TextFormatDecodingError.malformedText
}
Expand Down Expand Up @@ -1151,6 +1173,7 @@ internal struct TextFormatScanner {
// Unknown field name
break
case asciiOne...asciiNine: // 1-9 (field numbers are 123, not 0123)
let start = p
var fieldNum = Int(c) - Int(asciiZero)
p += 1
while p != end {
Expand All @@ -1161,6 +1184,9 @@ internal struct TextFormatScanner {
break
}
p += 1
if p - start > maxFieldNumLength {
throw TextFormatDecodingError.malformedText
}
}
skipWhitespace()
if names.names(for: fieldNum) != nil {
Expand Down
47 changes: 47 additions & 0 deletions Tests/SwiftProtobufTests/Test_TextFormatDecodingOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,53 @@ final class Test_TextFormatDecodingOptions: XCTestCase {
XCTAssertEqual(expected, msg)
}

func testIgnoreUnknown_fieldnumTooBig() {
let expected = SwiftProtoTesting_TestAllTypes.with {
$0.optionalInt32 = 1
$0.optionalUint32 = 2
}
var options = TextFormatDecodingOptions()
options.ignoreUnknownFields = true

// The max field number is 536,870,911, so anything that takes more digits, should
// fail as malformed.

let testCases: [(field: String, parses: Bool)] = [
("536870911", true),
("1536870911", false)
]

for testCase in testCases {
let text = """
optional_int32: 1 # !!! real field
# Unknown field that's a message to test parsing of field numbers
# nested within a unknown message.
does_not_exist {
\(testCase.field): 1
}
optional_uint32: 2 # !!! real field
"""

do {
let msg = try SwiftProtoTesting_TestAllTypes(textFormatString: text,
options: options)
// If we get here, it should be the expected message.
XCTAssertTrue(testCase.parses)
XCTAssertEqual(msg, expected)
} catch TextFormatDecodingError.malformedText {
if testCase.parses {
XCTFail("Unexpected malformedText - input: \(testCase.field)")
} else {
// Nothing, was the expected error
}
} catch {
XCTFail("Unexpected error: \(error) - input: \(testCase.field)")
}
}
}

func testIgnoreUnknownWithMessageDepthLimit() {
let textInput = "a: { a: { i: 1 } }"

Expand Down
25 changes: 25 additions & 0 deletions Tests/SwiftProtobufTests/Test_TextFormat_Unknown.swift
Original file line number Diff line number Diff line change
Expand Up @@ -386,4 +386,29 @@ final class Test_TextFormat_Unknown: XCTestCase, PBTestHelpers {
let textWithoutUnknowns = msg.textFormatString(options: encodeWithoutUnknowns)
XCTAssertEqual(textWithoutUnknowns, "")
}

func test_unknown_fieldnum_too_big() {
// The max field number is 536,870,911, so anything that takes more digits, should
// fail as malformed.

var opts = TextFormatDecodingOptions()
opts.ignoreUnknownFields = true

// Max value, will pass becuase of ignoring unknowns.
do {
let _ = try MessageTestType(textFormatString: "536870911: 1", options: opts)
} catch {
XCTFail("Unexpected error: \(error)")
}

// One more digit, should fail as malformed
do {
let _ = try MessageTestType(textFormatString: "1536870911: 1", options: opts)
XCTFail("Shouldn't get here")
} catch TextFormatDecodingError.malformedText {
// This is what should have happened.
} catch {
XCTFail("Unexpected error: \(error)")
}
}
}

0 comments on commit 0d5414d

Please sign in to comment.