{"thread":{"id":"46116","subject":"t5313-pack-bounds-checks.sh flaky?","startedAt":"2017-06-05T10:33:49Z","lastAt":"2017-06-06T20:22:02Z","messageCount":4,"participants":["Lars Schneider","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"321527","messageId":"9A7C995F-ED4F-4AEB-B2B7-00FF05B80E84@gmail.com","threadId":"46116","inReplyTo":null,"subject":"t5313-pack-bounds-checks.sh flaky?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-05T10:33:44Z","receivedAt":"2017-06-05T10:33:49Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi Peff,\n\nt5313.7 failed recently on master [1]. Are you aware of any flaky parts\nin this test?\n\nHere is the output:\n\nexpecting success: \n\t# We need two objects here, so we can plausibly require\n\t# an extended table (if the first object were larger than 2^31).\n\tdo_pack \"$object $(git rev-parse HEAD)\" --index-version=2 &&\n\n\t# We have to make extra room for the table, so we cannot\n\t# just munge in place as usual.\n\t{\n\t\tdd if=$idx bs=1 count=$(($(ofs_table 2) + 4)) &&\n\t\tprintf \"\\200\\0\\0\\0\" &&\n\t\tprintf \"\\377\\0\\0\\0\\0\\0\\0\\0\" &&\n\t\tdd if=$idx bs=1 skip=$(extended_table 2)\n\t} >tmp &&\n\tmv tmp \"$idx\" &&\n\tclear_base &&\n\ttest_must_fail git cat-file blob $object &&\n\ttest_must_fail git index-pack --verify $pack\n\n1084+0 records in\n1084+0 records out\n1084 bytes (1.1 kB) copied, 0.00119315 s, 909 kB/s\n40+0 records in\n40+0 records out\n40 bytes (40 B) copied, 6.9825e-05 s, 573 kB/s\n74\ntest_must_fail: command succeeded: git cat-file blob fff0a2476aa5c8e60a3ef21cfc66e0cc670920be\n\n\nCheers,\nLars\n\n\n[1] https://travis-ci.org/git/git/jobs/239451919"},{"id":"321550","messageId":"20170605185601.yzbq5e6r2tfbgzqw@sigill.intra.peff.net","threadId":"46116","inReplyTo":"9A7C995F-ED4F-4AEB-B2B7-00FF05B80E84@gmail.com","subject":"Re: t5313-pack-bounds-checks.sh flaky?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-05T18:56:01Z","receivedAt":"2017-06-05T18:56:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 05, 2017 at 12:33:44PM +0200, Lars Schneider wrote:\n\n> Hi Peff,\n> \n> t5313.7 failed recently on master [1]. Are you aware of any flaky parts\n> in this test?\n\nNo, I've never seen it fail. I poked at it some in response to your\nmessage and couldn't convince it fail under load. We corrupt the pack\nindex and expect that Git can't read the pack, but in your test output\nsomehow we could:\n\n> expecting success: \n> \t# We need two objects here, so we can plausibly require\n> \t# an extended table (if the first object were larger than 2^31).\n> \tdo_pack \"$object $(git rev-parse HEAD)\" --index-version=2 &&\n> \n> \t# We have to make extra room for the table, so we cannot\n> \t# just munge in place as usual.\n> \t{\n> \t\tdd if=$idx bs=1 count=$(($(ofs_table 2) + 4)) &&\n> \t\tprintf \"\\200\\0\\0\\0\" &&\n> \t\tprintf \"\\377\\0\\0\\0\\0\\0\\0\\0\" &&\n> \t\tdd if=$idx bs=1 skip=$(extended_table 2)\n> \t} >tmp &&\n> \tmv tmp \"$idx\" &&\n> \tclear_base &&\n> \ttest_must_fail git cat-file blob $object &&\n> \ttest_must_fail git index-pack --verify $pack\n> \n> 1084+0 records in\n> 1084+0 records out\n> 1084 bytes (1.1 kB) copied, 0.00119315 s, 909 kB/s\n> 40+0 records in\n> 40+0 records out\n> 40 bytes (40 B) copied, 6.9825e-05 s, 573 kB/s\n> 74\n> test_must_fail: command succeeded: git cat-file blob fff0a2476aa5c8e60a3ef21cfc66e0cc670920be\n\nI'm not sure where we would have found that object. It _is_ in another\npackfile, but our clear_base() call should drop that (and restore it\nafter the test finishes).\n\nHmm. This test does have _two_ objects in the pack, and the other is a\ncommit whose sha1 may vary based on the timestamp. The object whose\nentry we corrupt always has a sha1 starting with fff0a24. That's\ngenerally likely to be the second object in index-order, and I think the\ntest probably relies on that when doing its corruption. But there's a\n1-in-16^3 probability that the commit sha1 also starts with fff and\nsorts after $object.\n\nSo if that was the case I'd expect it to fail about once in every 4096\nruns. I'm surprised not to have hit it in my stress-testing, but I don't\nactually keep a count of runs. So maybe I didn't do that many runs, or\nmaybe I was just lucky.\n\nDoing this:\n\ndiff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\nindex a8a587abc..46665368d 100755\n--- a/t/t5313-pack-bounds-checks.sh\n+++ b/t/t5313-pack-bounds-checks.sh\n@@ -139,7 +139,8 @@ test_expect_success 'bogus offset into v2 extended table' '\n test_expect_success 'bogus offset inside v2 extended table' '\n \t# We need two objects here, so we can plausibly require\n \t# an extended table (if the first object were larger than 2^31).\n-\tdo_pack \"$object $(git rev-parse HEAD)\" --index-version=2 &&\n+\tsecond=$(echo 8038 | git hash-object -w --stdin) &&\n+\tdo_pack \"$object $second\" --index-version=2 &&\n \n \t# We have to make extra room for the table, so we cannot\n \t# just munge in place as usual.\n\ncauses the same failure you saw (I picked 8038 because it's the first\nblob I found whose sha1 sorts after fff0a24).\n\nSo that is probably the culprit: you ran the test at the exact second\nthat caused the 1-in-4096 sorting problem. I'll work up a patch.\n\n-Peff\n"},{"id":"321553","messageId":"20170605191525.666opa3se7gabdbv@sigill.intra.peff.net","threadId":"46116","inReplyTo":"20170605185601.yzbq5e6r2tfbgzqw@sigill.intra.peff.net","subject":"[PATCH] t5313: make extended-table test more deterministic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-06-05T19:15:25Z","receivedAt":"2017-06-05T19:15:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit a1283866b (t5313: test bounds-checks of\ncorrupted/malicious pack/idx files, 2016-02-25) added a test\nthat requires our corrupted pack index to have two objects.\nThe entry for the first one remains untouched, but we\ncorrupt the entry for second one. Since the index stores the\nentries in sha1-sorted order, this means that the test must\nmake sure that the sha1 of the object we expect to be\ncorrupted (\"$object\") sorts after the other placeholder\nobject.\n\nThat commit used the HEAD commit as the placeholder, but the\nscript never calls test_tick. That means that the commit\nobject (and thus its sha1) depends on the timestamp when the\ntest script is run. This usually works in practice, because\nthe sha1 of $object starts with \"fff\". The commit object\nwill sort after that only 1 in 4096 times, but when it does\nthe test will fail.\n\nOne obvious solution is to add the test_tick call to get a\ndeterministic commit sha1. But since we're relying on the\nsort order for the test to function, let's make that very\nexplicit by just generating a second blob with a known sha1.\n\nReported-by: Lars Schneider <larsxschneider@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5313-pack-bounds-checks.sh | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\nindex a8a587abc..9372508c9 100755\n--- a/t/t5313-pack-bounds-checks.sh\n+++ b/t/t5313-pack-bounds-checks.sh\n@@ -139,7 +139,13 @@ test_expect_success 'bogus offset into v2 extended table' '\n test_expect_success 'bogus offset inside v2 extended table' '\n \t# We need two objects here, so we can plausibly require\n \t# an extended table (if the first object were larger than 2^31).\n-\tdo_pack \"$object $(git rev-parse HEAD)\" --index-version=2 &&\n+\t#\n+\t# Note that the value is important here. We want $object as\n+\t# the second entry in sorted-sha1 order. The sha1 of 1485 starts\n+\t# with \"000\", which sorts before that of $object (which starts\n+\t# with \"fff\").\n+\tsecond=$(echo 1485 | git hash-object -w --stdin) &&\n+\tdo_pack \"$object $second\" --index-version=2 &&\n \n \t# We have to make extra room for the table, so we cannot\n \t# just munge in place as usual.\n-- \n2.13.1.662.g6e89c999d\n"},{"id":"321646","messageId":"4D6527BB-A642-4C63-B1C2-245BF0E759C9@gmail.com","threadId":"46116","inReplyTo":"20170605191525.666opa3se7gabdbv@sigill.intra.peff.net","subject":"Re: [PATCH] t5313: make extended-table test more deterministic","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-06-06T20:21:53Z","receivedAt":"2017-06-06T20:22:02Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 05 Jun 2017, at 21:15, Jeff King <peff@peff.net> wrote:\n> \n> Commit a1283866b (t5313: test bounds-checks of\n> corrupted/malicious pack/idx files, 2016-02-25) added a test\n> that requires our corrupted pack index to have two objects.\n> The entry for the first one remains untouched, but we\n> corrupt the entry for second one. Since the index stores the\n> entries in sha1-sorted order, this means that the test must\n> make sure that the sha1 of the object we expect to be\n> corrupted (\"$object\") sorts after the other placeholder\n> object.\n> \n> That commit used the HEAD commit as the placeholder, but the\n> script never calls test_tick. That means that the commit\n> object (and thus its sha1) depends on the timestamp when the\n> test script is run. This usually works in practice, because\n> the sha1 of $object starts with \"fff\". The commit object\n> will sort after that only 1 in 4096 times, but when it does\n> the test will fail.\n> \n> One obvious solution is to add the test_tick call to get a\n> deterministic commit sha1. But since we're relying on the\n> sort order for the test to function, let's make that very\n> explicit by just generating a second blob with a known sha1.\n\nWorks for me! Thanks for the explanation!\n\n- Lars\n\n> \n> Reported-by: Lars Schneider <larsxschneider@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> t/t5313-pack-bounds-checks.sh | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\n> index a8a587abc..9372508c9 100755\n> --- a/t/t5313-pack-bounds-checks.sh\n> +++ b/t/t5313-pack-bounds-checks.sh\n> @@ -139,7 +139,13 @@ test_expect_success 'bogus offset into v2 extended table' '\n> test_expect_success 'bogus offset inside v2 extended table' '\n> \t# We need two objects here, so we can plausibly require\n> \t# an extended table (if the first object were larger than 2^31).\n> -\tdo_pack \"$object $(git rev-parse HEAD)\" --index-version=2 &&\n> +\t#\n> +\t# Note that the value is important here. We want $object as\n> +\t# the second entry in sorted-sha1 order. The sha1 of 1485 starts\n> +\t# with \"000\", which sorts before that of $object (which starts\n> +\t# with \"fff\").\n> +\tsecond=$(echo 1485 | git hash-object -w --stdin) &&\n> +\tdo_pack \"$object $second\" --index-version=2 &&\n> \n> \t# We have to make extra room for the table, so we cannot\n> \t# just munge in place as usual.\n> -- \n> 2.13.1.662.g6e89c999d\n\n"}]}