t5313-pack-bounds-checks.sh flaky?

Hi Peff,

t5313.7 failed recently on master [1]. Are you aware of any flaky parts
in this test?

Here is the output:

expecting success: 
	# We need two objects here, so we can plausibly require
	# an extended table (if the first object were larger than 2^31).
	do_pack "$object $(git rev-parse HEAD)" --index-version=2 &&

	# We have to make extra room for the table, so we cannot
	# just munge in place as usual.
	{
		dd if=$idx bs=1 count=$(($(ofs_table 2) + 4)) &&
		printf "\200\0\0\0" &&
		printf "\377\0\0\0\0\0\0\0" &&
		dd if=$idx bs=1 skip=$(extended_table 2)
	} >tmp &&
	mv tmp "$idx" &&
	clear_base &&
	test_must_fail git cat-file blob $object &&
	test_must_fail git index-pack --verify $pack

1084+0 records in
1084+0 records out
1084 bytes (1.1 kB) copied, 0.00119315 s, 909 kB/s
40+0 records in
40+0 records out
40 bytes (40 B) copied, 6.9825e-05 s, 573 kB/s
74
test_must_fail: command succeeded: git cat-file blob fff0a2476aa5c8e60a3ef21cfc66e0cc670920be


Cheers,
Lars


[1] https://travis-ci.org/git/git/jobs/239451919

On Mon, Jun 05, 2017 at 12:33:44PM +0200, Lars Schneider wrote:

Hi Peff,

t5313.7 failed recently on master [1]. Are you aware of any flaky parts
in this test?

No, I've never seen it fail. I poked at it some in response to your
message and couldn't convince it fail under load. We corrupt the pack
index and expect that Git can't read the pack, but in your test output
somehow we could:

expecting success: 
	# We need two objects here, so we can plausibly require
	# an extended table (if the first object were larger than 2^31).
	do_pack "$object $(git rev-parse HEAD)" --index-version=2 &&

	# We have to make extra room for the table, so we cannot
	# just munge in place as usual.
	{
		dd if=$idx bs=1 count=$(($(ofs_table 2) + 4)) &&
		printf "\200\0\0\0" &&
		printf "\377\0\0\0\0\0\0\0" &&
		dd if=$idx bs=1 skip=$(extended_table 2)
	} >tmp &&
	mv tmp "$idx" &&
	clear_base &&
	test_must_fail git cat-file blob $object &&
	test_must_fail git index-pack --verify $pack

1084+0 records in
1084+0 records out
1084 bytes (1.1 kB) copied, 0.00119315 s, 909 kB/s
40+0 records in
40+0 records out
40 bytes (40 B) copied, 6.9825e-05 s, 573 kB/s
74
test_must_fail: command succeeded: git cat-file blob fff0a2476aa5c8e60a3ef21cfc66e0cc670920be

I'm not sure where we would have found that object. It _is_ in another
packfile, but our clear_base() call should drop that (and restore it
after the test finishes).

Hmm. This test does have _two_ objects in the pack, and the other is a
commit whose sha1 may vary based on the timestamp. The object whose
entry we corrupt always has a sha1 starting with fff0a24. That's
generally likely to be the second object in index-order, and I think the
test probably relies on that when doing its corruption. But there's a
1-in-16^3 probability that the commit sha1 also starts with fff and
sorts after $object.

So if that was the case I'd expect it to fail about once in every 4096
runs. I'm surprised not to have hit it in my stress-testing, but I don't
actually keep a count of runs. So maybe I didn't do that many runs, or
maybe I was just lucky.

Doing this:

diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
index a8a587abc..46665368d 100755
--- a/t/t5313-pack-bounds-checks.sh
+++ b/t/t5313-pack-bounds-checks.sh

@@ -139,7 +139,8 @@ test_expect_success 'bogus offset into v2 extended table' '
 test_expect_success 'bogus offset inside v2 extended table' '
 	# We need two objects here, so we can plausibly require
 	# an extended table (if the first object were larger than 2^31).
-	do_pack "$object $(git rev-parse HEAD)" --index-version=2 &&
+	second=$(echo 8038 | git hash-object -w --stdin) &&
+	do_pack "$object $second" --index-version=2 &&
 
 	# We have to make extra room for the table, so we cannot
 	# just munge in place as usual.

causes the same failure you saw (I picked 8038 because it's the first
blob I found whose sha1 sorts after fff0a24).

So that is probably the culprit: you ran the test at the exact second
that caused the 1-in-4096 sorting problem. I'll work up a patch.

-Peff

Commit a1283866b (t5313: test bounds-checks of
corrupted/malicious pack/idx files, 2016-02-25) added a test
that requires our corrupted pack index to have two objects.
The entry for the first one remains untouched, but we
corrupt the entry for second one. Since the index stores the
entries in sha1-sorted order, this means that the test must
make sure that the sha1 of the object we expect to be
corrupted ("$object") sorts after the other placeholder
object.

That commit used the HEAD commit as the placeholder, but the
script never calls test_tick. That means that the commit
object (and thus its sha1) depends on the timestamp when the
test script is run. This usually works in practice, because
the sha1 of $object starts with "fff". The commit object
will sort after that only 1 in 4096 times, but when it does
the test will fail.

One obvious solution is to add the test_tick call to get a
deterministic commit sha1. But since we're relying on the
sort order for the test to function, let's make that very
explicit by just generating a second blob with a known sha1.

Reported-by: Lars Schneider <redacted>
Signed-off-by: Jeff King <redacted>
---
 t/t5313-pack-bounds-checks.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh
index a8a587abc..9372508c9 100755
--- a/t/t5313-pack-bounds-checks.sh
+++ b/t/t5313-pack-bounds-checks.sh

@@ -139,7 +139,13 @@ test_expect_success 'bogus offset into v2 extended table' '
 test_expect_success 'bogus offset inside v2 extended table' '
 	# We need two objects here, so we can plausibly require
 	# an extended table (if the first object were larger than 2^31).
-	do_pack "$object $(git rev-parse HEAD)" --index-version=2 &&
+	#
+	# Note that the value is important here. We want $object as
+	# the second entry in sorted-sha1 order. The sha1 of 1485 starts
+	# with "000", which sorts before that of $object (which starts
+	# with "fff").
+	second=$(echo 1485 | git hash-object -w --stdin) &&
+	do_pack "$object $second" --index-version=2 &&
 
 	# We have to make extra room for the table, so we cannot
 	# just munge in place as usual.

-- 
2.13.1.662.g6e89c999d

On 05 Jun 2017, at 21:15, Jeff King [off-list ref] wrote:

Commit a1283866b (t5313: test bounds-checks of
corrupted/malicious pack/idx files, 2016-02-25) added a test
that requires our corrupted pack index to have two objects.
The entry for the first one remains untouched, but we
corrupt the entry for second one. Since the index stores the
entries in sha1-sorted order, this means that the test must
make sure that the sha1 of the object we expect to be
corrupted ("$object") sorts after the other placeholder
object.

That commit used the HEAD commit as the placeholder, but the
script never calls test_tick. That means that the commit
object (and thus its sha1) depends on the timestamp when the
test script is run. This usually works in practice, because
the sha1 of $object starts with "fff". The commit object
will sort after that only 1 in 4096 times, but when it does
the test will fail.

One obvious solution is to add the test_tick call to get a
deterministic commit sha1. But since we're relying on the
sort order for the test to function, let's make that very
explicit by just generating a second blob with a known sha1.

Works for me! Thanks for the explanation!

- Lars