Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797473003:
Add LLVM fuzzer: Skia 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, scroggo_chromium, hcm1, msarett1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd LLVM fuzzer: Skia color space and color transform
Add common color profile data file color_space_data.h, which
is a separate file so it can be included in other fuzzers.
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.
Speed: achieves ~1100 execs/s on Mac Air and 1900 execs/s on
my Mac Pro. Seed corpus of ICC color profiles for the fuzzer
uploaded to the skia_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
Review-Url: https://codereview.chromium.org/2797473003
Cr-Commit-Position: refs/heads/master@{#463156}
Committed: https://chromium.googlesource.com/chromium/src/+/373b1ecb464977173aa74566bb6f672be10df687
Patch Set 1 #
Total comments: 20
Patch Set 2 : Redesign the pixel generator. #
Total comments: 8
Patch Set 3 : Use hash and prgn, pre-compute srgb profiles. #
Total comments: 11
Patch Set 4 : Use size_t for alloc sizes. #Patch Set 5 : Make hash const. #
Total comments: 8
Patch Set 6 : rm -f DCHECK, use static buffers. #
Messages
Total messages: 66 (39 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...
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: Skia color space and color transform BUG=708016 ========== to ========== Add LLVM fuzzer: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1500 execs/s on Mac Air and 2600 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_color_space_fuzzer GCS bucket. BUG=708016 ==========
noel@chromium.org changed reviewers: + inferno@chromium.org, mmoroz@chromium.org
PTAL.
https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:33: uint32_t data[kPixels * 4]; This is a fine source of random pixel data, and contains whatever is on the data stack at this location. If this fuzzer also gets run in MSAN though, I suspect that MSAN might complain that the data is uninitialized, if/when it is read by the color transform code during transform->apply(). How do we tell MSAN to stfu about that, and just accept the uninitialized reads from the data variable?
Description was changed from ========== Add LLVM fuzzer: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1500 execs/s on Mac Air and 2600 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_color_space_fuzzer GCS bucket. BUG=708016 ========== to ========== Add LLVM fuzzer: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1500 execs/s on Mac Air and 2600 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_color_space_fuzzer GCS bucket. Set max_len to 4Meg; color profiles can be large and this is the limit that Chrome accepts. BUG=708016 ==========
Description was changed from ========== Add LLVM fuzzer: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1500 execs/s on Mac Air and 2600 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_color_space_fuzzer GCS bucket. Set max_len to 4Meg; color profiles can be large and this is the limit that Chrome accepts. BUG=708016 ========== to ========== Add LLVM fuzzer: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1500 execs/s on Mac Air and 2600 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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 ==========
Thanks for writing this target. It looks pretty interesting, I left some comments. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... testing/libfuzzer/fuzzers/BUILD.gn:423: fuzzer_test("skia_color_space_fuzzer") { Could you please move both the source code and the GN target to third_party/skia/ ? I know that we have several fuzz targets for skia here (https://cs.chromium.org/search/?q=file:skia.*+file:%5Esrc/testing/libfuzzer/f...), but this is bad and we actually should move them to the skia project directory as well. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/c... File testing/libfuzzer/fuzzers/color_space_data.h (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/c... testing/libfuzzer/fuzzers/color_space_data.h:8: static const char srgb_data[] = { It seems that names should be in the following format: kSrgbData, kAdobeData, etc. As per coding guidelines (https://google.github.io/styleguide/cppguide.html#Constant_Names) and existing examples (https://cs.chromium.org/search/?q=file:.*%5C.h+%22static+const+char%22+packag...) https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:33: uint32_t data[kPixels * 4]; On 2017/04/04 12:23:16, noel gordon wrote: > This is a fine source of random pixel data, and contains whatever is on the data > stack at this location. > > If this fuzzer also gets run in MSAN though, I suspect that MSAN might complain > that the data is uninitialized, if/when it is read by the color transform code > during transform->apply(). > > How do we tell MSAN to stfu about that, and just accept the uninitialized reads > from the data variable? I'm afraid that we cannot rely on this approach because: 1) all the testcases generated by the fuzzer should be fully reproducible if we re-run them, i.e. we cannot use a random data from a random memory region 2) this might be looking as a hacky though efficient solution, but we shouldn't use bad coding patterns anyway Since you always need a constant number of bytes here, I'd recommend to simply use first |4 * kPixels| bytes of |data| coming from fuzzer. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:51: return (T*)(address & ~31); // 32-bit aligned. Coudl you please use c++ cast here? https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:56: constexpr size_t kSizeLimit = 4 * 1024 * 1024; As a follow-up to comment on line #33, |kSizeLimit| here and in GN config should be increased by |4 * kPixels|. Also, it would make sense to have a lower limit check, i.e. |if (size <= 4 * kPixels)| . https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:60: test = SkColorSpace::MakeICC(data, size); As first |4 * kPixels| bytes of |data| will be used for initialization of data used in |ColorTransform| function, we should do here something like: std::vector<uint8_t> buffer(data + 4 * kPixels, buffer + size - 4 * kPixels); and then use |buffer.data()| and |buffer.size()| instead of |data| and |size|. That's important to have these bytes in a separate buffer to be able catch out-of-bound access to the left. Thus, we cannot just use |data + 4 * kPixels| and |size - 4 * kPixels| without creating a separate buffer. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:64: switch (size & 7) { // Create profile to transform pixels to/from. Nice trick, but it would be better to avoid relying only on |size| here, otherwise you would never get an input of length |8 * n| to be processed by cases other than case 0, and so forth. Our current recommendation for such cases is to use |std::hash| value calculated from |data|, e.g. https://cs.chromium.org/search/?q=file:fuzzer.*.cc+std::hash+package:%5Echrom...
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/2797473003/diff/1/testing/libfuzzer/fuzzers/B... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... testing/libfuzzer/fuzzers/BUILD.gn:423: fuzzer_test("skia_color_space_fuzzer") { On 2017/04/04 13:43:06, mmoroz wrote: > Could you please move both the source code and the GN target to > third_party/skia/ ? Don't really work on Skia myself. I know that we have several fuzz targets for skia here > (https://cs.chromium.org/search/?q=file:skia.*+file:%5Esrc/testing/libfuzzer/f...), > but this is bad and we actually should move them to the skia project directory > as well. Noticed there were a few skia fuzzer targets in the current BUILD file too. So that was a bit confusing to me, but perhaps another bug could move them all into Skia. Currently I want to share/use the color_space_data.h use another fuzzer I'm building (for qcms which has nothing to do with skia), so for now I'd like to do that. Where these fuzzers should eventually end up living is a good question though, but not one I'm gonna tackle. Perhaps after the fuzzers are built, we can decide on that, as a subject of another bug. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/c... File testing/libfuzzer/fuzzers/color_space_data.h (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/c... testing/libfuzzer/fuzzers/color_space_data.h:8: static const char srgb_data[] = { On 2017/04/04 13:43:06, mmoroz wrote: > It seems that names should be in the following format: kSrgbData, kAdobeData, > etc. Done. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:33: uint32_t data[kPixels * 4]; On 2017/04/04 13:43:06, mmoroz wrote: > On 2017/04/04 12:23:16, noel gordon wrote: > > This is a fine source of random pixel data, and contains whatever is on the > data > > stack at this location. > > > > If this fuzzer also gets run in MSAN though, I suspect that MSAN might > complain > > that the data is uninitialized, if/when it is read by the color transform code > > during transform->apply(). > > > > How do we tell MSAN to stfu about that, and just accept the uninitialized > reads > > from the data variable? > > I'm afraid that we cannot rely on this approach because: > 1) all the testcases generated by the fuzzer should be fully reproducible if we > re-run them, i.e. we cannot use a random data from a random memory region IC, we can fix that ... > 2) this might be looking as a hacky though efficient solution, but we shouldn't > use bad coding patterns anyway I created a deterministic "random" pixel generator. > Since you always need a constant number of bytes here, I'd recommend to simply > use first |4 * kPixels| bytes of |data| coming from fuzzer. Note the input data can be less than 4 * kPixels. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:51: return (T*)(address & ~31); // 32-bit aligned. On 2017/04/04 13:43:06, mmoroz wrote: > Coudl you please use c++ cast here? Removed all this. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:56: constexpr size_t kSizeLimit = 4 * 1024 * 1024; On 2017/04/04 13:43:06, mmoroz wrote: > As a follow-up to comment on line #33, |kSizeLimit| here and in GN config should > be increased by |4 * kPixels|. The new "random" pixel generator is independent of the size of input data, and so does not depend on kSizeLimit either. > Also, it would make sense to have a lower limit > check, i.e. |if (size <= 4 * kPixels)| . Added a lower limit that is sensible for color profiles (128). https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:60: test = SkColorSpace::MakeICC(data, size); On 2017/04/04 13:43:06, mmoroz wrote: > As first |4 * kPixels| bytes of |data| will be used for initialization of data > used in |ColorTransform| function, we should do here something like: > > std::vector<uint8_t> buffer(data + 4 * kPixels, buffer + size - 4 * kPixels); > > and then use |buffer.data()| and |buffer.size()| instead of |data| and |size|. > That's important to have these bytes in a separate buffer to be able catch > out-of-bound access to the left. Thus, we cannot just use |data + 4 * kPixels| > and |size - 4 * kPixels| without creating a separate buffer. With the new design, I'm not sure any of this applies. In thinking about a png decoder, for example, which creates many internal structures to read/write data from, I be surprised if a fuzzer could not spot any out-of-bounds reads or writes on said structures (none of which are wrapped in a std::vector). https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:64: switch (size & 7) { // Create profile to transform pixels to/from. On 2017/04/04 13:43:07, mmoroz wrote: > Nice trick, but it would be better to avoid relying only on |size| here, > otherwise you would never get an input of length |8 * n| to be processed by > cases other than case 0, and so forth. Not some a trick. The distribution of size & 7 tends to uniform as the corpus grows. The hash idea seems fine, but it does add measurable overhead which decreases exec/s ... > Our current recommendation for such cases is to use |std::hash| value calculated > from |data|, e.g. > https://cs.chromium.org/search/?q=file:fuzzer.*.cc+std::hash+package:%5Echrom... Hmmm: looking at the link, copying the input data into a std::string<> just to compute a std::hash wastes too time and is maybe not a good idea for color profile data. A better approach is to 1) use base::StringPiece and 2) compute a fixed-sized hash (we know that input data size is at least 128 now) to maintain performance and provide a reasonable hash.
Description was changed from ========== Add LLVM fuzzer: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1500 execs/s on Mac Air and 2600 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1400 execs/s on Mac Air and 2100 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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.
scroggo@chromium.org changed reviewers: + scroggo@chromium.org
https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... testing/libfuzzer/fuzzers/BUILD.gn:423: fuzzer_test("skia_color_space_fuzzer") { On 2017/04/05 14:14:07, noel gordon wrote: > On 2017/04/04 13:43:06, mmoroz wrote: > > Could you please move both the source code and the GN target to > > third_party/skia/ ? > > Don't really work on Skia myself. > > I know that we have several fuzz targets for skia here > > > (https://cs.chromium.org/search/?q=file:skia.*+file:%5Esrc/testing/libfuzzer/f...), > > but this is bad and we actually should move them to the skia project directory > > as well. > > Noticed there were a few skia fuzzer targets in the current BUILD file too. So > that was a bit confusing to me, but perhaps another bug could move them all into > Skia. > > Currently I want to share/use the color_space_data.h use another fuzzer I'm > building (for qcms which has nothing to do with skia), so for now I'd like to do > that. Where these fuzzers should eventually end up living is a good question > though, but not one I'm gonna tackle. Perhaps after the fuzzers are built, we > can decide on that, as a subject of another bug. +1. FWIW, I do not think it would make sense to put the fuzzers in third_party/skia, which is its own independent repository, since they depend on other Chromium code. Code inside the Skia repository should be runnable outside of Chromium. If this is not a good place for them, maybe src/skia is better? Skia has its own fuzzers though, and I wouldn't be opposed to adding a fuzzer in Skia which uses Skia's infrastructure. But it would be nice to be able to reuse for other fuzzers as well.
Looks good, but we should remove hacky |GeneratePixels| method and use a part of the data instead. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... File testing/libfuzzer/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... testing/libfuzzer/fuzzers/BUILD.gn:423: fuzzer_test("skia_color_space_fuzzer") { On 2017/04/05 21:02:47, scroggo_chromium wrote: > On 2017/04/05 14:14:07, noel gordon wrote: > > On 2017/04/04 13:43:06, mmoroz wrote: > > > Could you please move both the source code and the GN target to > > > third_party/skia/ ? > > > > Don't really work on Skia myself. > > > > I know that we have several fuzz targets for skia here > > > > > > (https://cs.chromium.org/search/?q=file:skia.*+file:%5Esrc/testing/libfuzzer/f...), > > > but this is bad and we actually should move them to the skia project > directory > > > as well. > > > > Noticed there were a few skia fuzzer targets in the current BUILD file too. > So > > that was a bit confusing to me, but perhaps another bug could move them all > into > > Skia. > > > > Currently I want to share/use the color_space_data.h use another fuzzer I'm > > building (for qcms which has nothing to do with skia), so for now I'd like to > do > > that. Where these fuzzers should eventually end up living is a good question > > though, but not one I'm gonna tackle. Perhaps after the fuzzers are built, we > > can decide on that, as a subject of another bug. > > +1. FWIW, I do not think it would make sense to put the fuzzers in > third_party/skia, which is its own independent repository, since they depend on > other Chromium code. Code inside the Skia repository should be runnable outside > of Chromium. If this is not a good place for them, maybe src/skia is better? > > Skia has its own fuzzers though, and I wouldn't be opposed to adding a fuzzer in > Skia which uses Skia's infrastructure. But it would be nice to be able to reuse > for other fuzzers as well. Makes sense. Thanks for the explanation! https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:33: uint32_t data[kPixels * 4]; On 2017/04/05 14:14:07, noel gordon wrote: > On 2017/04/04 13:43:06, mmoroz wrote: > > On 2017/04/04 12:23:16, noel gordon wrote: > > > This is a fine source of random pixel data, and contains whatever is on the > > data > > > stack at this location. > > > > > > If this fuzzer also gets run in MSAN though, I suspect that MSAN might > > complain > > > that the data is uninitialized, if/when it is read by the color transform > code > > > during transform->apply(). > > > > > > How do we tell MSAN to stfu about that, and just accept the uninitialized > > reads > > > from the data variable? > > > > I'm afraid that we cannot rely on this approach because: > > 1) all the testcases generated by the fuzzer should be fully reproducible if > we > > re-run them, i.e. we cannot use a random data from a random memory region > > IC, we can fix that ... > > > 2) this might be looking as a hacky though efficient solution, but we > shouldn't > > use bad coding patterns anyway > > I created a deterministic "random" pixel generator. > > > Since you always need a constant number of bytes here, I'd recommend to simply > > use first |4 * kPixels| bytes of |data| coming from fuzzer. > > Note the input data can be less than 4 * kPixels. > Right, but something like: if (size < 4 * kPixels) return 0; in the beginning of LLVMFuzzerTestOneInput function will help. LibFuzzer is able to quickly understand that inputs shorter than that limit do no make sense and won't spend time on them. https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:64: switch (size & 7) { // Create profile to transform pixels to/from. On 2017/04/05 14:14:07, noel gordon wrote: > On 2017/04/04 13:43:07, mmoroz wrote: > > Nice trick, but it would be better to avoid relying only on |size| here, > > otherwise you would never get an input of length |8 * n| to be processed by > > cases other than case 0, and so forth. > > Not some a trick. The distribution of size & 7 tends to uniform as the corpus > grows. The hash idea seems fine, but it does add measurable overhead which > decreases exec/s ... > > > Our current recommendation for such cases is to use |std::hash| value > calculated > > from |data|, e.g. > > > https://cs.chromium.org/search/?q=file:fuzzer.*.cc+std::hash+package:%5Echrom... > > Hmmm: looking at the link, copying the input data into a std::string<> just to > compute a std::hash wastes too time and is maybe not a good idea for color > profile data. > > A better approach is to 1) use base::StringPiece and 2) compute a fixed-sized > hash (we know that input data size is at least 128 now) to maintain performance > and provide a reasonable hash. The distribution of size & 7 doesn't matter because all inputs of the following sizes: 8, 16, 24, 32, ..., 1024, ..., 2048, ... 8 * n will never go into any of the cases except of the case 0. The same applies to inputs of sizes (1 + 8 * n) as they will always visit only case 1 branch, and so on. This is bad for coverage. If hash is a performance concern here, you can extract one byte from the data for flags randomization (example: https://cs.chromium.org/chromium/src/third_party/sqlite/fuzz/sqlite3_prepare_...) StringPieceHash looks fine to me as well. https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:23: pixels[i] = (memory = memory << 3 ^ kSRGBData[1024 + i % 2048]); It looks like |pixels| will be always the same under this condition. Why not to put this into a constant then? https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:26: pixels[i] = (memory = memory << 3 ^ *data++); This creates a dependency between |pixels| and |SkColorSpace| being created on line 58. This is bad for coverage. The only thing we allow ourselves in terms of "re-using the data" is to calculate a hash for flags randomization. In other cases we should split the data and use different parts of it independently. https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:55: if (size < 128 || size > kSizeLimit) Let's use lower boundary of |128 + 4 * kPixels| here and copy first |4 * kPixels| bytes of data to |pixels|. https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:58: test = SkColorSpace::MakeICC(data, size); I've taken a look at MakeICC implementation. The only thing it does with the buffer provided is memcpy to an internal buffer. Thus, it would be fine to use: test = SkColorSpace::MakeICC(data + 4 * kPixels, size - 4 * kPixels); without creating an additional buffer/string/whatever.
https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:60: test = SkColorSpace::MakeICC(data, size); On 2017/04/05 14:14:07, noel gordon wrote: > On 2017/04/04 13:43:06, mmoroz wrote: > > As first |4 * kPixels| bytes of |data| will be used for initialization of data > > used in |ColorTransform| function, we should do here something like: > > > > std::vector<uint8_t> buffer(data + 4 * kPixels, buffer + size - 4 * kPixels); > > > > and then use |buffer.data()| and |buffer.size()| instead of |data| and |size|. > > That's important to have these bytes in a separate buffer to be able catch > > out-of-bound access to the left. Thus, we cannot just use |data + 4 * kPixels| > > and |size - 4 * kPixels| without creating a separate buffer. > > With the new design, I'm not sure any of this applies. In thinking about a png > decoder, for example, which creates many internal structures to read/write data > from, I be surprised if a fuzzer could not spot any out-of-bounds reads or > writes on said structures (none of which are wrapped in a std::vector). > If the target creates internal structures with their own buffers, this is fine. Unless there are bugs in the initialization code which creates those internal structures. What I'm speaking about is cases like the following one: void VulnerableTarget(data, size) { <...> data[-1] = 0; <-- OOB access } LLVMFuzzerTestOneInput(data, size) { if (size < 5) return 0; <...> VulnerableTarget(data + 5, size - 5); } In this case, the OOB access won't be detected because it happens inside the accessible memory actually.
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: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1400 execs/s on Mac Air and 2100 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1200 execs/s on Mac Air and 2000 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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 ==========
On 2017/04/06 08:26:37, mmoroz wrote: > Looks good, but we should remove hacky |GeneratePixels| method and use a part of > the data instead. FYI this "hacky" thing you speak of has been used elsewhere in google to score 6 fuzz trophies on Color Management System software. You should perhaps be more careful with your language. > https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... > File testing/libfuzzer/fuzzers/BUILD.gn (right): > > https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/B... > testing/libfuzzer/fuzzers/BUILD.gn:423: fuzzer_test("skia_color_space_fuzzer") { > On 2017/04/05 21:02:47, scroggo_chromium wrote: > > On 2017/04/05 14:14:07, noel gordon wrote: > > > On 2017/04/04 13:43:06, mmoroz wrote: > > > > Could you please move both the source code and the GN target to > > > > third_party/skia/ ? > > > > > > Don't really work on Skia myself. > > > > > > I know that we have several fuzz targets for skia here > > > > > > > > > > (https://cs.chromium.org/search/?q=file:skia.*+file:%5Esrc/testing/libfuzzer/f...), > > > > but this is bad and we actually should move them to the skia project > > directory > > > > as well. > > > > > > Noticed there were a few skia fuzzer targets in the current BUILD file too. > > So > > > that was a bit confusing to me, but perhaps another bug could move them all > > into > > > Skia. > > > > > > Currently I want to share/use the color_space_data.h use another fuzzer I'm > > > building (for qcms which has nothing to do with skia), so for now I'd like > to > > do > > > that. Where these fuzzers should eventually end up living is a good question > > > though, but not one I'm gonna tackle. Perhaps after the fuzzers are built, > we > > > can decide on that, as a subject of another bug. > > > > +1. FWIW, I do not think it would make sense to put the fuzzers in > > third_party/skia, which is its own independent repository, since they depend > on > > other Chromium code. Code inside the Skia repository should be runnable > outside > > of Chromium. If this is not a good place for them, maybe src/skia is better? > > > > Skia has its own fuzzers though, and I wouldn't be opposed to adding a fuzzer > in > > Skia which uses Skia's infrastructure. But it would be nice to be able to > reuse > > for other fuzzers as well. > > Makes sense. Thanks for the explanation! > > https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... > File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): > > https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... > testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:33: uint32_t data[kPixels * > 4]; > On 2017/04/05 14:14:07, noel gordon wrote: > > On 2017/04/04 13:43:06, mmoroz wrote: > > > On 2017/04/04 12:23:16, noel gordon wrote: > > > > This is a fine source of random pixel data, and contains whatever is on > the > > > data > > > > stack at this location. > > > > > > > > If this fuzzer also gets run in MSAN though, I suspect that MSAN might > > > complain > > > > that the data is uninitialized, if/when it is read by the color transform > > code > > > > during transform->apply(). > > > > > > > > How do we tell MSAN to stfu about that, and just accept the uninitialized > > > reads > > > > from the data variable? > > > > > > I'm afraid that we cannot rely on this approach because: > > > 1) all the testcases generated by the fuzzer should be fully reproducible if > > we > > > re-run them, i.e. we cannot use a random data from a random memory region > > > > IC, we can fix that ... > > > > > 2) this might be looking as a hacky though efficient solution, but we > > shouldn't > > > use bad coding patterns anyway > > > > I created a deterministic "random" pixel generator. > > > > > Since you always need a constant number of bytes here, I'd recommend to > simply > > > use first |4 * kPixels| bytes of |data| coming from fuzzer. > > > > Note the input data can be less than 4 * kPixels. > > > > Right, but something like: > if (size < 4 * kPixels) return 0; > > in the beginning of LLVMFuzzerTestOneInput function will help. LibFuzzer is able > to quickly understand that inputs shorter than that limit do no make sense and > won't spend time on them. As mentioned, 128 is the correct minimum for a color profile. Many value profiles are less than 4 * kPixels. > https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... > testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:64: switch (size & 7) { // > Create profile to transform pixels to/from. > On 2017/04/05 14:14:07, noel gordon wrote: > > On 2017/04/04 13:43:07, mmoroz wrote: > > > Nice trick, but it would be better to avoid relying only on |size| here, > > > otherwise you would never get an input of length |8 * n| to be processed by > > > cases other than case 0, and so forth. > > > > Not some a trick. The distribution of size & 7 tends to uniform as the corpus > > grows. The hash idea seems fine, but it does add measurable overhead which > > decreases exec/s ... > > > > > Our current recommendation for such cases is to use |std::hash| value > > calculated > > > from |data|, e.g. > > > > > > https://cs.chromium.org/search/?q=file:fuzzer.*.cc+std::hash+package:%5Echrom... > > > > Hmmm: looking at the link, copying the input data into a std::string<> just to > > compute a std::hash wastes too time and is maybe not a good idea for color > > profile data. > > > > A better approach is to 1) use base::StringPiece and 2) compute a fixed-sized > > hash (we know that input data size is at least 128 now) to maintain > performance > > and provide a reasonable hash. > > The distribution of size & 7 doesn't matter because all inputs of the following > sizes: > 8, 16, 24, 32, ..., 1024, ..., 2048, ... 8 * n > > will never go into any of the cases except of the case 0. > > The same applies to inputs of sizes (1 + 8 * n) as they will always visit only > case 1 branch, and so on. This is bad for coverage. I use hash for the selection. I also compute these profiles once-only now: the input data is being tested, not these profiles. > If hash is a performance concern here, you can extract one byte from the data > for flags randomization (example: > https://cs.chromium.org/chromium/src/third_party/sqlite/fuzz/sqlite3_prepare_...) IC. Perhaps the trouble with profiles is the many bytes in the first 128 are 0. The first byte is very likely 0. I will just use the hash. > StringPieceHash looks fine to me as well. Yes avoiding the copy is good, and I changed it use the entire size of the input data so the hash is uniformly distributed (which is the result I get when I measure the hash distribution). StringPieceHash is also where all the performance goes now, but we do need a good hash for other reasons (selections).
https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:23: pixels[i] = (memory = memory << 3 ^ kSRGBData[1024 + i % 2048]); On 2017/04/06 08:26:37, mmoroz wrote: > It looks like |pixels| will be always the same under this condition. Why not to > put this into a constant then? Simplicity. https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:26: pixels[i] = (memory = memory << 3 ^ *data++); On 2017/04/06 08:26:37, mmoroz wrote: > This creates a dependency between |pixels| and |SkColorSpace| being created on > line 58. This is bad for coverage. The only thing we allow ourselves in terms of > "re-using the data" is to calculate a hash for flags randomization. In other > cases we should split the data and use different parts of it independently. I have replaced all this with prng based on std::mt19937_64, seeded by hash, and measured that it is as fast as doing: pixels[i] = (memory = memory << 3 ^ *data++); and that it provides uncorrelated random data. The pixels buffer is allocated now (once-only too for an amortized cost of 0) so that out of bounds is detectable. And I checked: the fuzzer indeed complains if I access pixels out of bounds, no need to wrap in a std::vector<> and incur the copy costs that would result on every test run. I did the same (a once-only allocation) for the output pixels of the color transform. Again, the fuzzer complained if I wrote out of its bounds in my testing. https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:55: if (size < 128 || size > kSizeLimit) On 2017/04/06 08:26:37, mmoroz wrote: > Let's use lower boundary of |128 + 4 * kPixels| here and copy first |4 * > kPixels| bytes of data to |pixels|. Again, many valid profiles are less that 4 * kPixels in size, so let's not do this :) The current max/min bounds are sensible for color profiles. https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:58: test = SkColorSpace::MakeICC(data, size); On 2017/04/06 08:26:37, mmoroz wrote: > I've taken a look at MakeICC implementation. The only thing it does with the > buffer provided is memcpy to an internal buffer. More data copying, how very sad. "Copies. Copies everywhere!", Buzz Lightyear notes. > Thus, it would be fine to use: > > test = SkColorSpace::MakeICC(data + 4 * kPixels, size - 4 * kPixels); > Let's just shove the data in (data, size) and not fiddle with it (since 4 * kPixels is unrelated to it in the new code).
On 2017/04/06 08:31:27, mmoroz wrote: > https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... > File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): > > https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... > testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:60: test = > SkColorSpace::MakeICC(data, size); > On 2017/04/05 14:14:07, noel gordon wrote: > > On 2017/04/04 13:43:06, mmoroz wrote: > > > As first |4 * kPixels| bytes of |data| will be used for initialization of > data > > > used in |ColorTransform| function, we should do here something like: > > > > > > std::vector<uint8_t> buffer(data + 4 * kPixels, buffer + size - 4 * > kPixels); > > > > > > and then use |buffer.data()| and |buffer.size()| instead of |data| and > |size|. > > > That's important to have these bytes in a separate buffer to be able catch > > > out-of-bound access to the left. Thus, we cannot just use |data + 4 * > kPixels| > > > and |size - 4 * kPixels| without creating a separate buffer. > > > > With the new design, I'm not sure any of this applies. In thinking about a > png > > decoder, for example, which creates many internal structures to read/write > data > > from, I be surprised if a fuzzer could not spot any out-of-bounds reads or > > writes on said structures (none of which are wrapped in a std::vector). > > > > If the target creates internal structures with their own buffers, this is fine. > Unless there are bugs in the initialization code which creates those internal > structures. > > What I'm speaking about is cases like the following one: > > void VulnerableTarget(data, size) { > <...> > data[-1] = 0; <-- OOB access > } Good. > LLVMFuzzerTestOneInput(data, size) { > if (size < 5) return 0; > <...> > VulnerableTarget(data + 5, size - 5); > } > > In this case, the OOB access won't be detected because it happens inside the > accessible memory actually. Hard to detect case. Perhaps the target being tested is written in C where, "stuff"[-1] is legal, but within the range of data provided. Not really applicable here since the data is passed directly through to the target (MakeICC). If we shifted the input data + 5, and passed that in, then the magic numbers in the data format are moved and the target would immediately reject the data. The fuzzer does that for us (moves data bytes around, I believe), so no real need for us to do it I think.
mmoroz@chromium.org changed reviewers: + kcc@chromium.org, mbarbella@chromium.org, ochang@chromium.org
Noel, please excuse me if "hacky" sounds offensive to you. I've seen it multiple times in codereviews (https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=...) as well as across the codebase (https://cs.chromium.org/search/?q=%22hacky%22+package:%5Echromium$&type=cs) to describe solutions / implementations that have some mistakes / omissions / bad practices / etc. Glad to hear that that implementation of GeneratePixels has helped to find bugs previously. However, it doesn't show whether the implementation is good or bad. It only shows that the software had bugs. To finally clarify this, I was not evaluating the quality of the generator itself. I assume that it works fairly good for its purposes, so I wasn't complaining on that. My concern was regarding re-usage of the same |data| bytes. Since we shouldn't re-use them (except of a hash-based approach), any implementation doing so would be considered as sub-optimal. I hope that 'sub-optimal' sounds more friendly to you :) --------- Regarding std::mt19937_64, I think we haven't used it before, but it looks interesting, especially since you ensured that performance is fine. Thanks for adding this. https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:15: static constexpr unsigned kPixels = 2048 / 4; Please use 'size_t' instead of 'unsigned' as per https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:21: pixels = new uint32_t[kPixels * 4]; If you tend to Simplicity, why not: static uint32_t pixels[kPixels * 4]; ? https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:34: DCHECK(test.get()); Since we are fuzzing both Release and Debug builds, we have two options regarding asserts in fuzz targets: A) use 'CHECK' if its failure means a bug in the target B) do not use asserts at all, if it failure doesn't mean a bug in the target https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:41: static uint32_t* output = new uint32_t[kPixels * 4]; Since this buffer has constant size, let's use a stack-based buffer or a static global one. It's simpler and usually it's faster.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
By the way, what does 'IC' mean?
On 2017/04/07 13:39:08, mmoroz wrote: > Noel, please excuse me if "hacky" sounds offensive to you. I've seen it multiple > times in codereviews No offense taken on my part, don't worry about it. We can't speak for how others might feel, though. So engineers don't use such terms given that they can unintentionally give offense to others, often without our knowing. > Glad to hear that that implementation of GeneratePixels has helped to find bugs > previously. However, it doesn't show whether the implementation is good or bad. > It only shows that the software had bugs. + or that the implementation was good enough to find said bugs :) > To finally clarify this, I was not evaluating the quality of the generator > itself. I assume that it works fairly good for its purposes, so I wasn't > complaining on that. My concern was regarding re-usage of the same |data| bytes. > Since we shouldn't re-use them (except of a hash-based approach), any > implementation doing so would be considered as sub-optimal. I hope that > 'sub-optimal' sounds more friendly to you :) The data was being used in two ways, once for the profile input of course, and also to inject randomness in pixel data. Seems to work for color profiles, so maybe no need to have concerns about reuse in this case, based on the existing results. (And it helped me find 2 issues in the current code for the record). Optimal or not? We can't really say precisely since the term "optimality" only has meaning when it w.r.t. a criteria. Being fast and finding bugs could be a criteria, amongst many others I could think of. > --------- > > Regarding std::mt19937_64, I think we haven't used it before, but it looks > interesting, especially since you ensured that performance is fine. Thanks for > adding this. Nope, I saw no uses of std::mt19937_64 in other fuzzers. And I wasn't expecting it (and the other new prng tools in C++11 I tested) to be fast. I instead found that std::mt19937_64 was very good speed-wise (compared to the xor memory shifter method). Interesting result.
https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:15: static constexpr unsigned kPixels = 2048 / 4; On 2017/04/07 13:39:08, mmoroz wrote: > Please use 'size_t' instead of 'unsigned' as per > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:21: pixels = new uint32_t[kPixels * 4]; On 2017/04/07 13:39:08, mmoroz wrote: > If you tend to Simplicity, why not: static uint32_t pixels[kPixels * 4]; ? I had it in a static array originally, right? Your review comments suggested to me that the fuzzer would not detect out-of-bounds access to this static pixel data? Given that, I went ahead and made it a once-only new instead, and tested that out-of-bounds access was detected by the fuzzer (and it sure was detected so problem solved, or at least I thought it was ... https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:34: DCHECK(test.get()); On 2017/04/07 13:39:08, mmoroz wrote: > Since we are fuzzing both Release and Debug builds, we have two options > regarding asserts in fuzz targets: > > A) use 'CHECK' if its failure means a bug in the target > > B) do not use asserts at all, if it failure doesn't mean a bug in the target Hmmm maybe C) use a DCHECK to state the code contract (as we do often in chrome)? |test| should never NULL in this case. DEBUG builds are not really about speed either, I suspect, and I doubt this DCHECK matters for speed-reasons. It states only what we expect to be true. Saved me a few times while developing this code btw. https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:41: static uint32_t* output = new uint32_t[kPixels * 4]; On 2017/04/07 13:39:08, mmoroz wrote: > Since this buffer has constant size, let's use a stack-based buffer or a static > global one. It's simpler and usually it's faster. Again, it was a static before, and I've made comments above about how the previous review comments convinced me to change it from that to something else. I used a static new. Speed-wise, I measured, and nope, a static new does not slow us down at all (good), and out-of-bounds access was correctly detected (good).
On 2017/04/07 13:44:12, mmoroz wrote: > By the way, what does 'IC' mean? IC == I see :)
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.
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.
https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:21: pixels = new uint32_t[kPixels * 4]; On 2017/04/07 16:03:03, noel gordon wrote: > On 2017/04/07 13:39:08, mmoroz wrote: > > If you tend to Simplicity, why not: static uint32_t pixels[kPixels * 4]; ? > > I had it in a static array originally, right? Your review comments suggested to > me that the fuzzer would not detect out-of-bounds access to this static pixel > data? Given that, I went ahead and made it a once-only new instead, and tested > that out-of-bounds access was detected by the fuzzer (and it sure was detected > so problem solved, or at least I thought it was ... > Yes, it was a static global buffer in patchset #2 (https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze...) and I wasn't complaining about that. OOB access has been discussed in the patchset #1 with regards to line 60 and the |data| buffer coming from fuzzing engine (https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s...). https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:34: DCHECK(test.get()); On 2017/04/07 16:03:03, noel gordon wrote: > On 2017/04/07 13:39:08, mmoroz wrote: > > Since we are fuzzing both Release and Debug builds, we have two options > > regarding asserts in fuzz targets: > > > > A) use 'CHECK' if its failure means a bug in the target > > > > B) do not use asserts at all, if it failure doesn't mean a bug in the target > > Hmmm maybe C) use a DCHECK to state the code contract (as we do often in > chrome)? > > |test| should never NULL in this case. DEBUG builds are not really about speed > either, I suspect, and I doubt this DCHECK matters for speed-reasons. It states > only what we expect to be true. Saved me a few times while developing this code > btw. Since "|test| should never NULL in this case", any scenario when |test| is NULL should be a bug, right? If so, we must be able to catch it in both Release and Debug builds, i.e. need to use CHECK. If |test| is NULL doesn't mean a bug in the target, we shouldn't use neither CHECK nor DCHECK. In that case it would be fine to "return 0;" to reject inputs causing such cases. https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:41: static uint32_t* output = new uint32_t[kPixels * 4]; On 2017/04/07 16:03:02, noel gordon wrote: > On 2017/04/07 13:39:08, mmoroz wrote: > > Since this buffer has constant size, let's use a stack-based buffer or a > static > > global one. It's simpler and usually it's faster. > > Again, it was a static before, and I've made comments above about how the > previous review comments convinced me to change it from that to something else. > I used a static new. Speed-wise, I measured, and nope, a static new does not > slow us down at all (good), and out-of-bounds access was correctly detected > (good). I've just checked two previous patchsets and this never was a static buffer. See PS#1: https://codereview.chromium.org/2797473003/diff/1/testing/libfuzzer/fuzzers/s... It was a stack-based buffer: "uint32_t data[kPixels * 4];" And PS#2: https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... Re-named stack-based buffer: "uint32_t output[kPixels * 4];" Moreover, I haven't left any comments regarding that buffer previously, as stack-based buffer with a constant size is totally fine. Though I'd also initialize it like "uint32_t output[kPixels * 4] = { 0 };", but it is not necessary in this case.
Copied the last round of comments to the latest patchset (for easier fixing / checking). LGTM once the comments addressed. FYI, I'll be travelling since tomorrow. Thanks for this tough discussion :) https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:17: static uint32_t* pixels = nullptr; Please make this static global buffer as it has been in PS#2, line 17: https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:20: if (!pixels) This won't be needed once the comment above addressed. https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:34: DCHECK(test.get()); As discussed before (line 34 at https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze...), I highly recommend to use CHECK() or replace this with 'if (!test.get()) return 0;'. https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:41: static uint32_t* output = new uint32_t[kPixels * 4]; Please move it to global the same way as for |pixels| array: static uint32_t output[kPixels * 4];
https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc (right): https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:17: static uint32_t* pixels = nullptr; On 2017/04/08 11:10:59, mmoroz (OOO) wrote: > Please make this static global buffer as it has been in PS#2, line 17: > https://codereview.chromium.org/2797473003/diff/20001/testing/libfuzzer/fuzze... Done. https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:20: if (!pixels) On 2017/04/08 11:10:59, mmoroz (OOO) wrote: > This won't be needed once the comment above addressed. Done. https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:34: DCHECK(test.get()); On 2017/04/08 11:10:59, mmoroz (OOO) wrote: > As discussed before (line 34 at > https://codereview.chromium.org/2797473003/diff/40001/testing/libfuzzer/fuzze...), > I highly recommend to use CHECK() or replace this with 'if (!test.get()) return > 0;'. Done. https://codereview.chromium.org/2797473003/diff/80001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc:41: static uint32_t* output = new uint32_t[kPixels * 4]; On 2017/04/08 11:10:59, mmoroz (OOO) wrote: > Please move it to global the same way as for |pixels| array: > > static uint32_t output[kPixels * 4]; Done.
The CQ bit was checked by noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmoroz@chromium.org Link to the patchset: https://codereview.chromium.org/2797473003/#ps100001 (title: "rm -f DCHECK, use static buffers.")
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: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1200 execs/s on Mac Air and 2000 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1100 execs/s on Mac Air and 1900 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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 ==========
On 2017/04/08 11:10:59, mmoroz (OOO) wrote: > Copied the last round of comments to the latest patchset (for easier fixing / > checking). LGTM once the comments addressed. Thanks, all done. > FYI, I'll be travelling since tomorrow. Thanks for this tough discussion :) You're welcome. Happy travels (and thanks for letting me know).
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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": 100001, "attempt_start_ts": 1491780031879650,
"parent_rev": "ea2b51465a6fad6d53324a7238c4407e572dab97", "commit_rev":
"373b1ecb464977173aa74566bb6f672be10df687"}
Message was sent while issue was closed.
Description was changed from ========== Add LLVM fuzzer: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1100 execs/s on Mac Air and 1900 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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: Skia color space and color transform Add common color profile data file color_space_data.h, which is a separate file so it can be included in other fuzzers. 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. Speed: achieves ~1100 execs/s on Mac Air and 1900 execs/s on my Mac Pro. Seed corpus of ICC color profiles for the fuzzer uploaded to the skia_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 Review-Url: https://codereview.chromium.org/2797473003 Cr-Commit-Position: refs/heads/master@{#463156} Committed: https://chromium.googlesource.com/chromium/src/+/373b1ecb464977173aa74566bb6f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/373b1ecb464977173aa74566bb6f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
