Re: [RFC][PATCH] index-pack: add testcases found using AFL

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 10, 2017, 22:58 UTC
Message-ID: <CACBZZX5fGU9C-z94KbMAs_AegOSGtq8nbrkRe-NxBCHYsDswkA@mail.gmail.com>
In-Reply-To: <20170310190641.i7geazhrlmzzfna6@sigill.intra.peff.net>

On Fri, Mar 10, 2017 at 8:06 PM, Jeff King <peff@peff.net> wrote:

Show 34 quoted lines

> [Note: your original email didn't make it to the list because it's over
> 100K; I'll quote liberally].
>
> On Fri, Mar 10, 2017 at 04:15:56PM +0100, Vegard Nossum wrote:
>
>> I've used AFL to generate a corpus of pack files that maximises the edge
>> coverage for 'git index-pack'.
>>
>> This is a supplement to (and not a replacement for) the regular test cases
>> where we know exactly what each test is checking for. These testcases are
>> more useful for avoiding regressions in edge cases or as a starting point
>> for future fuzzing efforts.
>>
>> To see the output of running 'git index-pack' on each file, you can do
>> something like this:
>>
>>   make -C t GIT_TEST_OPTS="--run=34 --verbose" t5300-pack-object.sh
>>
>> I observe the following coverage changes (for t5300 only):
>>
>>   path                  old%  new%    pp
>>   ----------------------------------------
>>   builtin/index-pack.c  74.3  76.6   2.3
>>   pack-write.c          79.8  80.4    .6
>>   patch-delta.c         67.4  81.4  14.0
>>   usage.c               26.6  35.5   8.9
>>   wrapper.c             42.0  46.1   4.1
>>   zlib.c                58.7  64.1   5.4
>
> I'm not sure how I feel about this. More coverage is good, I guess, but
> we don't have any idea what these packfiles are doing, or whether
> index-pack is behaving sanely in the new lines. The most we can say is
> that we tested more lines of code and that nothing segfaulted or
> triggered something like ASAN.

Isn't the main value with these sorts of tests that they make up the difference in the current manually maintained coverage & some randomized coverage. So when you change the code in the future and the randomized coverage changes, we don't know if that's a good or a bad thing, but at least we're more likely to know that it changed, and at that point someone's likely to actually investigate the root cause, which'll turn some AFL blob testcase into an isolated testcase?