Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807083002:
Add LLVM fuzzer: QCMS color space and color transform (Closed)
|
Created:
3 years, 8 months ago by Noel Gordon Modified:
3 years, 8 months ago CC:
chromium-reviews, fuzzing_chromium.org, Nico Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd LLVM fuzzer: QCMS color space and color transform
Add fuzzer that reads test input and creates a color profile
from it (or bails if not). Given that profile |test|, create
another profile |srgb| used to transform colors to/from when
running the color transform fuzzer test stage.
Test ICC version 2 profiles only since ICC version 4 support
in QCMS is not enabled in Chrome.
Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to
use //base and //testing code.
Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC
profiles uploaded to qcms_color_space_fuzzer GCS bucket.
Set max_len to 4Meg (color profiles can be large and this is
the limit that Chrome accepts), and use the ICC profile dict
added in http://crrev.com/461603
TBR=inferno@chromium.org
BUG=708016
Review-Url: https://codereview.chromium.org/2807083002
Cr-Commit-Position: refs/heads/master@{#464398}
Committed: https://chromium.googlesource.com/chromium/src/+/6997c5c92f9e46e7834fa539b360991ac71e2e01
Patch Set 1 : Add qcms fuzzer to //testing/libfuzzer/fuzzers. #Patch Set 2 : Review comments: Move fuzzer into third_party/qcms. #
Total comments: 2
Patch Set 3 : Review comments: Move to root BUILD.gn file. #
Total comments: 5
Patch Set 4 : Use an in-situ hash function. #
Messages
Total messages: 66 (38 generated)
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support is not enabled in chrome. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles to uploaded qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 BUG= ========== to ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles to uploaded qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 BUG= ==========
noel@chromium.org changed reviewers: + inferno@chromium.org
PTAL.
Description was changed from ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles to uploaded qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 BUG= ========== to ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles to uploaded qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ==========
Description was changed from ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles to uploaded qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ========== to ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kcc@chromium.org changed reviewers: + kcc@chromium.org
Can this go to the project's directory instead of testing/libfuzzer/fuzzers?
Maybe, I don't really know the answer to your question or why it's important.
On 2017/04/11 00:47:06, noel gordon wrote: > Maybe, I don't really know the answer to your question or why it's important. It's important because we want the fuzz target to be owned by the same folks who own the code being fuzzed, and the most straightforward way to enforce/document this ownership is to have fuzz targets adjacent to the code.
On 2017/04/11 01:11:06, kcc2 wrote: > On 2017/04/11 00:47:06, noel gordon wrote: > > Maybe, I don't really know the answer to your question or why it's important. > > It's important because we want the fuzz target to be owned by the same folks who > own the code being fuzzed, > and the most straightforward way to enforce/document this ownership is to have > fuzz targets adjacent to the code. What BUILD.gn are required to make this work? Is there an example (because I don't recall seeing an example reading the libfuzzer docs, though I was reading quickly and might of missed it).
Found the following, trying that ... https://chromium.googlesource.com/chromium/src/testing/libfuzzer/+/HEAD/getti...
On 2017/04/11 03:13:22, noel gordon wrote: > Found the following, trying that ... > > https://chromium.googlesource.com/chromium/src/testing/libfuzzer/+/HEAD/getti... third_party/qcms/BUILD.gn, added the following lines after moving qcms_color_space_fuzzer.cc into third_party/qcms fuzzer_test("qcms_color_space_fuzzer") { sources = [ "//testing/libfuzzer/fuzzers/color_space_data.h", "qcms_color_space_fuzzer.cc", ] deps = [ ":qcms", "//base", ] dict = "//testing/libfuzzer/fuzzers/dicts/icc.dict" libfuzzer_options = [ "max_len=4194304" ] } Then ran % ninja -C out/libfuzzer qcms_color_space_fuzzer ninja: Entering directory `out/libfuzzer' ninja: error: unknown target 'qcms_color_space_fuzzer'
Adding empty group("fuzzers") to qcms/BUILD.gn
group("fuzzers") {
}
Made testing/libfuzzer/fuzzers/BUILD.gn depend on that
group("fuzzers") {
deps = [ "//third_party/qcms:fuzzers"]
}
And tried compiling
% ninja -C out/libfuzzer qcms_color_space_fuzzer
[1/1] Regenerating ninja files
[2/2] LINK ./qcms_color_space_fuzzer
Add third_party/qcms/DEPS add to allow qcms_color_space_fuzzer to use //base and //testing stuff
Description was changed from ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ========== to ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to use //base and //testing code. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ==========
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in chrome. Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to use //base and //testing code. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ========== to ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in Chrome. Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to use //base and //testing code. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ==========
kcc@chromium.org changed reviewers: + ochang@chromium.org
https://codereview.chromium.org/2807083002/diff/20001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2807083002/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/BUILD.gn:14: "//third_party/qcms:fuzzers", chiming in here. is this explicit dependency necessary (i.e. is the fuzzer_test discoverable from the root BUILD.gn anyway)? If the dependency is necessary, then the convention is to put this in the root BUILD.gn instead here: https://chromium.googlesource.com/chromium/src/+/b87f01f54ba47ace1ddf35476a91...
https://codereview.chromium.org/2807083002/diff/20001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2807083002/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/BUILD.gn:14: "//third_party/qcms:fuzzers", On 2017/04/11 19:46:47, Oliver Chang wrote: > chiming in here. is this explicit dependency necessary (i.e. is the fuzzer_test > discoverable from the root BUILD.gn anyway)? > > If the dependency is necessary, then the convention is to put this in the root > BUILD.gn instead here: > > https://chromium.googlesource.com/chromium/src/+/b87f01f54ba47ace1ddf35476a91... > > Ok, moved it there.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
lgtm
noel@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for the DEPS change.
noel@chromium.org changed reviewers: + thakis@chromium.org
+thakis for the BUILD.gn change.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
deps lgtm https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... File third_party/qcms/qcms_color_space_fuzzer.cc (right): https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... third_party/qcms/qcms_color_space_fuzzer.cc:36: auto transform = Nit: auto* https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... third_party/qcms/qcms_color_space_fuzzer.cc:72: const size_t hash = Hash(reinterpret_cast<const char*>(data), size); How about just running this through base::SHA1HashBytes? https://cs.chromium.org/chromium/src/base/sha1.h?rcl=d7eaf476075d774e835fc031... I don't think there's any guarantee that StringPieceHash is stable over time, and I could see this theoretically causing issues with reproducibility.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... File third_party/qcms/qcms_color_space_fuzzer.cc (right): https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... third_party/qcms/qcms_color_space_fuzzer.cc:36: auto transform = On 2017/04/12 07:19:23, dcheng wrote: > Nit: auto* Done. https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... third_party/qcms/qcms_color_space_fuzzer.cc:72: const size_t hash = Hash(reinterpret_cast<const char*>(data), size); On 2017/04/12 07:19:23, dcheng wrote: > How about just running this through base::SHA1HashBytes? > > https://cs.chromium.org/chromium/src/base/sha1.h?rcl=d7eaf476075d774e835fc031... Too slow: need a good and fast hash. > I don't think there's any guarantee that StringPieceHash is stable over time, > and I could see this theoretically causing issues with reproducibility. OK, used StringPieceHash's hash in-situ, and removed the StringPiece dependency.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... File third_party/qcms/qcms_color_space_fuzzer.cc (right): https://codereview.chromium.org/2807083002/diff/40001/third_party/qcms/qcms_c... third_party/qcms/qcms_color_space_fuzzer.cc:72: const size_t hash = Hash(reinterpret_cast<const char*>(data), size); On 2017/04/12 12:09:47, noel gordon wrote: > On 2017/04/12 07:19:23, dcheng wrote: > > How about just running this through base::SHA1HashBytes? > > > > > https://cs.chromium.org/chromium/src/base/sha1.h?rcl=d7eaf476075d774e835fc031... > > Too slow: need a good and fast hash. > > > I don't think there's any guarantee that StringPieceHash is stable over time, > > and I could see this theoretically causing issues with reproducibility. > > OK, used StringPieceHash's hash in-situ, and removed the StringPiece dependency. Hmm... how about just use one byte of the test input for this then? I think that would be the simplest of all.
On 2017/04/12 20:21:10, dcheng wrote: > Hmm... how about just use one byte of the test input for this then? I think that > would be the simplest of all. In a previous review, the idea of picking a hash from some data byte(s), or from the data size, was discussed. There were concerns about coverage doing that. Using a hash computed from all the data was recommended to increase coverage [1]. The full hash over data does in fact produce a uniform distribution over size_t. The simpler methods did not. [1] https://codereview.chromium.org/2797473003/#msg11
On 2017/04/13 00:34:13, noel gordon wrote: > On 2017/04/12 20:21:10, dcheng wrote: > > > Hmm... how about just use one byte of the test input for this then? I think > that > > would be the simplest of all. > > In a previous review, the idea of picking a hash from some data byte(s), or from > the data size, was discussed. There were concerns about coverage doing that. > > Using a hash computed from all the data was recommended to increase coverage > [1]. The full hash over data does in fact produce a uniform distribution over > size_t. The simpler methods did not. > > [1] https://codereview.chromium.org/2797473003/#msg11 My comment shouldn't block this CL, but I want to better understand the fuzzing system: do we consider the data inputs to the fuzzer to be random? If so, what's wrong with using consuming a constant amount of bytes for any needed random seeds, and then using the remainder of the data to populate the qcms profiles? If we consider the input data random, then I don't see what hashing adds other than complexity. You'll also still get non-uniform distributions if you modulo by something that's not a power of 2. Note: I'm suggesting that any data used for random seeding and any data used for the test itself should be independent.
noel@chromium.org changed reviewers: + jochen@chromium.org - thakis@chromium.org
On 2017/04/13 00:59:30, dcheng wrote: > My comment shouldn't block this CL, but I want to better understand the fuzzing Ok thanks. Noticed Nico is OOO though, so +jochen for the src/BUILD.gn change.
lgtm
The CQ bit was checked by noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ochang@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2807083002/#ps60001 (title: "Use an in-situ hash function.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
noel@chromium.org changed reviewers: + aizatsky@chromium.org
** Presubmit ERRORS **
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
'+testing/libfuzzer/fuzzers',
Suggested missing target path OWNERS:
inferno@chromium.org
kcc@chromium.org
Description was changed from ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in Chrome. Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to use //base and //testing code. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 BUG=708016 ========== to ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in Chrome. Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to use //base and //testing code. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 TBR=inferno@chromium.org BUG=708016 ==========
The CQ bit was checked by noel@chromium.org
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": 60001, "attempt_start_ts": 1492085898465750,
"parent_rev": "6ad54113b471bd1e8e82179d23cb55d70b1b2f38", "commit_rev":
"6997c5c92f9e46e7834fa539b360991ac71e2e01"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492085898465750,
"parent_rev": "6ad54113b471bd1e8e82179d23cb55d70b1b2f38", "commit_rev":
"6997c5c92f9e46e7834fa539b360991ac71e2e01"}
Message was sent while issue was closed.
Description was changed from ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in Chrome. Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to use //base and //testing code. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 TBR=inferno@chromium.org BUG=708016 ========== to ========== Add LLVM fuzzer: QCMS color space and color transform Add fuzzer that reads test input and creates a color profile from it (or bails if not). Given that profile |test|, create another profile |srgb| used to transform colors to/from when running the color transform fuzzer test stage. Test ICC version 2 profiles only since ICC version 4 support in QCMS is not enabled in Chrome. Add third_party/qcms/DEPS: allows qcms_color_space_fuzzer to use //base and //testing code. Speed: achieves ~2300 execs/s on Mac Air. Seed corpus of ICC profiles uploaded to qcms_color_space_fuzzer GCS bucket. Set max_len to 4Meg (color profiles can be large and this is the limit that Chrome accepts), and use the ICC profile dict added in http://crrev.com/461603 TBR=inferno@chromium.org BUG=708016 Review-Url: https://codereview.chromium.org/2807083002 Cr-Commit-Position: refs/heads/master@{#464398} Committed: https://chromium.googlesource.com/chromium/src/+/6997c5c92f9e46e7834fa539b360... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6997c5c92f9e46e7834fa539b360... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
