fuzzers: add a new fuzzer for patch parsing#5269
Conversation
pks-t
left a comment
There was a problem hiding this comment.
Totally agreed, parsing arbitrary input is always likely to lead to errors. So this is a very welcome addition to libgit2, thanks a lot! Some minor comments which should be rather easy to fix, but other than that it looks good to me! :)
I was looking at this code anyway because the sr.ht people nerdsniped me, and it gave me that "I should fuzz this" feeling. So have a fuzzer!
|
Also, in case you find it useful, I formatted the new c code with this clang-format config: It seems to roughly match what you're already doing (it's cribbed from Mercurial's config, which appears to be similar, I suspect they both originated with the Linux kernel.) |
| if (size) { | ||
| git_patch *patch = NULL; | ||
| git_patch_options opts = GIT_PATCH_OPTIONS_INIT; | ||
| opts.prefix_len = (uint32_t)data[0]; |
There was a problem hiding this comment.
Sorry, I somehow didn't see this in my first round, but I'm wondering about the first byte of data here. So you're skipping the first byte and use it as prefix_len, right? But judging from the fuzzing corpora, the first byte is simply part of the diff, and thus it is very likely that the initial line is a non-valid diff header already when skipping the first byte. So shouldn't we just leave the prefix_len alone and just use data as input for git_patch_from_buffer directly?
E.g. just:
git_patch *patch = NULL;
git_patch_from_buffer(&patch, data, size, NULL);
git_patch_free(patch);
|
The included corpora files should both have a leading 0x01. GitHub doesn't
seem to show that...
…On Fri, Oct 18, 2019, 02:28 Patrick Steinhardt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In fuzzers/patch_parse_fuzzer.c
<#5269 (comment)>:
> +{
+ UNUSED(argc);
+ UNUSED(argv);
+
+ if (git_libgit2_init() < 0)
+ abort();
+
+ return 0;
+}
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ if (size) {
+ git_patch *patch = NULL;
+ git_patch_options opts = GIT_PATCH_OPTIONS_INIT;
+ opts.prefix_len = (uint32_t)data[0];
Sorry, I somehow didn't see this in my first round, but I'm wondering
about the first byte of data here. So you're skipping the first byte and
use it as prefix_len, right? But judging from the fuzzing corpora, the
first byte is simply part of the diff, and thus it is *very* likely that
the initial line is a non-valid diff header already. So shouldn't we just
leave the prefix_len alone and just use data as input for
git_patch_from_buffer directly?
E.g. just:
git_patch *patch = NULL;
git_patch_from_buffer(&patch, data, size, NULL);
git_patch_free(patch);
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5269?email_source=notifications&email_token=AAAE6LOL7KFNM77NF2PPV6LQPFJO5A5CNFSM4JBCKD6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIM57KA#pullrequestreview-303685544>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAE6LKDXVLTJIJC6I4HN7DQPFJO5ANCNFSM4JBCKD6A>
.
|
Ah, fair. Not too sure about that though, to be honest. I somehow guess that the fuzzers will generate a lot less valid patches and thus impact speed, especially so because it's unlikely that the fuzzer will generate correct prefixes depending on this prefix byte. For simplicity's sake I'd say to not have that and just keep the default options. I assume there will be enough things to be fixed already. |
|
From experience fuzzing Mercurial's parsers, the fuzzing system is absurdly
good at figuring out prefix bytes like this. But if you feel strongly I can
rip it out. What's your preference?
…On Fri, Oct 18, 2019, 08:38 Patrick Steinhardt ***@***.***> wrote:
The included corpora files should both have a leading 0x01. GitHub doesn't
seem to show that...
Ah, fair. Not too sure about that though, to be honest. I somehow guess
that the fuzzers will generate a lot less valid patches and thus impact
speed, especially so because it's unlikely that the fuzzer will generate
correct prefixes depending on this prefix byte. For simplicity's sake I'd
say to not have that and just keep the default options. I assume there will
be enough things to be fixed already.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5269?email_source=notifications&email_token=AAAE6LORN2YJXBCL444TQNLQPGU4PA5CNFSM4JBCKD6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBUIFSY#issuecomment-543720139>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAE6LJMBHZL3FFJE2AEDFLQPGU4PANCNFSM4JBCKD6A>
.
|
Okay, that sounds promising, I didn't expect them to be that "intelligent". Let's go for it then, no need to muse about this more than necessary. We can still change it at a later point if required. Thanks a lot! |
I was looking at this code anyway because the sr.ht people nerdsniped
me, and it gave me that "I should fuzz this" feeling. So have a fuzzer!