Repository navigation
fix(fuzz): use the real reader settings keys in the fuzzer - #1730
Merged
Merged
Conversation
The fuzzer set most CharReaderBuilder settings under names with a
trailing underscore ("allowComments_", "strictRoot_", ...), which
newCharReader() does not read, and set failIfExtra twice. As a result
only failIfExtra and collectComments ever varied; every other reader
mode was stuck at its default.
Use the real key names, give the duplicate failIfExtra bit to skipBom,
and add an optional small stackLimit (bit 11, value from the upper 16
bits) so the depth-limit error paths are reachable. Abort if
CharReaderBuilder::validate() rejects the settings, so a misspelled key
fails loudly instead of silently disabling a mode.
The FuzzTest unit test only passed "{}", which is shorter than the
4-byte settings header and returned before touching any settings.
Exercise no settings, all settings, and a tiny stack limit.
In a local 2-minute libFuzzer run from test/data seeds, json_reader.cpp
edge coverage goes from 465 to 570 of 1996.
Coverage Report for CI Build 40342106111Coverage increased (+0.02%) to 89.926%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
 [Low impact] The PR appears safe to merge.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
src/test_lib_json/fuzz.cppset mostCharReaderBuildersettings under names with a trailing underscore (allowComments_,strictRoot_,rejectDupKeys_, …).newCharReader()only reads the names without the underscore, so those keys were ignored, andfailIfExtrawas also set twice. OnlyfailIfExtraandcollectCommentsever varied, which means OSS-Fuzz has been fuzzing the reader with every other mode left at its default.The existing
FuzzTest/fuzzDoesntCrashunit test didn't catch this: its input,"{}", is shorter than the 4-byte settings header, so the fuzzer returned before it reached the settings code.Fix
failIfExtrabit (6) now controlsskipBom.stackLimit, taken from the upper 16 bits, so the depth-limit error paths are reachable without huge inputs.abort()ifCharReaderBuilder::validate()rejects the settings, so a misspelled key crashes immediately instead of silently disabling a mode.fuzzDoesntCrashnow runs inputs with no settings, all settings, and a stack limit of 1.The proto fuzzer currently in OSS-Fuzz has the same misspelled keys; #1724 already fixes them in its copy.