patch-delta: consistently report corruption

When applying a delta, if we see an opcode that cannot be
fulfilled (e.g., asking to write more bytes than the
destination has left), we break out of our parsing loop but
don't signal an explicit error. We rely on the sanity check
after the loop to see if we have leftover delta bytes or
didn't fill our result buffer.

This can silently ignore corruption when the delta buffer
ends with a bogus command and the destination buffer is
already full. Instead, let's jump into the error handler
directly when we see this case.

Note that the tests also cover the "bad opcode" case, which
already handles this correctly.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/patch-delta.c b/patch-delta.c
index b937afd2c99c8ac40b2eaa37620a6942e46f8b4c..283fb4b7591a179c9e248ec3d9ac8ac47f3a4c9f 100644 (file)
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
                        if (unsigned_add_overflows(cp_off, cp_size) ||
                            cp_off + cp_size > src_size ||
                            cp_size > size)
-                               break;
+                               goto bad_length;
                        memcpy(out, (char *) src_buf + cp_off, cp_size);
                        out += cp_size;
                        size -= cp_size;
                } else if (cmd) {
                        if (cmd > size || cmd > top - data)
-                               break;
+                               goto bad_length;
                        memcpy(out, data, cmd);
                        out += cmd;
                        data += cmd;
@@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 
        /* sanity check */
        if (data != top || size != 0) {
+               bad_length:
                error("delta replay has gone wild");
                bad:
                free(dst_buf);

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 7114c31ade75e6ef551c273cafa83fcfc0ba7c00..41dc947d3f17950f97d0d92e9c6a6b8b89d767b4 100755 (executable)
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -370,4 +370,34 @@ test_expect_failure \
      echo base >base &&
      test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'
 
+# \0 - empty base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+# \1 - trailing garbage command
+test_expect_success \
+    'apply delta with trailing garbage literal' \
+    'printf "\0\1\1X\1" > tail_garbage_literal &&
+     test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null'
+
+# \5 - five bytes in base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+# \221 - copy, one byte offset, one byte size
+#   \0 - copy from offset 0
+#   \1 - copy 1 byte
+test_expect_success \
+    'apply delta with trailing garbage copy' \
+    'printf "\5\1\1X\221\0\1" > tail_garbage_copy &&
+     echo base >base &&
+     test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null'
+
+# \0 - empty base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+# \0 - bogus opcode
+test_expect_success \
+    'apply delta with trailing garbage opcode' \
+    'printf "\0\1\1X\0" > tail_garbage_opcode &&
+     test_must_fail test-tool delta -p /dev/null tail_garbage_opcode /dev/null'
+
 test_done