Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1531743002:
fuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented (Closed)
Descriptionfuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented
R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org
TBR=krasin@chromium.org
BUG=539572
Committed: https://crrev.com/6604dc8a92b4c635de2c5c29e2f303f4dc573016
Cr-Commit-Position: refs/heads/master@{#367162}
Patch Set 1 #
Total comments: 5
Patch Set 2 : fix nits and split payload into two parts for two input arguments #
Total comments: 2
Patch Set 3 : fix header path and upgrade choosing of codepage parameter #Patch Set 4 : added local variable, comments + rebased #
Total comments: 2
Patch Set 5 : removed confusing local var + fixed buffer passed to UnicodeString constructor #
Messages
Total messages: 28 (14 generated)
inferno@chromium.org changed reviewers: + jshin@chromium.org
https://codereview.chromium.org/1531743002/diff/1/testing/libfuzzer/fuzzers/u... File testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc (right): https://codereview.chromium.org/1531743002/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:12: // Need null-terminated string Nit: end with period (.). https://codereview.chromium.org/1531743002/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:16: //TODO: need best input generation for third argument passed Nit: space b/w // and TODO and next line, no need of extra starting space. https://codereview.chromium.org/1531743002/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:20: buffer.data()); I wonder if we should split data into two parts and use one for buffer and one for codepage. +cc Jshin@, our unicode expert.
krasin@google.com changed reviewers: + krasin@google.com
https://codereview.chromium.org/1531743002/diff/1/testing/libfuzzer/fuzzers/u... File testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc (right): https://codereview.chromium.org/1531743002/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:17: // may be use dicitonary of different codepages typo: s/dicitonary/dictionary https://codereview.chromium.org/1531743002/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:20: buffer.data()); On 2015/12/16 16:58:08, inferno wrote: > I wonder if we should split data into two parts and use one for buffer and one > for codepage. +cc Jshin@, our unicode expert. That's generally a good idea. See https://code.google.com/p/chromium/codesearch#chromium/src/testing/libfuzzer/... as an example (I split the data into three parts there) Otherwise, it sometimes is not possible to get the full (or even decent) coverage.
LGTM https://codereview.chromium.org/1531743002/diff/20001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc (right): https://codereview.chromium.org/1531743002/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:8: #include "unicode/unistr.h" nit: In chromium, we use a longer path : third_party/icu/source/common/unicode/unistr.h (in Blink, we use a shorter path). https://codereview.chromium.org/1531743002/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:17: // may be use dictionary of different codepages. I agree. The list of encoding names (and their aliases) in use by Blink and Chrome is in third_party/icu/source/data/mappings/convrtrs.txt or third_party/icu/source/data/mappings/ucmlocal.txt
The CQ bit was checked by mmoroz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org Link to the patchset: https://codereview.chromium.org/1531743002/#ps40001 (title: "fix header path and upgrade choosing of codepage parameter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531743002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531743002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by mmoroz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531743002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531743002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/1531743002/diff/60001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc (right): https://codereview.chromium.org/1531743002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:83: if (offset < 0) offset is confusing, just change size. https://codereview.chromium.org/1531743002/diff/60001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/unicode_string_codepage_create_fuzzer.cc:94: icu::UnicodeString unicode_string(buffer.data() + 1, offset, codepage); buffer.data()+1 seems wrong, it should be buffer.data()
The CQ bit was checked by inferno@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org Link to the patchset: https://codereview.chromium.org/1531743002/#ps80001 (title: "removed confusing local var + fixed buffer passed to UnicodeString constructor")
Description was changed from ========== fuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org BUG=539572 ========== to ========== fuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org TBR=krasin@chromium.org BUG=539572 ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531743002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531743002/80001
The CQ bit was unchecked by inferno@chromium.org
The CQ bit was checked by inferno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1531743002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1531743002/80001
Message was sent while issue was closed.
Description was changed from ========== fuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org TBR=krasin@chromium.org BUG=539572 ========== to ========== fuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org TBR=krasin@chromium.org BUG=539572 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== fuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org TBR=krasin@chromium.org BUG=539572 ========== to ========== fuzzer for third_party icu::UnicodeString::UnicodeString (invokes doCodepageCreate) implemented R=aizatsky@chromium.org, inferno@chromium.org, krasin@chromium.org TBR=krasin@chromium.org BUG=539572 Committed: https://crrev.com/6604dc8a92b4c635de2c5c29e2f303f4dc573016 Cr-Commit-Position: refs/heads/master@{#367162} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6604dc8a92b4c635de2c5c29e2f303f4dc573016 Cr-Commit-Position: refs/heads/master@{#367162} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
