Re: [PATCH 3/3] t5303: add tests for corrupted deltas
On Wed, Aug 29, 2018 at 06:03:53PM -0400, Jeff King wrote: > Without your second patch applied, this complains about mmap-ing > /dev/null (or any zero-length file). > > Also, \x escapes are sadly not portable (dash, for example, does not > respect them). You have to use octal instead (which is not too onerous > for these small numbers). > > I needed the patch below to get it to behave as expected (I also > annotated the deltas to make it more comprehensible to somebody who > hasn't just been digging in the patch code ;) ). > > I wonder if we should more fully test the 4 cases I outlined in my > earlier mail, too. So here's a version which checks each of those cases (plus your minimal base-case and the trailing garbage case from your original). I did it as a straight patch rather than on top of yours, since I think that's easier to read (and I'd expect us to squash together whatever we end up with anyway). This doesn't cover out-of-bounds reads while parsing the offset/size. It's dinner-time here, but I may take a look at that later tonight. --- diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh index 3634e258f8..305922eeb3 100755 --- a/t/t5303-pack-corruption-resilience.sh +++ b/t/t5303-pack-corruption-resilience.sh @@ -311,4 +311,81 @@ test_expect_success \ test_must_fail git cat-file blob $blob_2 > /dev/null && test_must_fail git cat-file blob $blob_3 > /dev/null' +# Base sanity check; this is the smallest possible delta. +# +# \5 - five bytes in base (though we do not use it) +# \1 - one byte in result +# \1 - copy one byte (X) +test_expect_success \ +'apply good minimal delta' \ +'printf "\5\1\1X" > minimal_delta && + echo base >base && + test-tool delta -p base minimal_delta /dev/null + ' + +# This delta has too many literal bytes to fit in destination. +# +# \5 - five bytes in base (though we do not use it) +# \1 - 1 byte in result +# \2 - copy two bytes (one too many) +test_expect_success \ +'apply truncated delta' \ +'printf "\5\1\2XX" > too_big_literal && + echo base >base && + test_must_fail test-tool delta -p base too_big_literal /dev/null + ' + +# This delta has too many copy bytes to fit in destination. +# +# \5 - five bytes in base +# \1 - one byte in result +# \221 - copy, one byte offset, one byte size +# \0 - copy from offset 0 +# \2 - copy two bytes (one too many) +test_expect_success \ +'apply delta with trailing garbage command' \ +'printf "\5\1\221\0\2" > too_big_copy && + echo base >base && + test_must_fail test-tool delta -p base too_big_copy /dev/null + ' + +# This delta has too few bytes in the delta itself. +# +# \5 - five bytes in base (though we do not use it) +# \2 - two bytes in result +# \2 - copy two bytes (we are short one) +test_expect_success \ +'apply truncated delta' \ +'printf "\5\2\2X" > truncated_delta && + echo base >base && + test_must_fail test-tool delta -p base truncated_delta /dev/null + ' + +# This delta has too few bytes in the base. +# +# \5 - five bytes in base +# \6 - six bytes in result +# \221 - copy, one byte offset, one byte size +# \0 - copy from offset 0 +# \6 - copy six bytes (one too many) +test_expect_success \ +'apply delta with trailing garbage command' \ +'printf "\5\6\221\0\6" > truncated_base && + echo base >base && + test_must_fail test-tool delta -p base truncated_base /dev/null + ' + +# Trailing garbage command. +# +# \5 - five bytes in base (though we do not use it) +# \1 - one byte in result +# \1 - copy one byte (X) +# \1 - trailing garbage command +test_expect_success \ +'apply delta with trailing garbage command' \ +'printf "\5\1\1X\1" > tail_garbage_delta && + echo base >base && + test_must_fail test-tool delta -p base tail_garbage_delta /dev/null + ' + test_done
Re: [PATCH 3/3] t5303: add tests for corrupted deltas
On Wed, Aug 29, 2018 at 10:58:57PM +0200, Jann Horn wrote: > This verifies the changes from commit "patch-delta: fix oob read". A minor nit, but usually we'd either introduce tests along with the fix, or introduce them beforehand as test_expect_failure and then flip them to success along with the fix. > diff --git a/t/t5303-pack-corruption-resilience.sh > b/t/t5303-pack-corruption-resilience.sh > index 3634e258f..7152376b6 100755 > --- a/t/t5303-pack-corruption-resilience.sh > +++ b/t/t5303-pack-corruption-resilience.sh > @@ -311,4 +311,22 @@ test_expect_success \ > test_must_fail git cat-file blob $blob_2 > /dev/null && > test_must_fail git cat-file blob $blob_3 > /dev/null' > > +test_expect_success \ > +'apply good minimal delta' \ > +'printf "\x00\x01\x01X" > minimal_delta && > + test-tool delta -p /dev/null minimal_delta /dev/null > + ' Without your second patch applied, this complains about mmap-ing /dev/null (or any zero-length file). Also, \x escapes are sadly not portable (dash, for example, does not respect them). You have to use octal instead (which is not too onerous for these small numbers). I needed the patch below to get it to behave as expected (I also annotated the deltas to make it more comprehensible to somebody who hasn't just been digging in the patch code ;) ). I wonder if we should more fully test the 4 cases I outlined in my earlier mail, too. -Peff --- diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh index 7152376b67..df28cce68b 100755 --- a/t/t5303-pack-corruption-resilience.sh +++ b/t/t5303-pack-corruption-resilience.sh @@ -311,22 +311,35 @@ test_expect_success \ test_must_fail git cat-file blob $blob_2 > /dev/null && test_must_fail git cat-file blob $blob_3 > /dev/null' +# \5 - five bytes in base (though we do not use it) +# \1 - one byte in result +# \1 - copy one byte (X) test_expect_success \ 'apply good minimal delta' \ -'printf "\x00\x01\x01X" > minimal_delta && - test-tool delta -p /dev/null minimal_delta /dev/null +'printf "\5\1\1X" > minimal_delta && + echo base >base && + test-tool delta -p base minimal_delta /dev/null ' +# \5 - five bytes in base (though we do not use it) +# \2 - two bytes in result +# \2 - copy two bytes (we are short one) test_expect_success \ 'apply truncated delta' \ -'printf "\x00\x02\x02X" > truncated_delta && - test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null +'printf "\5\2\2X" > truncated_delta && + echo base >base && + test_must_fail test-tool delta -p base truncated_delta /dev/null ' +# \5 - five bytes in base (though we do not use it) +# \1 - one byte in result +# \1 - copy one byte (X) +# \1 - trailing garbage command test_expect_success \ 'apply delta with trailing garbage command' \ -'printf "\x00\x01\x01X\x01" > tail_garbage_delta && - test_must_fail test-tool delta -p /dev/null tail_garbage_delta /dev/null +'printf "\5\1\1X\1" > tail_garbage_delta && + echo base >base && + test_must_fail test-tool delta -p base tail_garbage_delta /dev/null ' test_done
[PATCH 3/3] t5303: add tests for corrupted deltas
This verifies the changes from commit "patch-delta: fix oob read". Signed-off-by: Jann Horn --- t/t5303-pack-corruption-resilience.sh | 18 ++ 1 file changed, 18 insertions(+) diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh index 3634e258f..7152376b6 100755 --- a/t/t5303-pack-corruption-resilience.sh +++ b/t/t5303-pack-corruption-resilience.sh @@ -311,4 +311,22 @@ test_expect_success \ test_must_fail git cat-file blob $blob_2 > /dev/null && test_must_fail git cat-file blob $blob_3 > /dev/null' +test_expect_success \ +'apply good minimal delta' \ +'printf "\x00\x01\x01X" > minimal_delta && + test-tool delta -p /dev/null minimal_delta /dev/null + ' + +test_expect_success \ +'apply truncated delta' \ +'printf "\x00\x02\x02X" > truncated_delta && + test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null + ' + +test_expect_success \ +'apply delta with trailing garbage command' \ +'printf "\x00\x01\x01X\x01" > tail_garbage_delta && + test_must_fail test-tool delta -p /dev/null tail_garbage_delta /dev/null + ' + test_done -- 2.19.0.rc0.228.g281dcd1b4d0-goog
