Merge branch 'jk/patch-corrupted-delta-fix'

Malformed or crafted data in packstream can make our code attempt
to read or write past the allocated buffer and abort, instead of
reporting an error, which has been fixed.

* jk/patch-corrupted-delta-fix:
  t5303: use printf to generate delta bases
  patch-delta: handle truncated copy parameters
  patch-delta: consistently report corruption
  patch-delta: fix oob read
  t5303: test some corrupt deltas
  test-delta: read input into a heap buffer

patch-delta.c

@@ -40,24 +40,31 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 		cmd = *data++;
 		if (cmd & 0x80) {
 			unsigned long cp_off = 0, cp_size = 0;
-			if (cmd & 0x01) cp_off = *data++;
-			if (cmd & 0x02) cp_off |= (*data++ << 8);
-			if (cmd & 0x04) cp_off |= (*data++ << 16);
-			if (cmd & 0x08) cp_off |= ((unsigned) *data++ << 24);
-			if (cmd & 0x10) cp_size = *data++;
-			if (cmd & 0x20) cp_size |= (*data++ << 8);
-			if (cmd & 0x40) cp_size |= (*data++ << 16);
+#define PARSE_CP_PARAM(bit, var, shift) do { \
+			if (cmd & (bit)) { \
+				if (data >= top) \
+					goto bad_length; \
+				var |= ((unsigned) *data++ << (shift)); \
+			} } while (0)
+			PARSE_CP_PARAM(0x01, cp_off, 0);
+			PARSE_CP_PARAM(0x02, cp_off, 8);
+			PARSE_CP_PARAM(0x04, cp_off, 16);
+			PARSE_CP_PARAM(0x08, cp_off, 24);
+			PARSE_CP_PARAM(0x10, cp_size, 0);
+			PARSE_CP_PARAM(0x20, cp_size, 8);
+			PARSE_CP_PARAM(0x40, cp_size, 16);
+#undef PARSE_CP_PARAM
 			if (cp_size == 0) cp_size = 0x10000;
 			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)
-				break;
+			if (cmd > size || cmd > top - data)
+				goto bad_length;
 			memcpy(out, data, cmd);
 			out += cmd;
 			data += cmd;
@@ -75,6 +82,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);

t/helper/test-delta.c

@@ -34,8 +34,8 @@ int cmd__delta(int argc, const char **argv)
 		return 1;
 	}
 	from_size = st.st_size;
-	from_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (from_buf == MAP_FAILED) {
+	from_buf = xmalloc(from_size);
+	if (read_in_full(fd, from_buf, from_size) < 0) {
 		perror(argv[2]);
 		close(fd);
 		return 1;
@@ -48,8 +48,8 @@ int cmd__delta(int argc, const char **argv)
 		return 1;
 	}
 	data_size = st.st_size;
-	data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (data_buf == MAP_FAILED) {
+	data_buf = xmalloc(data_size);
+	if (read_in_full(fd, data_buf, data_size) < 0) {
 		perror(argv[3]);
 		close(fd);
 		return 1;

t/t5303-pack-corruption-resilience.sh

@@ -311,4 +311,93 @@ 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'
 
+# \0 - empty base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+test_expect_success \
+    'apply good minimal delta' \
+    'printf "\0\1\1X" > minimal_delta &&
+     test-tool delta -p /dev/null minimal_delta /dev/null'
+
+# \0 - empty base
+# \1 - 1 byte in result
+# \2 - two literal bytes (one too many)
+test_expect_success \
+    'apply delta with too many literal bytes' \
+    'printf "\0\1\2XX" > too_big_literal &&
+     test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null'
+
+# \4 - four 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 too many copied bytes' \
+    'printf "\4\1\221\0\2" > too_big_copy &&
+     printf base >base &&
+     test_must_fail test-tool delta -p base too_big_copy /dev/null'
+
+# \0 - empty base
+# \2 - two bytes in result
+# \2 - two literal bytes (we are short one)
+test_expect_success \
+    'apply delta with too few literal bytes' \
+    'printf "\0\2\2X" > truncated_delta &&
+     test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null'
+
+# \0 - empty base
+# \1 - one byte in result
+# \221 - copy, one byte offset, one byte size
+#   \0 - copy from offset 0
+#   \1 - copy one byte (we are short one)
+test_expect_success \
+    'apply delta with too few bytes in base' \
+    'printf "\0\1\221\0\1" > truncated_base &&
+     test_must_fail test-tool delta -p /dev/null truncated_base /dev/null'
+
+# \4 - four bytes in base
+# \2 - two bytes in result
+# \1 - one literal byte (X)
+# \221 - copy, one byte offset, one byte size
+#        (offset/size missing)
+#
+# Note that the literal byte is necessary to get past the uninteresting minimum
+# delta size check.
+test_expect_success \
+    'apply delta with truncated copy parameters' \
+    'printf "\4\2\1X\221" > truncated_copy_delta &&
+     printf 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'
+
+# \4 - four 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 "\4\1\1X\221\0\1" > tail_garbage_copy &&
+     printf 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