Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449323004:
json parser/writer correctness fuzzer (Closed)
Descriptionjson parser/writer correctness fuzzer
BUG=539572
Committed: https://crrev.com/022c9c2dd56926d03f2dd21036ebde0da869ee7b
Cr-Commit-Position: refs/heads/master@{#428829}
Patch Set 1 #
Total comments: 17
Patch Set 2 : review #
Total comments: 2
Patch Set 3 : added comment #
Total comments: 1
Patch Set 4 : moved comment up #
Messages
Total messages: 16 (5 generated)
aizatsky@chromium.org changed reviewers: + danakj@chromium.org, inferno@chromium.org, ochang@chromium.org
> BUG= No bug to point to for this work? https://codereview.chromium.org/2449323004/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2449323004/diff/1/base/BUILD.gn#newcode2462 base/BUILD.gn:2462: "json/base_json_correctness_fuzzer.cc", the base_json_ in this file name is redundant, it's already in base/json/. https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... File base/json/base_json_correctness_fuzzer.cc (right): https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:7: // so that presumably valdid json is parsed/written again. valid https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:22: if (size < 1) if (!size) is the usual way to write this, but don't you need at least 2 bytes? cuz 1 byte would be an empty string with options https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:29: const int options = data[size - 1]; Where is this format documented? https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:30: auto input_value = base::JSONReader::ReadAndReturnError( parsed_value? https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:36: std::string output1; parsed_output? https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:40: auto value2 = base::JSONReader::ReadAndReturnError( double_parsed_value? https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:44: std::string output2; double_parsed_output? using numbers to distinguish variable names isn't my fave
Description was changed from ========== json parser/writer correctness fuzzer BUG= ========== to ========== json parser/writer correctness fuzzer BUG=539572 ==========
All issues addressed, file renamed, bug added. PTAL.
Thanks https://codereview.chromium.org/2449323004/diff/20001/base/json/correctness_f... File base/json/correctness_fuzzer.cc (right): https://codereview.chromium.org/2449323004/diff/20001/base/json/correctness_f... base/json/correctness_fuzzer.cc:29: const int options = data[size - 1]; Still wondering tho, where is this input format for |data| documented? If nowhere at least it should be documented in a comment on top of this method.
https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... File base/json/base_json_correctness_fuzzer.cc (right): https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/10/28 at 01:50:56, danakj wrote: > 2016 done https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:7: // so that presumably valdid json is parsed/written again. On 2016/10/28 at 01:50:56, danakj wrote: > valid done https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:22: if (size < 1) On 2016/10/28 at 01:50:56, danakj wrote: > if (!size) is the usual way to write this, but don't you need at least 2 bytes? cuz 1 byte would be an empty string with options done. Agree, less than 2 bytes is pointless. https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:29: const int options = data[size - 1]; On 2016/10/28 at 01:50:56, danakj wrote: > Where is this format documented? It is not. It is each fuzzer's decision on how to interpret input from fuzzer engine. Using a byte or two for config is common pattern. https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:30: auto input_value = base::JSONReader::ReadAndReturnError( On 2016/10/28 at 01:50:56, danakj wrote: > parsed_value? done https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:36: std::string output1; On 2016/10/28 at 01:50:56, danakj wrote: > parsed_output? done https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:40: auto value2 = base::JSONReader::ReadAndReturnError( On 2016/10/28 at 01:50:56, danakj wrote: > double_parsed_value? done https://codereview.chromium.org/2449323004/diff/1/base/json/base_json_correct... base/json/base_json_correctness_fuzzer.cc:44: std::string output2; On 2016/10/28 at 01:50:56, danakj wrote: > double_parsed_output? > > using numbers to distinguish variable names isn't my fave done https://codereview.chromium.org/2449323004/diff/20001/base/json/correctness_f... File base/json/correctness_fuzzer.cc (right): https://codereview.chromium.org/2449323004/diff/20001/base/json/correctness_f... base/json/correctness_fuzzer.cc:29: const int options = data[size - 1]; On 2016/10/29 at 01:12:42, danakj wrote: > Still wondering tho, where is this input format for |data| documented? If nowhere at least it should be documented in a comment on top of this method. done
LGTM https://codereview.chromium.org/2449323004/diff/40001/base/json/correctness_f... File base/json/correctness_fuzzer.cc (right): https://codereview.chromium.org/2449323004/diff/40001/base/json/correctness_f... base/json/correctness_fuzzer.cc:22: // We will use the last byte of data as parsing options. nit: I suggest putting this on top of the function, like after the "Entry point for libFuzzer. We will use the ..."
On 2016/10/31 at 20:02:09, danakj wrote: > LGTM > > https://codereview.chromium.org/2449323004/diff/40001/base/json/correctness_f... > File base/json/correctness_fuzzer.cc (right): > > https://codereview.chromium.org/2449323004/diff/40001/base/json/correctness_f... > base/json/correctness_fuzzer.cc:22: // We will use the last byte of data as parsing options. > nit: I suggest putting this on top of the function, like after the "Entry point for libFuzzer. We will use the ..." Done. Thank you for review, submitting. Let's see how useful this is.
The CQ bit was checked by aizatsky@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2449323004/#ps60001 (title: "moved comment up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/31 20:11:15, aizatsky wrote: > On 2016/10/31 at 20:02:09, danakj wrote: > > LGTM > > > > > https://codereview.chromium.org/2449323004/diff/40001/base/json/correctness_f... > > File base/json/correctness_fuzzer.cc (right): > > > > > https://codereview.chromium.org/2449323004/diff/40001/base/json/correctness_f... > > base/json/correctness_fuzzer.cc:22: // We will use the last byte of data as > parsing options. > > nit: I suggest putting this on top of the function, like after the "Entry > point for libFuzzer. We will use the ..." > > Done. Thank you for review, submitting. > Let's see how useful this is. It's probably worth pointing out that the base json parser is not meant to be security hardened and handle invalid json. So depending on what kind of fuzzing you're doing we may or may not want to act on the results. See https://bugs.chromium.org/p/chromium/issues/detail?id=644664#c5.
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== json parser/writer correctness fuzzer BUG=539572 ========== to ========== json parser/writer correctness fuzzer BUG=539572 Committed: https://crrev.com/022c9c2dd56926d03f2dd21036ebde0da869ee7b Cr-Commit-Position: refs/heads/master@{#428829} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/022c9c2dd56926d03f2dd21036ebde0da869ee7b Cr-Commit-Position: refs/heads/master@{#428829} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
