|
|
DescriptionAdd XFA pdfium fuzzer
This CL updates the pdfium_fuzzer to also build in the needed XFA code.
The BUILD file is changed to only build pdfium_fuzzer if we're not
in XFA and build pdfium_xfa_fuzzer if we're in XFA.
Review-Url: https://codereview.chromium.org/2754103002
Cr-Commit-Position: refs/heads/master@{#459186}
Committed: https://chromium.googlesource.com/chromium/src/+/034ca9381180401b9b25eac088babf7fdae847d8
Patch Set 1 #
Total comments: 2
Patch Set 2 : Review feedback #Patch Set 3 : Split fuzzer for XFA #Patch Set 4 : Bail early in XFA if non-xfa doc #Patch Set 5 : Seed corpus #Patch Set 6 : Rebase to master #Patch Set 7 : seed corpus #
Messages
Total messages: 37 (11 generated)
dsinclair@chromium.org changed reviewers: + mbarbella@chromium.org, thestig@chromium.org, tsepez@chromium.org
PTAL. mbarbella@ will this work like I want it too? The other fuzzers run in Clusterfuzz now, so I'm trying to get one that runs over XFA files. To do that, I updated the pdfium_fuzzer and changed the build rules around so it will build pdfium_fuzzer in non-xfa and pdfium_xfa_fuzzer in xfa. (Without the !xfa check we'd end up building an xfa fuzzer out of the non-xfa file). Will this do the right thing with clusterfuzz where we'll have pdfium_fuzzer running like now and have a pdfium_xfa_fuzzer that runs XFA files?
The CQ bit was checked by dsinclair@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...
LGTM, but make sure Marty is OK with this too.
https://codereview.chromium.org/2754103002/diff/1/pdf/pdfium/fuzzers/BUILD.gn File pdf/pdfium/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2754103002/diff/1/pdf/pdfium/fuzzers/BUILD.gn... pdf/pdfium/fuzzers/BUILD.gn:106: # If XFA is enabled in Chrome, this fuzzer becomes the same as the Do you think it might be better to put this entry and pdfium_fuzzer entry in their own separate if-else block?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2754103002/diff/1/pdf/pdfium/fuzzers/BUILD.gn File pdf/pdfium/fuzzers/BUILD.gn (right): https://codereview.chromium.org/2754103002/diff/1/pdf/pdfium/fuzzers/BUILD.gn... pdf/pdfium/fuzzers/BUILD.gn:106: # If XFA is enabled in Chrome, this fuzzer becomes the same as the On 2017/03/16 18:52:19, Lei Zhang (super slow) wrote: > Do you think it might be better to put this entry and pdfium_fuzzer entry in > their own separate if-else block? Done.
lgtm
Martin, ping.
Description was changed from ========== Add XFA pdfium fuzzer This CL updates the pdfium_fuzzer to also build in the needed XFA code. The BUILD file is changed to only build pdfium_fuzzer if we're not in XFA and build pdfium_xfa_fuzzer if we're in XFA. ========== to ========== Add XFA pdfium fuzzer This CL updates the pdfium_fuzzer to also build in the needed XFA code. The BUILD file is changed to only build pdfium_fuzzer if we're not in XFA and build pdfium_xfa_fuzzer if we're in XFA. ==========
On 2017/03/21 13:10:04, dsinclair wrote: > Martin, ping. Sorry for the slow reply. Assuming I understand this correctly I don't think it would just work, but I'm not the best reviewer for this. I'd expect that the builder would only have one of the configurations, and only one of the two targets would be added to the archive. +mmoroz, ochang: Do either of you have any ideas for how we might be able to do this?
On 2017/03/21 20:42:07, Martin Barbella wrote: > On 2017/03/21 13:10:04, dsinclair wrote: > > Martin, ping. > > Sorry for the slow reply. > > Assuming I understand this correctly I don't think it would just work, but I'm > not the best reviewer for this. I'd expect that the builder would only have one > of the configurations, and only one of the two targets would be added to the > archive. > > +mmoroz, ochang: Do either of you have any ideas for how we might be able to do > this? Does it pull all the fuzzers from just one builder? There is XFA code which the fuzzers are running for that would only get built on certain bots. I can achieve the same result by duplicating the pdfium_fuzzer and making an explicit XFA one and then building both. Duplicate code is annoying, but would be pretty easy.
On 2017/03/21 20:46:46, dsinclair wrote: > On 2017/03/21 20:42:07, Martin Barbella wrote: > > On 2017/03/21 13:10:04, dsinclair wrote: > > > Martin, ping. > > > > Sorry for the slow reply. > > > > Assuming I understand this correctly I don't think it would just work, but I'm > > not the best reviewer for this. I'd expect that the builder would only have > one > > of the configurations, and only one of the two targets would be added to the > > archive. > > > > +mmoroz, ochang: Do either of you have any ideas for how we might be able to > do > > this? > > > Does it pull all the fuzzers from just one builder? There is XFA code which the > fuzzers are running for that would only get built on certain bots. > > I can achieve the same result by duplicating the pdfium_fuzzer and making an > explicit XFA one and then building both. Duplicate code is annoying, but would > be pretty easy. We have multiple builders but the difference between them tend to be memory debugging tools. For each of them there's some manual setup in ClusterFuzz and having a very specialized builder would be problematic. Max and Oliver, if either of you have any ideas for cleaner solutions please let us know. Duplicating the code sounds better than anything else I can think of at the moment since at least everything would just work on the ClusterFuzz side.
On 2017/03/21 22:37:10, Martin Barbella wrote: > On 2017/03/21 20:46:46, dsinclair wrote: > > On 2017/03/21 20:42:07, Martin Barbella wrote: > > > On 2017/03/21 13:10:04, dsinclair wrote: > > > > Martin, ping. > > > > > > Sorry for the slow reply. > > > > > > Assuming I understand this correctly I don't think it would just work, but > I'm > > > not the best reviewer for this. I'd expect that the builder would only have > > one > > > of the configurations, and only one of the two targets would be added to the > > > archive. > > > > > > +mmoroz, ochang: Do either of you have any ideas for how we might be able to > > do > > > this? > > > > > > Does it pull all the fuzzers from just one builder? There is XFA code which > the > > fuzzers are running for that would only get built on certain bots. > > > > I can achieve the same result by duplicating the pdfium_fuzzer and making an > > explicit XFA one and then building both. Duplicate code is annoying, but would > > be pretty easy. > > We have multiple builders but the difference between them tend to be memory > debugging tools. For each of them there's some manual setup in ClusterFuzz and > having a very specialized builder would be problematic. > > Max and Oliver, if either of you have any ideas for cleaner solutions please let > us know. Duplicating the code sounds better than anything else I can think of at > the moment since at least everything would just work on the ClusterFuzz side. We only have one builder for these fuzz targets, and we force XFA on for them. So, pdfium_fuzzer is already being built with PDF_ENABLE_XFA (but without the changes you made in the CL).
On 2017/03/21 22:46:52, Oliver Chang wrote: > On 2017/03/21 22:37:10, Martin Barbella wrote: > > On 2017/03/21 20:46:46, dsinclair wrote: > > > On 2017/03/21 20:42:07, Martin Barbella wrote: > > > > On 2017/03/21 13:10:04, dsinclair wrote: > > > > > Martin, ping. > > > > > > > > Sorry for the slow reply. > > > > > > > > Assuming I understand this correctly I don't think it would just work, but > > I'm > > > > not the best reviewer for this. I'd expect that the builder would only > have > > > one > > > > of the configurations, and only one of the two targets would be added to > the > > > > archive. > > > > > > > > +mmoroz, ochang: Do either of you have any ideas for how we might be able > to > > > do > > > > this? > > > > > > > > > Does it pull all the fuzzers from just one builder? There is XFA code which > > the > > > fuzzers are running for that would only get built on certain bots. > > > > > > I can achieve the same result by duplicating the pdfium_fuzzer and making an > > > explicit XFA one and then building both. Duplicate code is annoying, but > would > > > be pretty easy. > > > > We have multiple builders but the difference between them tend to be memory > > debugging tools. For each of them there's some manual setup in ClusterFuzz and > > having a very specialized builder would be problematic. > > > > Max and Oliver, if either of you have any ideas for cleaner solutions please > let > > us know. Duplicating the code sounds better than anything else I can think of > at > > the moment since at least everything would just work on the ClusterFuzz side. > > We only have one builder for these fuzz targets, and we force XFA on for them. > So, pdfium_fuzzer is already being built with PDF_ENABLE_XFA (but without the > changes you made in the CL). pdfium_test on the other hand is built with the default XFA flag (off). Does this work for you?
On 2017/03/21 22:46:52, Oliver Chang wrote: > On 2017/03/21 22:37:10, Martin Barbella wrote: > > On 2017/03/21 20:46:46, dsinclair wrote: > > > On 2017/03/21 20:42:07, Martin Barbella wrote: > > > > On 2017/03/21 13:10:04, dsinclair wrote: > > > > > Martin, ping. > > > > > > > > Sorry for the slow reply. > > > > > > > > Assuming I understand this correctly I don't think it would just work, but > > I'm > > > > not the best reviewer for this. I'd expect that the builder would only > have > > > one > > > > of the configurations, and only one of the two targets would be added to > the > > > > archive. > > > > > > > > +mmoroz, ochang: Do either of you have any ideas for how we might be able > to > > > do > > > > this? > > > > > > > > > Does it pull all the fuzzers from just one builder? There is XFA code which > > the > > > fuzzers are running for that would only get built on certain bots. > > > > > > I can achieve the same result by duplicating the pdfium_fuzzer and making an > > > explicit XFA one and then building both. Duplicate code is annoying, but > would > > > be pretty easy. > > > > We have multiple builders but the difference between them tend to be memory > > debugging tools. For each of them there's some manual setup in ClusterFuzz and > > having a very specialized builder would be problematic. > > > > Max and Oliver, if either of you have any ideas for cleaner solutions please > let > > us know. Duplicating the code sounds better than anything else I can think of > at > > the moment since at least everything would just work on the ClusterFuzz side. > > We only have one builder for these fuzz targets, and we force XFA on for them. > So, pdfium_fuzzer is already being built with PDF_ENABLE_XFA (but without the > changes you made in the CL). In that case, I'll duplicate the code and create an XFA version of the fuzzer. That way we'll have both running.
On 2017/03/22 00:44:46, dsinclair wrote: > On 2017/03/21 22:46:52, Oliver Chang wrote: > > On 2017/03/21 22:37:10, Martin Barbella wrote: > > > On 2017/03/21 20:46:46, dsinclair wrote: > > > > On 2017/03/21 20:42:07, Martin Barbella wrote: > > > > > On 2017/03/21 13:10:04, dsinclair wrote: > > > > > > Martin, ping. > > > > > > > > > > Sorry for the slow reply. > > > > > > > > > > Assuming I understand this correctly I don't think it would just work, > but > > > I'm > > > > > not the best reviewer for this. I'd expect that the builder would only > > have > > > > one > > > > > of the configurations, and only one of the two targets would be added to > > the > > > > > archive. > > > > > > > > > > +mmoroz, ochang: Do either of you have any ideas for how we might be > able > > to > > > > do > > > > > this? > > > > > > > > > > > > Does it pull all the fuzzers from just one builder? There is XFA code > which > > > the > > > > fuzzers are running for that would only get built on certain bots. > > > > > > > > I can achieve the same result by duplicating the pdfium_fuzzer and making > an > > > > explicit XFA one and then building both. Duplicate code is annoying, but > > would > > > > be pretty easy. > > > > > > We have multiple builders but the difference between them tend to be memory > > > debugging tools. For each of them there's some manual setup in ClusterFuzz > and > > > having a very specialized builder would be problematic. > > > > > > Max and Oliver, if either of you have any ideas for cleaner solutions please > > let > > > us know. Duplicating the code sounds better than anything else I can think > of > > at > > > the moment since at least everything would just work on the ClusterFuzz > side. > > > > We only have one builder for these fuzz targets, and we force XFA on for them. > > So, pdfium_fuzzer is already being built with PDF_ENABLE_XFA (but without the > > changes you made in the CL). > > > In that case, I'll duplicate the code and create an XFA version of the fuzzer. > That way we'll have both running. Duplicate the target for for different flags SGTM. We have something similar in https://codereview.chromium.org/2762763002/
PTAL. This creates a new fuzzer which will do the LoadXFA call. Do we need to upload any seed documents or anything or will clusterfuzz figure it out? Note, the XFA fuzzer will crash in V8 if run due to a has_named_interceptor check failure.
mmoroz@chromium.org changed reviewers: + mmoroz@chromium.org
LGTM (though I'm note familiar with pdfium API). Regarding corpus, it doesn't happen automatically, but I've just duplicated existing corpus via: $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer and $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer_static gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer_static No need to worry about corpus anymore :)
On 2017/03/22 15:23:30, mmoroz wrote: > LGTM (though I'm note familiar with pdfium API). > > Regarding corpus, it doesn't happen automatically, but I've just duplicated > existing corpus via: > > $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer > gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer > > and > > $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer_static > gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer_static > > No need to worry about corpus anymore :) Do you know if any of the samples in those corpus's have XFA content? Or will clusterfuzz just figure out how to make XFA content?
On 2017/03/22 15:24:43, dsinclair wrote: > On 2017/03/22 15:23:30, mmoroz wrote: > > LGTM (though I'm note familiar with pdfium API). > > > > Regarding corpus, it doesn't happen automatically, but I've just duplicated > > existing corpus via: > > > > $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer > > gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer > > > > and > > > > $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer_static > > gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer_static > > > > No need to worry about corpus anymore :) > > > > Do you know if any of the samples in those corpus's have XFA content? Or will > clusterfuzz just figure out how to make XFA content? I don't know, to be honest. However, it won't harm if could upload some samples to https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... If you have samples in Chromium repository, you can also use seed_corpus GN attribute instead of manual upload (https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/eff...)
On 2017/03/22 15:28:20, mmoroz wrote: > On 2017/03/22 15:24:43, dsinclair wrote: > > On 2017/03/22 15:23:30, mmoroz wrote: > > > LGTM (though I'm note familiar with pdfium API). > > > > > > Regarding corpus, it doesn't happen automatically, but I've just duplicated > > > existing corpus via: > > > > > > $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer > > > gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer > > > > > > and > > > > > > $ gsutil -m rsync gs://clusterfuzz-corpus/libfuzzer/pdfium_fuzzer_static > > > gs://clusterfuzz-corpus/libfuzzer/pdfium_xfa_fuzzer_static > > > > > > No need to worry about corpus anymore :) > > > > > > > > Do you know if any of the samples in those corpus's have XFA content? Or will > > clusterfuzz just figure out how to make XFA content? > > I don't know, to be honest. However, it won't harm if could upload some samples > to > https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... > > If you have samples in Chromium repository, you can also use seed_corpus GN > attribute instead of manual upload > (https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/eff...) Added the seed_corpus entry. The directory only has 10 files, but it's at least a starting point, heh.
Nice! I believe that we can add it for pdifum_fuzzer as well.
On 2017/03/22 16:55:14, mmoroz wrote: > Nice! I believe that we can add it for pdifum_fuzzer as well. done.
The CQ bit was checked by dsinclair@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, thestig@chromium.org, mmoroz@chromium.org Link to the patchset: https://codereview.chromium.org/2754103002/#ps120001 (title: "seed corpus")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/23 18:44:54, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... We have another corpus: https://pdfium.googlesource.com/pdfium_tests/+/master which contains some XFA and other test files. It isn't checked out in chrome, but we could upload those to the folders as well if you wanted more seeds.
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490294644050690, "parent_rev": "d36c05b57d332e75f5e01a43bde92ed55ad54ba5", "commit_rev": "034ca9381180401b9b25eac088babf7fdae847d8"}
Message was sent while issue was closed.
Description was changed from ========== Add XFA pdfium fuzzer This CL updates the pdfium_fuzzer to also build in the needed XFA code. The BUILD file is changed to only build pdfium_fuzzer if we're not in XFA and build pdfium_xfa_fuzzer if we're in XFA. ========== to ========== Add XFA pdfium fuzzer This CL updates the pdfium_fuzzer to also build in the needed XFA code. The BUILD file is changed to only build pdfium_fuzzer if we're not in XFA and build pdfium_xfa_fuzzer if we're in XFA. Review-Url: https://codereview.chromium.org/2754103002 Cr-Commit-Position: refs/heads/master@{#459186} Committed: https://chromium.googlesource.com/chromium/src/+/034ca9381180401b9b25eac088ba... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/034ca9381180401b9b25eac088ba...
Message was sent while issue was closed.
On 2017/03/23 18:48:09, dsinclair wrote: > On 2017/03/23 18:44:54, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > We have another corpus: https://pdfium.googlesource.com/pdfium_tests/+/master > which contains some XFA and other test files. It isn't checked out in chrome, > but we could upload those to the folders as well if you wanted more seeds. Sure, I believe that having more corpus is better. Though an ideal approach would be to verify locally that those files increase the coverage. If they do, it would be great to have them either in Chromium repo or in https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... and https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... GCS directories.
Message was sent while issue was closed.
On 2017/03/31 13:24:23, mmoroz wrote: > On 2017/03/23 18:48:09, dsinclair wrote: > > On 2017/03/23 18:44:54, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > We have another corpus: https://pdfium.googlesource.com/pdfium_tests/+/master > > which contains some XFA and other test files. It isn't checked out in chrome, > > but we could upload those to the folders as well if you wanted more seeds. > > Sure, I believe that having more corpus is better. Though an ideal approach > would be to verify locally that those files increase the coverage. If they do, > it would be great to have them either in Chromium repo or in > https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... > and > https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... > GCS directories. Not sure what you mean by the chromium repo? We have them in their own repo so as to not increase the size of the pdfium repo. The files can be seen at https://pdfium.googlesource.com/pdfium_tests/+/master
Message was sent while issue was closed.
On 2017/04/03 13:43:28, dsinclair wrote: > On 2017/03/31 13:24:23, mmoroz wrote: > > On 2017/03/23 18:48:09, dsinclair wrote: > > > On 2017/03/23 18:44:54, commit-bot: I haz the power wrote: > > > > CQ is trying da patch. Follow status at > > > > > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > > > > We have another corpus: > https://pdfium.googlesource.com/pdfium_tests/+/master > > > which contains some XFA and other test files. It isn't checked out in > chrome, > > > but we could upload those to the folders as well if you wanted more seeds. > > > > Sure, I believe that having more corpus is better. Though an ideal approach > > would be to verify locally that those files increase the coverage. If they do, > > it would be great to have them either in Chromium repo or in > > > https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... > > and > > > https://pantheon.corp.google.com/storage/browser/clusterfuzz-corpus/libfuzzer... > > GCS directories. > > > Not sure what you mean by the chromium repo? We have them in their own repo so > as to not increase the size of the pdfium repo. The files can be seen at > https://pdfium.googlesource.com/pdfium_tests/+/master By chromium repo I meant: https://chromium.googlesource.com/chromium/chromium/ Unfortunately, we do not have a way to grab seed corpus from any other repository. Only that one or the GCS bucket. |