{"thread":{"id":"45344","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","startedAt":"2017-03-10T16:01:05Z","lastAt":"2017-03-13T19:13:59Z","messageCount":13,"participants":["Vegard Nossum","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313751","messageId":"636b65b5-2a19-0416-beef-fbc57aa4d89d@oracle.com","threadId":"45344","inReplyTo":"20170310151556.18490-1-vegard.nossum@oracle.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-10T16:00:45Z","receivedAt":"2017-03-10T16:01:05Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 10/03/2017 16:15, Vegard Nossum wrote:\n> I've used AFL to generate a corpus of pack files that maximises the edge\n> coverage for 'git index-pack'.\n>\n> This is a supplement to (and not a replacement for) the regular test cases\n> where we know exactly what each test is checking for. These testcases are\n> more useful for avoiding regressions in edge cases or as a starting point\n> for future fuzzing efforts.\n>\n> To see the output of running 'git index-pack' on each file, you can do\n> something like this:\n>\n> make -C t GIT_TEST_OPTS=\"--run=34 --verbose\" t5300-pack-object.sh\n>\n> I observe the following coverage changes (for t5300 only):\n>\n> path old% new% pp\n> ----------------------------------------\n> builtin/index-pack.c 74.3 76.6 2.3\n> pack-write.c 79.8 80.4 .6\n> patch-delta.c 67.4 81.4 14.0\n> usage.c 26.6 35.5 8.9\n> wrapper.c 42.0 46.1 4.1\n> zlib.c 58.7 64.1 5.4\n\nAnd if you add this simple patch on top (sorry, I didn't think of it\nuntil after I'd sent the previous e-mail):\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 19e02ffc2..db705ba5c 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -425,8 +425,10 @@ test_expect_success 'index-pack works in \nnon-repo' '\n test_expect_success 'index-pack edge coverage' '\n for pack in \"$TEST_DIRECTORY\"/t5300/*.pack\n do\n- rm -rf \"${pack%.pack}.idx\" &&\n- test_might_fail git index-pack $pack\n+ rm -rf \"${pack%.pack}.idx\" tmp.pack tmp.idx &&\n+ test_might_fail git index-pack $pack &&\n+ test_might_fail git index-pack --strict $pack &&\n+ test_might_fail git index-pack --stdin --fix-thin \ntmp.pack < $pack\n done\n '\n\n\nyou get this change to the coverage profile instead:\n\npath old% new% pp\n----------------------------------------\n\nalloc.c 58.1 67.4 9.3\nbuiltin/index-pack.c 74.3 80.7 6.4\ncommit.c 13.9 17.4 3.5\ndate.c 3.5 4.2 .7\nfsck.c 15.7 33.7 18.0\nobject.c 56.0 58.7 2.7\npack-write.c 79.8 81.4 1.6\npatch-delta.c 67.4 81.4 14.0\npath.c 31.6 32.1 .5\nsha1_file.c 48.9 49.6 .7\ntag.c 3.7 16.8 13.1\ntree.c 36.6 37.5 .9\nusage.c 26.6 35.5 8.9\nwrapper.c 42.0 46.1 4.1\nzlib.c 58.7 64.1 5.4\n\nOf course, it's likely some of those gains can be found in other\ntestcases outside t5300 -- also, coverage isn't everything. Still seems\nlike a nice gain with very little effort.\n\n\nVegard\n"},{"id":"313775","messageId":"20170310190641.i7geazhrlmzzfna6@sigill.intra.peff.net","threadId":"45344","inReplyTo":"20170310151556.18490-1-vegard.nossum@oracle.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-10T19:06:42Z","receivedAt":"2017-03-10T19:06:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[Note: your original email didn't make it to the list because it's over\n100K; I'll quote liberally].\n\nOn Fri, Mar 10, 2017 at 04:15:56PM +0100, Vegard Nossum wrote:\n\n> I've used AFL to generate a corpus of pack files that maximises the edge\n> coverage for 'git index-pack'.\n> \n> This is a supplement to (and not a replacement for) the regular test cases\n> where we know exactly what each test is checking for. These testcases are\n> more useful for avoiding regressions in edge cases or as a starting point\n> for future fuzzing efforts.\n> \n> To see the output of running 'git index-pack' on each file, you can do\n> something like this:\n> \n> make -C t GIT_TEST_OPTS=\"--run=34 --verbose\" t5300-pack-object.sh\n> \n> I observe the following coverage changes (for t5300 only):\n> \n> path old% new% pp\n> ----------------------------------------\n> builtin/index-pack.c 74.3 76.6 2.3\n> pack-write.c 79.8 80.4 .6\n> patch-delta.c 67.4 81.4 14.0\n> usage.c 26.6 35.5 8.9\n> wrapper.c 42.0 46.1 4.1\n> zlib.c 58.7 64.1 5.4\n\nI'm not sure how I feel about this. More coverage is good, I guess, but\nwe don't have any idea what these packfiles are doing, or whether\nindex-pack is behaving sanely in the new lines. The most we can say is\nthat we tested more lines of code and that nothing segfaulted or\ntriggered something like ASAN.\n\nThat's something I guess, but I'm not enthused by the idea of just\ndumping a bunch of binary test cases that nobody, not even the author,\nunderstands.\n\n-Peff\n"},{"id":"313778","messageId":"eec5ab2a-7fe7-b47f-8073-a8212a9634f1@oracle.com","threadId":"45344","inReplyTo":"20170310190641.i7geazhrlmzzfna6@sigill.intra.peff.net","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-10T19:34:45Z","receivedAt":"2017-03-10T19:35:06Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 10/03/2017 20:06, Jeff King wrote:\n> On Fri, Mar 10, 2017 at 04:15:56PM +0100, Vegard Nossum wrote:\n>\n>> I've used AFL to generate a corpus of pack files that maximises the edge\n>> coverage for 'git index-pack'.\n>>\n>> This is a supplement to (and not a replacement for) the regular test cases\n>> where we know exactly what each test is checking for. These testcases are\n>> more useful for avoiding regressions in edge cases or as a starting point\n>> for future fuzzing efforts.\n>>\n>> To see the output of running 'git index-pack' on each file, you can do\n>> something like this:\n>>\n>> make -C t GIT_TEST_OPTS=\"--run=34 --verbose\" t5300-pack-object.sh\n>>\n>> I observe the following coverage changes (for t5300 only):\n>>\n>> path old% new% pp\n>> ----------------------------------------\n>> builtin/index-pack.c 74.3 76.6 2.3\n>> pack-write.c 79.8 80.4 .6\n>> patch-delta.c 67.4 81.4 14.0\n>> usage.c 26.6 35.5 8.9\n>> wrapper.c 42.0 46.1 4.1\n>> zlib.c 58.7 64.1 5.4\n>\n> I'm not sure how I feel about this. More coverage is good, I guess, but\n> we don't have any idea what these packfiles are doing, or whether\n> index-pack is behaving sanely in the new lines. The most we can say is\n> that we tested more lines of code and that nothing segfaulted or\n> triggered something like ASAN.\n>\n> That's something I guess, but I'm not enthused by the idea of just\n> dumping a bunch of binary test cases that nobody, not even the author,\n> understands.\n\nI understand your concern. This is how I see it:\n\nNegatives:\n\n - 'make test' takes 1 second longer to run\n\n - 548K data added to git.git\n\nPositives:\n\n - regularly exercising more of the code, especially some of the corner\ncases which are not caught by the rest of the test suite, possibly\ncatching bugs in a security-critical bit of git before it makes it into\na release\n\n - no impact to existing code, everything self-contained in 1 directory\n\n - giving more people access to the testcases I discovered without\nhaving to repeat the effort of setting up AFL, fixing up SHA1 checksums,\nminimising the corpus, running AFL for a week, etc. (each step by itself\nis pretty small, but taken altogether I think it's worthwhile not\nto have to repeat that).\n\nThen I guess you have to weigh the negatives and positives. For me it's\na clear net win, but others may see it differently.\n\nFor sure, I (or somebody else) can go through each testcase and figure\nout what it's doing, what it's doing differently from the existing\nmanual testcases in t5300, and what its expected output should be. It's\nnot that I couldn't understand what each testcase is doing if I tried,\nbut I don't think it's worth the effort. I've run everything under\nvalgrind and the only thing that turned up were some suspicious-looking\nallocations (which AFAICT should be safe to ignore because of git's\nbuilt-in limits). Otherwise it's mostly hitting sanity checks:\n\nerror: inflate: data stream error (incorrect header check)\nfatal: pack has 4 unresolved deltas\nfatal: pack has bad object at offset 12: delta base offset is out of bound\nfatal: invalid tag\n...\n\nThese are errors you wouldn't see normally and which the existing\ntestcases don't check for (or not exhaustively or systematically, in any\ncase).\n\nIf somebody were to look at the code and say: \"hey, that check looks a\nbit off\" (which is something I personally do all the time), then being\nable to quickly find an input to execute exactly that line of code is\nextremely valuable -- and you can do that simply by running the\ntestcases through gcov.\n\nAnyway, the patch/data is there, use it or don't.\n\n\nVegard\n"},{"id":"313782","messageId":"20170310194245.p37w6mew4que6oya@sigill.intra.peff.net","threadId":"45344","inReplyTo":"eec5ab2a-7fe7-b47f-8073-a8212a9634f1@oracle.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-10T19:42:45Z","receivedAt":"2017-03-10T19:43:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 08:34:45PM +0100, Vegard Nossum wrote:\n\n> > That's something I guess, but I'm not enthused by the idea of just\n> > dumping a bunch of binary test cases that nobody, not even the author,\n> > understands.\n> \n> I understand your concern. This is how I see it:\n> \n> Negatives:\n> \n> - 'make test' takes 1 second longer to run\n> \n> - 548K data added to git.git\n\nMy real concern is that this is the tip of the ice berg. So we increased\ncoverage in one program by a few percent. But wouldn't this procedure be\napplicable to lots of _other_ parts of Git, too?\n\nIOW, I'm worried about a day when we've added dozens or hundreds of\nseconds to the test suite. Sure, we can quit adding at any time, but I\nfeel like it's easier to make a decision from the outset.\n\nI'm tempted to say these should go into a different test-suite, or be\nmarked with a special flag or something. But then I guess nobody runs\nthem.\n\n-Peff\n"},{"id":"313799","messageId":"3a09226f-6749-6956-6fb9-265383cd4d66@oracle.com","threadId":"45344","inReplyTo":"20170310194245.p37w6mew4que6oya@sigill.intra.peff.net","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-10T21:18:23Z","receivedAt":"2017-03-10T21:19:01Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 10/03/2017 20:42, Jeff King wrote:\n>>> That's something I guess, but I'm not enthused by the idea of just\n>>> dumping a bunch of binary test cases that nobody, not even the author,\n>>> understands.\n[...]\n\n> My real concern is that this is the tip of the ice berg. So we increased\n> coverage in one program by a few percent. But wouldn't this procedure be\n> applicable to lots of _other_ parts of Git, too?\n\nI think that index-pack is in a special position given its role as the\nverifier for packs received over the network, which you also wrote here:\nhttps://www.spinics.net/lists/git/msg265118.html\n\nI also think increased coverage for other parts of git which are not\nconsidered security-sensitive is less valuable without testing for an\nactual expected result.\n\n\nVegard\n"},{"id":"313806","messageId":"CACBZZX5fGU9C-z94KbMAs_AegOSGtq8nbrkRe-NxBCHYsDswkA@mail.gmail.com","threadId":"45344","inReplyTo":"20170310190641.i7geazhrlmzzfna6@sigill.intra.peff.net","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-03-10T22:58:13Z","receivedAt":"2017-03-10T22:58:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Mar 10, 2017 at 8:06 PM, Jeff King wrote:\n> [Note: your original email didn't make it to the list because it's over\n> 100K; I'll quote liberally].\n>\n> On Fri, Mar 10, 2017 at 04:15:56PM +0100, Vegard Nossum wrote:\n>\n>> I've used AFL to generate a corpus of pack files that maximises the edge\n>> coverage for 'git index-pack'.\n>>\n>> This is a supplement to (and not a replacement for) the regular test cases\n>> where we know exactly what each test is checking for. These testcases are\n>> more useful for avoiding regressions in edge cases or as a starting point\n>> for future fuzzing efforts.\n>>\n>> To see the output of running 'git index-pack' on each file, you can do\n>> something like this:\n>>\n>> make -C t GIT_TEST_OPTS=\"--run=34 --verbose\" t5300-pack-object.sh\n>>\n>> I observe the following coverage changes (for t5300 only):\n>>\n>> path old% new% pp\n>> ----------------------------------------\n>> builtin/index-pack.c 74.3 76.6 2.3\n>> pack-write.c 79.8 80.4 .6\n>> patch-delta.c 67.4 81.4 14.0\n>> usage.c 26.6 35.5 8.9\n>> wrapper.c 42.0 46.1 4.1\n>> zlib.c 58.7 64.1 5.4\n>\n> I'm not sure how I feel about this. More coverage is good, I guess, but\n> we don't have any idea what these packfiles are doing, or whether\n> index-pack is behaving sanely in the new lines. The most we can say is\n> that we tested more lines of code and that nothing segfaulted or\n> triggered something like ASAN.\n\nIsn't the main value with these sorts of tests that they make up the\ndifference in the current manually maintained coverage & some\nrandomized coverage. So when you change the code in the future and the\nrandomized coverage changes, we don't know if that's a good or a bad\nthing, but at least we're more likely to know that it changed, and at\nthat point someone's likely to actually investigate the root cause,\nwhich'll turn some AFL blob testcase into an isolated testcase?\n"},{"id":"313853","messageId":"20170312122458.5iyypzfbpl6kblkn@sigill.intra.peff.net","threadId":"45344","inReplyTo":"3a09226f-6749-6956-6fb9-265383cd4d66@oracle.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T12:24:59Z","receivedAt":"2017-03-12T12:25:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 10:18:23PM +0100, Vegard Nossum wrote:\n\n> On 10/03/2017 20:42, Jeff King wrote:\n> > > > That's something I guess, but I'm not enthused by the idea of just\n> > > > dumping a bunch of binary test cases that nobody, not even the author,\n> > > > understands.\n> [...]\n> \n> > My real concern is that this is the tip of the ice berg. So we increased\n> > coverage in one program by a few percent. But wouldn't this procedure be\n> > applicable to lots of _other_ parts of Git, too?\n> \n> I think that index-pack is in a special position given its role as the\n> verifier for packs received over the network, which you also wrote here:\n> https://www.spinics.net/lists/git/msg265118.html\n\nThat's true. If we take that attitude, my \"slippery slope\" concerns go\naway.\n\n-Peff\n"},{"id":"313854","messageId":"20170312123212.3rnqyx3dvi5yppk5@sigill.intra.peff.net","threadId":"45344","inReplyTo":"CACBZZX5fGU9C-z94KbMAs_AegOSGtq8nbrkRe-NxBCHYsDswkA@mail.gmail.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T12:32:12Z","receivedAt":"2017-03-12T12:32:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 10, 2017 at 11:58:13PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> >> I observe the following coverage changes (for t5300 only):\n> >>\n> >> path old% new% pp\n> >> ----------------------------------------\n> >> builtin/index-pack.c 74.3 76.6 2.3\n> >> pack-write.c 79.8 80.4 .6\n> >> patch-delta.c 67.4 81.4 14.0\n> >> usage.c 26.6 35.5 8.9\n> >> wrapper.c 42.0 46.1 4.1\n> >> zlib.c 58.7 64.1 5.4\n> >\n> > I'm not sure how I feel about this. More coverage is good, I guess, but\n> > we don't have any idea what these packfiles are doing, or whether\n> > index-pack is behaving sanely in the new lines. The most we can say is\n> > that we tested more lines of code and that nothing segfaulted or\n> > triggered something like ASAN.\n> \n> Isn't the main value with these sorts of tests that they make up the\n> difference in the current manually maintained coverage & some\n> randomized coverage. So when you change the code in the future and the\n> randomized coverage changes, we don't know if that's a good or a bad\n> thing, but at least we're more likely to know that it changed, and at\n> that point someone's likely to actually investigate the root cause,\n> which'll turn some AFL blob testcase into an isolated testcase?\n\nI think you may be more optimistic than me in people actually looking at\nour coverage numbers. There is a \"make coverage\" target, but I don't\nthink its output is useful for anybody trying to make comparison\nmetrics.\n\nOne further devil's advocate:\n\nIf people really _do_ care about coverage, arguably the AFL tests are a\npollution of that concept. Because they are running the code, but doing\na very perfunctory job of testing it. IOW, our coverage of \"code that\ndoesn't segfault or trigger ASAN\" is improved, but our coverage of \"code\nthat has been tested to be correct\" is not (and since the tests are\nlumped together, it's hard to get anything but one number).\n\nSo I dunno. I remain on the fence about the patch.\n\n-Peff\n"},{"id":"313859","messageId":"8fb54c74-a5a5-eb55-8734-61a3753c05e1@oracle.com","threadId":"45344","inReplyTo":"20170312123212.3rnqyx3dvi5yppk5@sigill.intra.peff.net","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-12T13:44:52Z","receivedAt":"2017-03-12T13:45:14Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 12/03/2017 13:32, Jeff King wrote:\n> If people really _do_ care about coverage, arguably the AFL tests are a\n> pollution of that concept. Because they are running the code, but doing\n> a very perfunctory job of testing it. IOW, our coverage of \"code that\n> doesn't segfault or trigger ASAN\" is improved, but our coverage of \"code\n> that has been tested to be correct\" is not (and since the tests are\n> lumped together, it's hard to get anything but one number).\n\nIt wouldn't be hard to separate out the testcases found by fuzzing\nI've attached a patch that does just that -- none of the new testcases\nare run unless you pass -f/--fuzzing in GIT_TEST_OPTS.\n\n$ make -C t GIT_TEST_OPTS=\"--run=34\" t5300-pack-object.sh\nmake: Entering directory '/home/vegard/git/git/t'\n*** t5300-pack-object.sh ***\n[...]\nok 34 # skip index-pack edge coverage (missing FUZZING)\n[...]\n\n$ make -C t GIT_TEST_OPTS=\"--run=34 -f\" t5300-pack-object.sh\nmake: Entering directory '/home/vegard/git/git/t'\n*** t5300-pack-object.sh ***\n[...]\nok 34 - index-pack edge coverage\n[...]\n\nI assume automatic testing like e.g. Travis would want to enable this.\n\nWould that help at all?\n\n\nVegard\n\n\nFrom 04446ce562eee129588f2c92c4eef2c82ed4bb4f Mon Sep 17 00:00:00 2001\nFrom: Vegard Nossum \nDate: Sun, 12 Mar 2017 14:35:25 +0100\nSubject: [PATCH] test-lib: add --fuzzing option\n\nFrom t/README:\n\n\tThis causes additional testcases found by fuzzing to be run,\n\tfor more exhaustive testing. Please note that these testcases\n\thave not been vetted for correctness, but they may uncover\n\tbugs introduced in code paths which are not otherwise run\n\tin other tests.\n\nThe -f/--fuzzing/FUZZING name is up for discussion, I just couldn't think\nof anything more descriptive.\n---\n t/README | 8 ++++++++\n t/t5300-pack-object.sh | 2 +-\n t/test-lib.sh | 6 ++++++\n 3 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/t/README b/t/README\nindex 4982d1c52..2c56567b1 100644\n--- a/t/README\n+++ b/t/README\n@@ -110,6 +110,14 @@ appropriately before running \"make\".\n \tThis causes additional long-running tests to be run (where\n \tavailable), for more exhaustive testing.\n \n+-f::\n+--fuzzing::\n+\tThis causes additional testcases found by fuzzing to be run,\n+\tfor more exhaustive testing. Please note that these testcases\n+\thave not been vetted for correctness, but they may uncover\n+\tbugs introduced in code paths which are not otherwise run\n+\tin other tests.\n+\n -r::\n --run=::\n \tRun only the subset of tests indicated by\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 19e02ffc2..f58d0d4bf 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -422,7 +422,7 @@ test_expect_success 'index-pack works in non-repo' '\n '\n \n # These pack files were generated using AFL\n-test_expect_success 'index-pack edge coverage' '\n+test_expect_success FUZZING 'index-pack edge coverage' '\n \tfor pack in \"$TEST_DIRECTORY\"/t5300/*.pack\n \tdo\n \t\trm -rf \"${pack%.pack}.idx\" &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 86d77c16d..35df2bd6c 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -209,6 +209,8 @@ do\n \t\timmediate=t; shift ;;\n \t-l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests)\n \t\tGIT_TEST_LONG=t; export GIT_TEST_LONG; shift ;;\n+\t-f|--f|--fuzzing)\n+\t\tGIT_TEST_FUZZING=t; export GIT_TEST_FUZZING; shift ;;\n \t-r)\n \t\tshift; test \"$#\" -ne 0 || {\n \t\t\techo 'error: -r requires an argument' >&2;\n@@ -1098,6 +1100,10 @@ test_lazy_prereq EXPENSIVE '\n \ttest -n \"$GIT_TEST_LONG\"\n '\n \n+test_lazy_prereq FUZZING '\n+\ttest -n \"$GIT_TEST_FUZZING\"\n+'\n+\n test_lazy_prereq USR_BIN_TIME '\n \ttest -x /usr/bin/time\n '\n-- \n2.12.0.rc0\n\n"},{"id":"313865","messageId":"xmqq7f3uuzuu.fsf@gitster.mtv.corp.google.com","threadId":"45344","inReplyTo":"20170312123212.3rnqyx3dvi5yppk5@sigill.intra.peff.net","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-12T18:14:17Z","receivedAt":"2017-03-12T18:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> One further devil's advocate:\n>\n> If people really _do_ care about coverage, arguably the AFL tests are a\n> pollution of that concept. Because they are running the code, but doing\n> a very perfunctory job of testing it. IOW, our coverage of \"code that\n> doesn't segfault or trigger ASAN\" is improved, but our coverage of \"code\n> that has been tested to be correct\" is not (and since the tests are\n> lumped together, it's hard to get anything but one number).\n>\n> So I dunno. I remain on the fence about the patch.\n\nYeah, I have been disturbed by your earlier remark \"binary test\ncases that nobody, not even the author, understands\", and the above\nsummarizes it more clearly.\n\nContinuously running fuzzer tests on the codebase would have value,\nbut how exactly are these fuzzballs generated? Don't they depend on\nthe code being tested? IOW, how effective is a set of fuzzballs\nthat forces the code to take more branches in the current codepath\nfor the purpose of testing new code that updates the codepath,\nchanging the structure of the codeflow? Unless a new set of\nfuzzballs to match the updated codeflow is generated, wouldn't the\ntest coverage with these fuzzballs erode over time, making them less\nand less useful baggage we carry around, without nobody noticing that\nthey no longer are effective to help test coverage?\n"},{"id":"313882","messageId":"ee3f01f0-2dc1-f919-223c-dad6032fa396@oracle.com","threadId":"45344","inReplyTo":"xmqq7f3uuzuu.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-13T11:07:32Z","receivedAt":"2017-03-13T11:07:55Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 12/03/2017 19:14, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> One further devil's advocate:\n>>\n>> If people really _do_ care about coverage, arguably the AFL tests are a\n>> pollution of that concept. Because they are running the code, but doing\n>> a very perfunctory job of testing it. IOW, our coverage of \"code that\n>> doesn't segfault or trigger ASAN\" is improved, but our coverage of \"code\n>> that has been tested to be correct\" is not (and since the tests are\n>> lumped together, it's hard to get anything but one number).\n>>\n>> So I dunno. I remain on the fence about the patch.\n>\n> Yeah, I have been disturbed by your earlier remark \"binary test\n> cases that nobody, not even the author, understands\", and the above\n> summarizes it more clearly.\n>\n> Continuously running fuzzer tests on the codebase would have value,\n> but how exactly are these fuzzballs generated? Don't they depend on\n> the code being tested? IOW, how effective is a set of fuzzballs\n> that forces the code to take more branches in the current codepath\n> for the purpose of testing new code that updates the codepath,\n> changing the structure of the codeflow? Unless a new set of\n> fuzzballs to match the updated codeflow is generated, wouldn't the\n> test coverage with these fuzzballs erode over time, making them less\n> and less useful baggage we carry around, without nobody noticing that\n> they no longer are effective to help test coverage?\n\nYes, the testcases are generated based on the feedback from the\ninstrumented code, so they are very clearly shaped by the code itself.\n\nHowever, I think it's more useful to think of these testcases not as\n\"binary test that nobody knows what they are doing\", but as \"(sometimes\ninvalid) packfiles which tickle interesting code paths in the packfile\nparser\".\n\nWith this perspective it becomes clearer that while they were generated\nfrom the code, they also in a sense describe the packfile format itself.\n\nI did a few experiments in changing the code of the packfile reader in\nvarious small ways (e.g. deleting a check, reordering some code) to see\nthe effects of the testcases found by fuzzing, and I have to admit it\nwas fairly disappointing. The testcases I added did not catch a single\nbuggy change, whereas the other testcases did catch many of them.\n\nSo I guess you are right -- these testcases are maybe really not that\nuseful as part of the test suite without explicit comparison between the\nexpected and actual results.\n\nForget about the patch, I will just put the testcases in a separate repo\ninstead. Maybe I will convert some of them into \"real\" tests.\n\nThanks, and sorry for the noise.\n\n\nVegard\n"},{"id":"313898","messageId":"xmqqbmt5t83o.fsf@gitster.mtv.corp.google.com","threadId":"45344","inReplyTo":"ee3f01f0-2dc1-f919-223c-dad6032fa396@oracle.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T17:11:23Z","receivedAt":"2017-03-13T17:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Vegard Nossum writes:\n\n> However, I think it's more useful to think of these testcases not as\n> \"binary test that nobody knows what they are doing\", but as \"(sometimes\n> invalid) packfiles which tickle interesting code paths in the packfile\n> parser\".\n>\n> With this perspective it becomes clearer that while they were generated\n> from the code, they also in a sense describe the packfile format itself.\n\nI do agree with these two paragraphs (that is why I said that\ncontinuously running fuzzer tests on the codebase would have value),\nand I really appreciate the effort.\n\n> I did a few experiments in changing the code of the packfile reader in\n> various small ways (e.g. deleting a check, reordering some code) to see\n> the effects of the testcases found by fuzzing, and I have to admit it\n> was fairly disappointing. The testcases I added did not catch a single\n> buggy change, whereas the other testcases did catch many of them.\n\nIn short, the summary of the above three paragraphs is that we still\ndo believe the general approach of using fuzzer has value, but your\nexperiment indicates that data presented in the patch in this thread\nweren't particularly good examples to demonstrate the merit?\n\n\n"},{"id":"313931","messageId":"2ec3df43-6ca6-9642-89b9-b118c60a62fc@oracle.com","threadId":"45344","inReplyTo":"xmqqbmt5t83o.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC][PATCH] index-pack: add testcases found using AFL","fromName":"Vegard Nossum","fromEmail":"vegard.nossum@oracle.com","sentAt":"2017-03-13T19:13:26Z","receivedAt":"2017-03-13T19:13:59Z","isPatch":true,"sender":{"key":"vegard.nossum@oracle.com","avatar":"https://avatars.githubusercontent.com/u/24173?v=4"},"body":"On 13/03/2017 18:11, Junio C Hamano wrote:\n> Vegard Nossum writes:\n>\n>> However, I think it's more useful to think of these testcases not as\n>> \"binary test that nobody knows what they are doing\", but as \"(sometimes\n>> invalid) packfiles which tickle interesting code paths in the packfile\n>> parser\".\n>>\n>> With this perspective it becomes clearer that while they were generated\n>> from the code, they also in a sense describe the packfile format itself.\n>\n> I do agree with these two paragraphs (that is why I said that\n> continuously running fuzzer tests on the codebase would have value),\n> and I really appreciate the effort.\n>\n>> I did a few experiments in changing the code of the packfile reader in\n>> various small ways (e.g. deleting a check, reordering some code) to see\n>> the effects of the testcases found by fuzzing, and I have to admit it\n>> was fairly disappointing. The testcases I added did not catch a single\n>> buggy change, whereas the other testcases did catch many of them.\n>\n> In short, the summary of the above three paragraphs is that we still\n> do believe the general approach of using fuzzer has value, but your\n> experiment indicates that data presented in the patch in this thread\n> weren't particularly good examples to demonstrate the merit?\n\nCorrect.\n\nI thought a priori that the testcases found by AFL would work well as a\nregression suite in the face of buggy code changes, but this turned out\nnot to be the case in practice when I tried introducing bugs on purpose.\n\nThe testcases would still have value for the following purposes:\n\n- as a seed for continued fuzzing (as the fuzzing effort would not have\nto start over from scratch)\n\n- as a way to quickly find an input that reaches a specific line of code\nwithout having to manually poke at a packfile\n\n- as a basis for writing new testcases with specific expected results\n\n\nVegard\n"}]}