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:

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:

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:

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 <larsxschneider@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5313-pack-bounds-checks.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Works for me! Thanks for the explanation!

- Lars