Re: [PATCH 3/3] t5303: add tests for corrupted deltas

From: Jeff King <peff@peff.net>
Date: Aug 29, 2018, 22:03 UTC
Message-ID: <20180829220353.GF29880@sigill.intra.peff.net>
In-Reply-To: <20180829205857.77340-3-jannh@google.com>

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.

Show 13 quoted lines

> 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