Skip to content

Commit b3e3f8c

Browse files
Lukasathomasvl
authored andcommitted
Don't do characterwise-compares if not needed.
When attempting to locate a field number for a JSON document, some code paths will find themselves needing to strip `[` and `]` from a String. Right now they do this by grabbing `first` and `last` from the String and then comparing the `Character` to a `String` containing the given characters. This is a fairly heavyweight operation: it invokes unicode normalisation, among other things. This means that with specially crafted field names it is possible to cause us to spend a surprisingly long time normalizing strings unnecessarily. Given that we're looking for UTF-8 characters, and we know these strings will already be UTF-8, the easier fix is to just do the check on the UTF-8 view. The result is a vastly shortened runtime, taking the duration of a clusterfuzz test case (also submitted with this patch) down from 60s to 186ms: a 300x speedup. Note that this patch is not terribly _rigorous_: I haven't gone searching for other places where we do direct String comparisons that can be done on the UTF-8 view. That would likely be a productive line of enquiry for performance enhancement in the JSON codepath at the very least.
1 parent 1d45734 commit b3e3f8c

File tree

2 files changed

+4
-2
lines changed

2 files changed

+4
-2
lines changed

FuzzTesting/FailCases/clusterfuzz-testcase-FuzzJSON_release-5596804654694400

Lines changed: 2 additions & 0 deletions
Large diffs are not rendered by default.

Sources/SwiftProtobuf/JSONScanner.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1276,8 +1276,8 @@ internal struct JSONScanner {
12761276
return fieldNumber
12771277
}
12781278
}
1279-
if let first = fieldName.first, first == "[",
1280-
let last = fieldName.last, last == "]"
1279+
if let first = fieldName.utf8.first, first == UInt8(ascii: "["),
1280+
let last = fieldName.utf8.last, last == UInt8(ascii: "]")
12811281
{
12821282
fieldName.removeFirst()
12831283
fieldName.removeLast()

0 commit comments

Comments
 (0)