Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881583002:
[fuzzer] Add input validation in the beginning of the parser fuzz target. (Closed)
Description[fuzzer] Add input validation in the beginning of the parser fuzz target.
Non-printable characters do not make sense.
Inputs with non balanced brackets are mostly useless as well.
This validation function makes the fuzzer 15-20x faster.
Also use -only_ascii=1 option of libFuzzer:
https://codereview.chromium.org/2875933003
BUG=chromium:584819
Review-Url: https://codereview.chromium.org/2881583002
Cr-Commit-Position: refs/heads/master@{#45367}
Committed: https://chromium.googlesource.com/v8/v8/+/96628339311653f8b05e7d9658af1ae681b26bea
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix coding style and apply git cl format #Messages
Total messages: 17 (7 generated)
Description was changed from ========== [fuzzer] Add input validation in the beginning of the fuzz target. Non-printable characters do not make sense. Inputs with non balanced brackets are mostly useless as well. This validation function makes the fuzzer 15-20x faster. BUG= ========== to ========== [fuzzer] Add input validation in the beginning of the parser fuzz target. Non-printable characters do not make sense. Inputs with non balanced brackets are mostly useless as well. This validation function makes the fuzzer 15-20x faster. Also use -only_ascii=1 option of libFuzzer: https://codereview.chromium.org/2875933003 BUG=584819 ==========
mmoroz@chromium.org changed reviewers: + marja@chromium.org, mbarbella@chromium.org
On 2017/05/12 09:31:08, mmoroz wrote: > mailto:mmoroz@chromium.org changed reviewers: > + mailto:marja@chromium.org, mailto:mbarbella@chromium.org Please take a look, this is a tiny change :)
q: is the current parser fuzzer producing any interesting reports, ie is it ok to fully repurpose it?
On 2017/05/16 12:30:12, marja wrote: > q: is the current parser fuzzer producing any interesting reports, ie is it ok > to fully repurpose it? I do not run it locally since the electricity outage last week. Looking at the input I've gotten before then, they seem to have potential. At least, they are better than binary trash that is being rejected by parser anyway. I also think that it makes sense to use some -max_len limit. Maybe 48 or 64? I'll add it to https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/BUILD.gn?q=est...
On 2017/05/16 13:09:21, mmoroz wrote: > On 2017/05/16 12:30:12, marja wrote: > > q: is the current parser fuzzer producing any interesting reports, ie is it ok > > to fully repurpose it? > > I do not run it locally since the electricity outage last week. Looking at the > input I've gotten before then, they seem to have potential. At least, they are > better than binary trash that is being rejected by parser anyway. > > I also think that it makes sense to use some -max_len limit. Maybe 48 or 64? > I'll add it to > https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/BUILD.gn?q=est... So is the plan to fully discard the current parser fuzzer and make it produce different kind of examples? 48 / 64 sound really short - they're probably good for the minimal examples I showed you, but there are other interesting cases which don't fit there.
On 2017/05/16 13:12:12, marja wrote: > On 2017/05/16 13:09:21, mmoroz wrote: > > On 2017/05/16 12:30:12, marja wrote: > > > q: is the current parser fuzzer producing any interesting reports, ie is it > ok > > > to fully repurpose it? > > > > I do not run it locally since the electricity outage last week. Looking at the > > input I've gotten before then, they seem to have potential. At least, they are > > better than binary trash that is being rejected by parser anyway. > > > > I also think that it makes sense to use some -max_len limit. Maybe 48 or 64? > > I'll add it to > > > https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/BUILD.gn?q=est... > > So is the plan to fully discard the current parser fuzzer and make it produce > different kind of examples? > > 48 / 64 sound really short - they're probably good for the minimal examples I > showed you, but there are other interesting cases which don't fit there. I wouldn't say "fully discard", but would say "iteratively upgrade" the fuzzer to generate better testcases :) Current upgrade resolves two issues: a) bad characters in the input b) non balanced braces These are low hanging fruits, but anyway it seems to be a movement in right direction. As a side effect, the fuzzer becomes much faster, as we reject bad inputs before starting all the V8 stuff. Thanks for clarification on max_len, I won't change it then.
On 2017/05/16 14:14:56, mmoroz wrote: > On 2017/05/16 13:12:12, marja wrote: > > On 2017/05/16 13:09:21, mmoroz wrote: > > > On 2017/05/16 12:30:12, marja wrote: > > > > q: is the current parser fuzzer producing any interesting reports, ie is > it > > ok > > > > to fully repurpose it? > > > > > > I do not run it locally since the electricity outage last week. Looking at > the > > > input I've gotten before then, they seem to have potential. At least, they > are > > > better than binary trash that is being rejected by parser anyway. > > > > > > I also think that it makes sense to use some -max_len limit. Maybe 48 or 64? > > > I'll add it to > > > > > > https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/BUILD.gn?q=est... > > > > So is the plan to fully discard the current parser fuzzer and make it produce > > different kind of examples? > > > > 48 / 64 sound really short - they're probably good for the minimal examples I > > showed you, but there are other interesting cases which don't fit there. > > I wouldn't say "fully discard", but would say "iteratively upgrade" the fuzzer > to generate better testcases :) > > Current upgrade resolves two issues: > a) bad characters in the input > b) non balanced braces > > These are low hanging fruits, but anyway it seems to be a movement in right17923 > direction. As a side effect, the fuzzer becomes much faster, as we reject bad > inputs before starting all the V8 stuff. > > Thanks for clarification on max_len, I won't change it then. I also checked how the coverage changes if we land this. Existing fuzzer has shown 16933 edges on a minimized corpus from the cloud If I replace all bad characters with spaces (' '), that corpus gives 16407 edges If I add new corpus files (generated during 2-3 days on my desktop), it becomes 17646 I think that we lose a bit of "trashy" coverage, that happens due to the parser bailing out on invalid inputs, but we get a good increment of "good" coverage that is produced by somewhat valid-looking inputs. PTAL
lgtm https://codereview.chromium.org/2881583002/diff/1/test/fuzzer/parser.cc File test/fuzzer/parser.cc (right): https://codereview.chromium.org/2881583002/diff/1/test/fuzzer/parser.cc#newco... test/fuzzer/parser.cc:26: if (!(std::isspace(ptr[i]) || std::isprint(ptr[i]))) Coding style nit: if the body is on the next line, this needs { } https://codereview.chromium.org/2881583002/diff/1/test/fuzzer/parser.cc#newco... test/fuzzer/parser.cc:60: if (!IsValidInput(data, size)) ditto
Description was changed from ========== [fuzzer] Add input validation in the beginning of the parser fuzz target. Non-printable characters do not make sense. Inputs with non balanced brackets are mostly useless as well. This validation function makes the fuzzer 15-20x faster. Also use -only_ascii=1 option of libFuzzer: https://codereview.chromium.org/2875933003 BUG=584819 ========== to ========== [fuzzer] Add input validation in the beginning of the parser fuzz target. Non-printable characters do not make sense. Inputs with non balanced brackets are mostly useless as well. This validation function makes the fuzzer 15-20x faster. Also use -only_ascii=1 option of libFuzzer: https://codereview.chromium.org/2875933003 BUG=chromium:584819 ==========
Thanks Marja! https://codereview.chromium.org/2881583002/diff/1/test/fuzzer/parser.cc File test/fuzzer/parser.cc (right): https://codereview.chromium.org/2881583002/diff/1/test/fuzzer/parser.cc#newco... test/fuzzer/parser.cc:26: if (!(std::isspace(ptr[i]) || std::isprint(ptr[i]))) On 2017/05/17 08:56:20, marja wrote: > Coding style nit: if the body is on the next line, this needs { } Done. https://codereview.chromium.org/2881583002/diff/1/test/fuzzer/parser.cc#newco... test/fuzzer/parser.cc:60: if (!IsValidInput(data, size)) On 2017/05/17 08:56:20, marja wrote: > ditto Done.
The CQ bit was checked by mmoroz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org Link to the patchset: https://codereview.chromium.org/2881583002/#ps20001 (title: "Fix coding style and apply git cl format")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495015179742440,
"parent_rev": "9798469980a65c2e66b14553d484e6e070180fc5", "commit_rev":
"96628339311653f8b05e7d9658af1ae681b26bea"}
Message was sent while issue was closed.
Description was changed from ========== [fuzzer] Add input validation in the beginning of the parser fuzz target. Non-printable characters do not make sense. Inputs with non balanced brackets are mostly useless as well. This validation function makes the fuzzer 15-20x faster. Also use -only_ascii=1 option of libFuzzer: https://codereview.chromium.org/2875933003 BUG=chromium:584819 ========== to ========== [fuzzer] Add input validation in the beginning of the parser fuzz target. Non-printable characters do not make sense. Inputs with non balanced brackets are mostly useless as well. This validation function makes the fuzzer 15-20x faster. Also use -only_ascii=1 option of libFuzzer: https://codereview.chromium.org/2875933003 BUG=chromium:584819 Review-Url: https://codereview.chromium.org/2881583002 Cr-Commit-Position: refs/heads/master@{#45367} Committed: https://chromium.googlesource.com/v8/v8/+/96628339311653f8b05e7d9658af1ae681b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/96628339311653f8b05e7d9658af1ae681b... |
