{"thread":{"id":"49254","subject":"[PATCH 1/3] patch-delta: fix oob read","startedAt":"2018-08-29T20:59:04Z","lastAt":"2018-08-31T21:55:57Z","messageCount":31,"participants":["Jann Horn","Jeff King","Nicolas Pitre","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"356879","messageId":"20180829205857.77340-1-jannh@google.com","threadId":"49254","inReplyTo":null,"subject":"[PATCH 1/3] patch-delta: fix oob read","fromName":"Jann Horn","fromEmail":"jannh@google.com","sentAt":"2018-08-29T20:58:55Z","receivedAt":"2018-08-29T20:59:04Z","isPatch":true,"sender":{"key":"jannh@google.com","avatar":"https://avatars.githubusercontent.com/u/499370?v=4"},"body":"If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the\n`memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`\ninto `dst_buf`.\n\nThis is not an exploitable bug because triggering the bug increments the\n`data` pointer beyond `top`, causing the `data != top` sanity check after\nthe loop to trigger and discard the destination buffer - which means that\nthe result of the out-of-bounds read is never used for anything.\n\nAlso, directly jump into the error handler instead of just breaking out of\nthe loop - otherwise, data corruption would be silently ignored if the\ndelta buffer ends with a command and the destination buffer is already\nfull.\n\nSigned-off-by: Jann Horn <jannh@google.com>\n---\n patch-delta.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/patch-delta.c b/patch-delta.c\nindex 56e0a5ede..283fb4b75 100644\n--- a/patch-delta.c\n+++ b/patch-delta.c\n@@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n \t\t\tif (unsigned_add_overflows(cp_off, cp_size) ||\n \t\t\t    cp_off + cp_size > src_size ||\n \t\t\t    cp_size > size)\n-\t\t\t\tbreak;\n+\t\t\t\tgoto bad_length;\n \t\t\tmemcpy(out, (char *) src_buf + cp_off, cp_size);\n \t\t\tout += cp_size;\n \t\t\tsize -= cp_size;\n \t\t} else if (cmd) {\n-\t\t\tif (cmd > size)\n-\t\t\t\tbreak;\n+\t\t\tif (cmd > size || cmd > top - data)\n+\t\t\t\tgoto bad_length;\n \t\t\tmemcpy(out, data, cmd);\n \t\t\tout += cmd;\n \t\t\tdata += cmd;\n@@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n \n \t/* sanity check */\n \tif (data != top || size != 0) {\n+\t\tbad_length:\n \t\terror(\"delta replay has gone wild\");\n \t\tbad:\n \t\tfree(dst_buf);\n-- \n2.19.0.rc0.228.g281dcd1b4d0-goog\n\n"},{"id":"356880","messageId":"20180829205857.77340-2-jannh@google.com","threadId":"49254","inReplyTo":"20180829205857.77340-1-jannh@google.com","subject":"[PATCH 2/3] t/helper/test-delta: segfault on OOB access","fromName":"Jann Horn","fromEmail":"jannh@google.com","sentAt":"2018-08-29T20:58:56Z","receivedAt":"2018-08-29T20:59:07Z","isPatch":true,"sender":{"key":"jannh@google.com","avatar":"https://avatars.githubusercontent.com/u/499370?v=4"},"body":"This ensures that any attempts to access memory directly after the input\nbuffer or delta buffer in a delta test will cause a segmentation fault.\n\nInspired by vsftpd.\n\nSigned-off-by: Jann Horn <jannh@google.com>\n---\n t/helper/test-delta.c | 78 +++++++++++++++++++++++++++++--------------\n 1 file changed, 53 insertions(+), 25 deletions(-)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 34c725924..64d0ec902 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -16,45 +16,73 @@\n static const char usage_str[] =\n \t\"test-tool delta (-d|-p) <from_file> <data_file> <out_file>\";\n \n-int cmd__delta(int argc, const char **argv)\n+/*\n+ * We want to detect OOB reads behind the resulting buffer, even in non-ASAN\n+ * builds. This helper reads some data into memory, aligns the *end* of the\n+ * buffer on a page boundary, and reserves the next virtual page. This ensures\n+ * that a single-byte OOB access segfaults.\n+ */\n+static void *map_with_adjacent_trailing_guard(const char *path,\n+\t\t\t\t\t      unsigned long *sizep)\n {\n \tint fd;\n \tstruct stat st;\n-\tvoid *from_buf, *data_buf, *out_buf;\n-\tunsigned long from_size, data_size, out_size;\n+\tunsigned long page_size = getpagesize();\n+\tunsigned long padded_size, padding;\n \n-\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n-\t\tfprintf(stderr, \"usage: %s\\n\", usage_str);\n-\t\treturn 1;\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0) {\n+\t\tperror(path);\n+\t\treturn NULL;\n \t}\n+\tif (fstat(fd, &st)) {\n+\t\tperror(path);\n+\t\tclose(fd);\n+\t\treturn NULL;\n+\t}\n+\t*sizep = st.st_size;\n \n-\tfd = open(argv[2], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[2]);\n-\t\treturn 1;\n+\t/* pad in front for alignment and add trailing page */\n+\tpadded_size = ((page_size-1) + st.st_size + page_size) & ~(page_size-1);\n+\tpadding = padded_size - (st.st_size + page_size);\n+\n+\tchar *mapping = mmap(NULL, padded_size, PROT_READ|PROT_WRITE,\n+\t\t\t     MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);\n+\tif (mapping == MAP_FAILED ||\n+\t    mprotect(mapping + padded_size - page_size, page_size, PROT_NONE)) {\n+\t\tperror(\"mmap\");\n+\t\tclose(fd);\n+\t\treturn NULL;\n \t}\n-\tfrom_size = st.st_size;\n-\tfrom_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (from_buf == MAP_FAILED) {\n-\t\tperror(argv[2]);\n+\tif (read_in_full(fd, mapping + padding, st.st_size) != st.st_size) {\n+\t\tperror(\"read_in_full\");\n+\t\tmunmap(mapping, padded_size);\n \t\tclose(fd);\n-\t\treturn 1;\n+\t\treturn NULL;\n \t}\n+\tmprotect(mapping, padded_size - page_size, PROT_READ);\n \tclose(fd);\n+\treturn mapping + padding;\n+}\n \n-\tfd = open(argv[3], O_RDONLY);\n-\tif (fd < 0 || fstat(fd, &st)) {\n-\t\tperror(argv[3]);\n+int cmd__delta(int argc, const char **argv)\n+{\n+\tint fd;\n+\tvoid *from_buf, *data_buf, *out_buf;\n+\tunsigned long from_size, data_size, out_size;\n+\n+\tif (argc != 5 || (strcmp(argv[1], \"-d\") && strcmp(argv[1], \"-p\"))) {\n+\t\tfprintf(stderr, \"usage: %s\\n\", usage_str);\n \t\treturn 1;\n \t}\n-\tdata_size = st.st_size;\n-\tdata_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (data_buf == MAP_FAILED) {\n-\t\tperror(argv[3]);\n-\t\tclose(fd);\n+\n+\tfrom_buf = map_with_adjacent_trailing_guard(argv[2], &from_size);\n+\tif (from_buf == NULL)\n+\t\treturn 1;\n+\n+\tdata_buf = map_with_adjacent_trailing_guard(argv[3], &data_size);\n+\tif (data_buf == NULL)\n \t\treturn 1;\n-\t}\n-\tclose(fd);\n \n \tif (argv[1][1] == 'd')\n \t\tout_buf = diff_delta(from_buf, from_size,\n-- \n2.19.0.rc0.228.g281dcd1b4d0-goog\n\n"},{"id":"356881","messageId":"20180829205857.77340-3-jannh@google.com","threadId":"49254","inReplyTo":"20180829205857.77340-1-jannh@google.com","subject":"[PATCH 3/3] t5303: add tests for corrupted deltas","fromName":"Jann Horn","fromEmail":"jannh@google.com","sentAt":"2018-08-29T20:58:57Z","receivedAt":"2018-08-29T20:59:10Z","isPatch":true,"sender":{"key":"jannh@google.com","avatar":"https://avatars.githubusercontent.com/u/499370?v=4"},"body":"This verifies the changes from commit \"patch-delta: fix oob read\".\n\nSigned-off-by: Jann Horn <jannh@google.com>\n---\n t/t5303-pack-corruption-resilience.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 3634e258f..7152376b6 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -311,4 +311,22 @@ test_expect_success \\\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n \n+test_expect_success \\\n+    'apply good minimal delta' \\\n+    'printf \"\\x00\\x01\\x01X\" > minimal_delta &&\n+     test-tool delta -p /dev/null minimal_delta /dev/null\n+     '\n+\n+test_expect_success \\\n+    'apply truncated delta' \\\n+    'printf \"\\x00\\x02\\x02X\" > truncated_delta &&\n+     test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null\n+     '\n+\n+test_expect_success \\\n+    'apply delta with trailing garbage command' \\\n+    'printf \"\\x00\\x01\\x01X\\x01\" > tail_garbage_delta &&\n+     test_must_fail test-tool delta -p /dev/null tail_garbage_delta /dev/null\n+     '\n+\n test_done\n-- \n2.19.0.rc0.228.g281dcd1b4d0-goog\n\n"},{"id":"356889","messageId":"20180829212025.GB29880@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180829205857.77340-1-jannh@google.com","subject":"Re: [PATCH 1/3] patch-delta: fix oob read","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T21:20:25Z","receivedAt":"2018-08-29T21:20:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:\n\n> If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the\n> `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`\n> into `dst_buf`.\n> \n> This is not an exploitable bug because triggering the bug increments the\n> `data` pointer beyond `top`, causing the `data != top` sanity check after\n> the loop to trigger and discard the destination buffer - which means that\n> the result of the out-of-bounds read is never used for anything.\n> \n> Also, directly jump into the error handler instead of just breaking out of\n> the loop - otherwise, data corruption would be silently ignored if the\n> delta buffer ends with a command and the destination buffer is already\n> full.\n\nNice catch. The patch looks good to me, but just to lay out my thought\nprocess looking for other related problems:\n\nWe have two types of instructions:\n\n  1. Take N bytes from position P within the source.\n\n  2. Take the next N bytes from the delta stream.\n\nIn both cases we need to check that:\n\n  a. We have enough space in the destination buffer.\n\n  b. We have enough source bytes.\n\nSo this:\n\n> diff --git a/patch-delta.c b/patch-delta.c\n> index 56e0a5ede..283fb4b75 100644\n> --- a/patch-delta.c\n> +++ b/patch-delta.c\n> @@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n>  \t\t\tif (unsigned_add_overflows(cp_off, cp_size) ||\n>  \t\t\t    cp_off + cp_size > src_size ||\n>  \t\t\t    cp_size > size)\n> -\t\t\t\tbreak;\n> +\t\t\t\tgoto bad_length;\n>  \t\t\tmemcpy(out, (char *) src_buf + cp_off, cp_size);\n\nCovers 1a (cp_size > size) and 1b (cp_off + cp_size > src_size), plus\nthe obvious overflow possibility.\n\nAnd then here:\n\n>  \t\t} else if (cmd) {\n> -\t\t\tif (cmd > size)\n> -\t\t\t\tbreak;\n> +\t\t\tif (cmd > size || cmd > top - data)\n> +\t\t\t\tgoto bad_length;\n>  \t\t\tmemcpy(out, data, cmd);\n\nWe had previously dealt with 2a (cmd > size), but failed to handle 2b,\nwhich you've added. We don't need to deal with over/underflow here,\nbecause our subtraction is on pointers to the same buffer.\n\n> @@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n>  \n>  \t/* sanity check */\n>  \tif (data != top || size != 0) {\n> +\t\tbad_length:\n>  \t\terror(\"delta replay has gone wild\");\n>  \t\tbad:\n>  \t\tfree(dst_buf);\n\nAnd I agree that jumping straight here is a good idea.\n\nOverall, very nicely done.\n\n-Peff\n"},{"id":"356894","messageId":"20180829213433.GC29880@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180829205857.77340-2-jannh@google.com","subject":"Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T21:34:34Z","receivedAt":"2018-08-29T21:34:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 10:58:56PM +0200, Jann Horn wrote:\n\n> This ensures that any attempts to access memory directly after the input\n> buffer or delta buffer in a delta test will cause a segmentation fault.\n> \n> Inspired by vsftpd.\n\nNeat trick, but it seems funny to protect this one buffer in\nnon-production code. Obviously you were interested in demonstrating the\nissue for your tests, but do we want to carry this all the time?\n\nIf we want to detect this kind of thing in tests, we should probably be\nrelying on tools like ASan, which would cover all mmaps.\n\nIt would be nice if there was a low-cost way to detect this in\nproduction use, but it looks like this replaces mmap with\nread_in_full(), which I think is a non-starter for most uses.\n\n-Peff\n"},{"id":"356896","messageId":"CAG48ez2BL1fgP_FkUi=6EYAuA+ja3ujZFFqKkPQN50tRCEUJiQ@mail.gmail.com","threadId":"49254","inReplyTo":"20180829213433.GC29880@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access","fromName":"Jann Horn","fromEmail":"jannh@google.com","sentAt":"2018-08-29T21:40:41Z","receivedAt":"2018-08-29T21:41:10Z","isPatch":true,"sender":{"key":"jannh@google.com","avatar":"https://avatars.githubusercontent.com/u/499370?v=4"},"body":"On Wed, Aug 29, 2018 at 11:34 PM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Aug 29, 2018 at 10:58:56PM +0200, Jann Horn wrote:\n>\n> > This ensures that any attempts to access memory directly after the input\n> > buffer or delta buffer in a delta test will cause a segmentation fault.\n> >\n> > Inspired by vsftpd.\n>\n> Neat trick, but it seems funny to protect this one buffer in\n> non-production code. Obviously you were interested in demonstrating the\n> issue for your tests, but do we want to carry this all the time?\n>\n> If we want to detect this kind of thing in tests, we should probably be\n> relying on tools like ASan, which would cover all mmaps.\n>\n> It would be nice if there was a low-cost way to detect this in\n> production use, but it looks like this replaces mmap with\n> read_in_full(), which I think is a non-starter for most uses.\n\nI think even with ASAN, you'd still need read_in_full() or an mmap()\nwrapper that fiddles with the ASAN shadow, because mmap() always maps\nwhole pages:\n\n$ cat mmap-read-asan-blah.c\n#include <sys/mman.h>\n#include <stdlib.h>\nint main(void) {\n  volatile char *p = mmap(NULL, 1, PROT_READ|PROT_WRITE,\nMAP_PRIVATE|MAP_ANONYMOUS, -1, 0);\n  p[200] = 1;\n}\n$ gcc -o mmap-read-asan-blah mmap-read-asan-blah.c -fsanitize=address\n$ ./mmap-read-asan-blah\n$\n\nBut that aside, you do have a point about having some custom hack for\na single patch.\n"},{"id":"356898","messageId":"20180829214621.GD29880@sigill.intra.peff.net","threadId":"49254","inReplyTo":"CAG48ez2BL1fgP_FkUi=6EYAuA+ja3ujZFFqKkPQN50tRCEUJiQ@mail.gmail.com","subject":"Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T21:46:21Z","receivedAt":"2018-08-29T21:46:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 11:40:41PM +0200, Jann Horn wrote:\n\n> > If we want to detect this kind of thing in tests, we should probably be\n> > relying on tools like ASan, which would cover all mmaps.\n> >\n> > It would be nice if there was a low-cost way to detect this in\n> > production use, but it looks like this replaces mmap with\n> > read_in_full(), which I think is a non-starter for most uses.\n> \n> I think even with ASAN, you'd still need read_in_full() or an mmap()\n> wrapper that fiddles with the ASAN shadow, because mmap() always maps\n> whole pages:\n> \n> $ cat mmap-read-asan-blah.c\n> #include <sys/mman.h>\n> #include <stdlib.h>\n> int main(void) {\n>   volatile char *p = mmap(NULL, 1, PROT_READ|PROT_WRITE,\n> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);\n>   p[200] = 1;\n> }\n> $ gcc -o mmap-read-asan-blah mmap-read-asan-blah.c -fsanitize=address\n> $ ./mmap-read-asan-blah\n> $\n\nYeah, I was just trying to run your tests with ASan and couldn't\nconvince it to complain. I also tried MSan, but no luck.\n\n> But that aside, you do have a point about having some custom hack for\n> a single patch.\n\nI'm also not sure how portable it is. Looks like we have a Windows\nwrapper for getpagesize(), but I don't see any other uses of mprotect().\n\n-Peff\n"},{"id":"356899","messageId":"20180829214836.GE29880@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180829214621.GD29880@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T21:48:36Z","receivedAt":"2018-08-29T21:48:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 05:46:21PM -0400, Jeff King wrote:\n\n> > I think even with ASAN, you'd still need read_in_full() or an mmap()\n> > wrapper that fiddles with the ASAN shadow, because mmap() always maps\n> > whole pages:\n> > \n> > $ cat mmap-read-asan-blah.c\n> > #include <sys/mman.h>\n> > #include <stdlib.h>\n> > int main(void) {\n> >   volatile char *p = mmap(NULL, 1, PROT_READ|PROT_WRITE,\n> > MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);\n> >   p[200] = 1;\n> > }\n> > $ gcc -o mmap-read-asan-blah mmap-read-asan-blah.c -fsanitize=address\n> > $ ./mmap-read-asan-blah\n> > $\n> \n> Yeah, I was just trying to run your tests with ASan and couldn't\n> convince it to complain. I also tried MSan, but no luck.\n> \n> > But that aside, you do have a point about having some custom hack for\n> > a single patch.\n> \n> I'm also not sure how portable it is. Looks like we have a Windows\n> wrapper for getpagesize(), but I don't see any other uses of mprotect().\n\nActually, there is no real need for this test helper to use mmap. I\nsuppose we could swap it out for malloc + read_in_full() and let ASan\n(or even valgrind) handle the tricky parts.\n\n-Peff\n"},{"id":"356900","messageId":"20180829220353.GF29880@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180829205857.77340-3-jannh@google.com","subject":"Re: [PATCH 3/3] t5303: add tests for corrupted deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T22:03:53Z","receivedAt":"2018-08-29T22:03:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 10:58:57PM +0200, Jann Horn wrote:\n\n> This verifies the changes from commit \"patch-delta: fix oob read\".\n\nA minor nit, but usually we'd either introduce tests along with the\nfix, or introduce them beforehand as test_expect_failure and then flip\nthem to success along with the fix.\n\n> diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\n> index 3634e258f..7152376b6 100755\n> --- a/t/t5303-pack-corruption-resilience.sh\n> +++ b/t/t5303-pack-corruption-resilience.sh\n> @@ -311,4 +311,22 @@ test_expect_success \\\n>       test_must_fail git cat-file blob $blob_2 > /dev/null &&\n>       test_must_fail git cat-file blob $blob_3 > /dev/null'\n>  \n> +test_expect_success \\\n> +    'apply good minimal delta' \\\n> +    'printf \"\\x00\\x01\\x01X\" > minimal_delta &&\n> +     test-tool delta -p /dev/null minimal_delta /dev/null\n> +     '\n\nWithout your second patch applied, this complains about mmap-ing\n/dev/null (or any zero-length file).\n\nAlso, \\x escapes are sadly not portable (dash, for example, does not\nrespect them). You have to use octal instead (which is not too onerous\nfor these small numbers).\n\nI needed the patch below to get it to behave as expected (I also\nannotated the deltas to make it more comprehensible to somebody who\nhasn't just been digging in the patch code ;) ).\n\nI wonder if we should more fully test the 4 cases I outlined in my\nearlier mail, too.\n\n-Peff\n\n---\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 7152376b67..df28cce68b 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -311,22 +311,35 @@ test_expect_success \\\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n \n+# \\5 - five bytes in base (though we do not use it)\n+# \\1 - one byte in result\n+# \\1 - copy one byte (X)\n test_expect_success \\\n     'apply good minimal delta' \\\n-    'printf \"\\x00\\x01\\x01X\" > minimal_delta &&\n-     test-tool delta -p /dev/null minimal_delta /dev/null\n+    'printf \"\\5\\1\\1X\" > minimal_delta &&\n+     echo base >base &&\n+     test-tool delta -p base minimal_delta /dev/null\n      '\n \n+# \\5 - five bytes in base (though we do not use it)\n+# \\2 - two bytes in result\n+# \\2 - copy two bytes (we are short one)\n test_expect_success \\\n     'apply truncated delta' \\\n-    'printf \"\\x00\\x02\\x02X\" > truncated_delta &&\n-     test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null\n+    'printf \"\\5\\2\\2X\" > truncated_delta &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base truncated_delta /dev/null\n      '\n \n+# \\5 - five bytes in base (though we do not use it)\n+# \\1 - one byte in result\n+# \\1 - copy one byte (X)\n+# \\1 - trailing garbage command\n test_expect_success \\\n     'apply delta with trailing garbage command' \\\n-    'printf \"\\x00\\x01\\x01X\\x01\" > tail_garbage_delta &&\n-     test_must_fail test-tool delta -p /dev/null tail_garbage_delta /dev/null\n+    'printf \"\\5\\1\\1X\\1\" > tail_garbage_delta &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base tail_garbage_delta /dev/null\n      '\n \n test_done\n"},{"id":"356902","messageId":"20180829221838.GA10400@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180829212025.GB29880@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] patch-delta: fix oob read","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T22:18:38Z","receivedAt":"2018-08-29T22:18:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 05:20:25PM -0400, Jeff King wrote:\n\n> Nice catch. The patch looks good to me, but just to lay out my thought\n> process looking for other related problems:\n> \n> We have two types of instructions:\n> \n>   1. Take N bytes from position P within the source.\n> \n>   2. Take the next N bytes from the delta stream.\n> \n> In both cases we need to check that:\n> \n>   a. We have enough space in the destination buffer.\n> \n>   b. We have enough source bytes.\n\nActually, there's one other case to consider: reading the actual offset\nfor a copy operation. E.g., if we see '0x80' at the end of the delta,\nI think we'd read garbage into cp_offset? That would typically generate\nnonsense that would be caught by the other checks, but I think it's\npossible that the memory could happen to produce a valid copy\ninstruction from the base, leading to a confusing result (though it\nwould still need to match the expected result size).\n\nThinking on it more, one other interesting tidbit here: in actual git\ncode (i.e., not test-delta), we'd always be reading from an mmap'd\npackfile. And we're guaranteed to have at least 20 trailing bytes in the\nmmap due to the pack format (which is enforced when we parse the pack).\n\nSo I don't think this can ever read truly out-of-bounds memory, though\nobviously it's something we should fix regardless.\n\n-Peff\n"},{"id":"356903","messageId":"20180829223048.GA12624@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180829220353.GF29880@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] t5303: add tests for corrupted deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T22:30:48Z","receivedAt":"2018-08-29T22:30:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 06:03:53PM -0400, Jeff King wrote:\n\n> Without your second patch applied, this complains about mmap-ing\n> /dev/null (or any zero-length file).\n> \n> Also, \\x escapes are sadly not portable (dash, for example, does not\n> respect them). You have to use octal instead (which is not too onerous\n> for these small numbers).\n> \n> I needed the patch below to get it to behave as expected (I also\n> annotated the deltas to make it more comprehensible to somebody who\n> hasn't just been digging in the patch code ;) ).\n> \n> I wonder if we should more fully test the 4 cases I outlined in my\n> earlier mail, too.\n\nSo here's a version which checks each of those cases (plus your minimal\nbase-case and the trailing garbage case from your original).\n\nI did it as a straight patch rather than on top of yours, since I think\nthat's easier to read (and I'd expect us to squash together whatever we\nend up with anyway).\n\nThis doesn't cover out-of-bounds reads while parsing the offset/size.\nIt's dinner-time here, but I may take a look at that later tonight.\n\n---\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 3634e258f8..305922eeb3 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -311,4 +311,81 @@ test_expect_success \\\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n \n+# Base sanity check; this is the smallest possible delta.\n+#\n+# \\5 - five bytes in base (though we do not use it)\n+# \\1 - one byte in result\n+# \\1 - copy one byte (X)\n+test_expect_success \\\n+    'apply good minimal delta' \\\n+    'printf \"\\5\\1\\1X\" > minimal_delta &&\n+     echo base >base &&\n+     test-tool delta -p base minimal_delta /dev/null\n+     '\n+\n+# This delta has too many literal bytes to fit in destination.\n+#\n+# \\5 - five bytes in base (though we do not use it)\n+# \\1 - 1 byte in result\n+# \\2 - copy two bytes (one too many)\n+test_expect_success \\\n+    'apply truncated delta' \\\n+    'printf \"\\5\\1\\2XX\" > too_big_literal &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base too_big_literal /dev/null\n+     '\n+\n+# This delta has too many copy bytes to fit in destination.\n+#\n+# \\5 - five bytes in base\n+# \\1 - one byte in result\n+# \\221 - copy, one byte offset, one byte size\n+#   \\0 - copy from offset 0\n+#   \\2 - copy two bytes (one too many)\n+test_expect_success \\\n+    'apply delta with trailing garbage command' \\\n+    'printf \"\\5\\1\\221\\0\\2\" > too_big_copy &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base too_big_copy /dev/null\n+     '\n+\n+# This delta has too few bytes in the delta itself.\n+#\n+# \\5 - five bytes in base (though we do not use it)\n+# \\2 - two bytes in result\n+# \\2 - copy two bytes (we are short one)\n+test_expect_success \\\n+    'apply truncated delta' \\\n+    'printf \"\\5\\2\\2X\" > truncated_delta &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base truncated_delta /dev/null\n+     '\n+\n+# This delta has too few bytes in the base.\n+#\n+# \\5 - five bytes in base\n+# \\6 - six bytes in result\n+# \\221 - copy, one byte offset, one byte size\n+#   \\0 - copy from offset 0\n+#   \\6 - copy six bytes (one too many)\n+test_expect_success \\\n+    'apply delta with trailing garbage command' \\\n+    'printf \"\\5\\6\\221\\0\\6\" > truncated_base &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base truncated_base /dev/null\n+     '\n+\n+# Trailing garbage command.\n+#\n+# \\5 - five bytes in base (though we do not use it)\n+# \\1 - one byte in result\n+# \\1 - copy one byte (X)\n+# \\1 - trailing garbage command\n+test_expect_success \\\n+    'apply delta with trailing garbage command' \\\n+    'printf \"\\5\\1\\1X\\1\" > tail_garbage_delta &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base tail_garbage_delta /dev/null\n+     '\n+\n test_done\n"},{"id":"356937","messageId":"20180830070548.GA15081@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180829205857.77340-1-jannh@google.com","subject":"[PATCH 0/5] handle corruption in patch-delta","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T07:05:48Z","receivedAt":"2018-08-30T07:05:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:\n\n> If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the\n> `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`\n> into `dst_buf`.\n> \n> This is not an exploitable bug because triggering the bug increments the\n> `data` pointer beyond `top`, causing the `data != top` sanity check after\n> the loop to trigger and discard the destination buffer - which means that\n> the result of the out-of-bounds read is never used for anything.\n> \n> Also, directly jump into the error handler instead of just breaking out of\n> the loop - otherwise, data corruption would be silently ignored if the\n> delta buffer ends with a command and the destination buffer is already\n> full.\n\nBased on my earlier observations, here's a replacement patch series I\ncame up with. It has:\n\n  [1/5]: test-delta: read input into a heap buffer\n\n    A simpler replacement for your patch 2 which avoids portability\n    issues.\n\n  [2/5]: t5303: test some corrupt deltas\n\n    A more complete set of boundary tests based on the 4 cases I laid\n    out, plus the cp_size problem I found.\n\n  [3/5]: patch-delta: fix oob read\n\n    Your actual fix.\n\n  [4/5]: patch-delta: consistently report corruption\n\n    Your related trailing-garbage fix. I split this into two in order to\n    better demonstrate the cases this part covers.\n\n  [5/5]: patch-delta: handle truncated copy parameters\n\n    My fix for the cp_size read.\n\nI hope you don't mind me hacking up your patches a bit. Thanks again for\nyour original report and patch.\n\n-Peff\n"},{"id":"356938","messageId":"20180830070751.GA15420@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180830070548.GA15081@sigill.intra.peff.net","subject":"[PATCH 1/5] test-delta: read input into a heap buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T07:07:52Z","receivedAt":"2018-08-30T07:07:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently read the input to test-delta by mmap()-ing it.\nHowever, memory-checking tools like valgrind and ASan are\nless able to detect reads/writes past the end of an mmap'd\nbuffer, because the OS is likely to give us extra bytes to\npad out the final page size. So instead, let's read into a\nheap buffer.\n\nAs a bonus, this also makes it possible to write tests with\nempty bases, as mmap() will complain about a zero-length\nmap.\n\nThis is based on a patch by Jann Horn <jannh@google.com>\nwhich actually aligned the data at the end of a page, and\nfollowed it with another page marked with mprotect(). That\nwould detect problems even without a tool like ASan, but it\nwas significantly more complex and may have introduced\nportability problems. By comparison, this approach pushes\nthe complexity onto existing memory-checking tools.\n\nNote that this could be done even more simply by using\nstrbuf_read_file(), but that would defeat the purpose:\nstrbufs generally overallocate (and at the very least\ninclude a trailing NUL which we do not care about), which\nwould defeat most memory checkers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-delta.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 34c7259248..e749a49c88 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -34,8 +34,8 @@ int cmd__delta(int argc, const char **argv)\n \t\treturn 1;\n \t}\n \tfrom_size = st.st_size;\n-\tfrom_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (from_buf == MAP_FAILED) {\n+\tfrom_buf = xmalloc(from_size);\n+\tif (read_in_full(fd, from_buf, from_size) < 0) {\n \t\tperror(argv[2]);\n \t\tclose(fd);\n \t\treturn 1;\n@@ -48,8 +48,8 @@ int cmd__delta(int argc, const char **argv)\n \t\treturn 1;\n \t}\n \tdata_size = st.st_size;\n-\tdata_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tif (data_buf == MAP_FAILED) {\n+\tdata_buf = xmalloc(data_size);\n+\tif (read_in_full(fd, data_buf, data_size) < 0) {\n \t\tperror(argv[3]);\n \t\tclose(fd);\n \t\treturn 1;\n-- \n2.19.0.rc1.539.g3876d0831e\n\n"},{"id":"356939","messageId":"20180830070932.GB15420@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180830070548.GA15081@sigill.intra.peff.net","subject":"[PATCH 2/5] t5303: test some corrupt deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T07:09:32Z","receivedAt":"2018-08-30T07:09:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't have any tests that specifically check boundary\ncases in patch_delta(). It obviously gets exercised by tests\nwhich read from packfiles, but it's hard to create packfiles\nwith bogus deltas.\n\nSo let's cover some obvious boundary cases:\n\n  1. commands that overflow the result buffer\n\n     a. literal content from the delta\n\n     b. copies from a base\n\n  2. commands where the source isn't large enough\n\n     a. literal content from a truncated delta\n\n     b. copies that need more bytes than the base has\n\n  3. copy commands who parameters are truncated\n\nAnd indeed, we have problems with both 2a and 3. I've marked\nthese both as expect_failure, though note that because they\ninvolve reading past the end of a buffer, they will\ntypically only be caught when run under valgrind or ASan.\n\nThere's one more test here, too, which just applies a basic\ndelta. Since all of the other tests expect failure and we\ndon't otherwise use \"test-tool delta\" in the test suite,\nthis gives a sanity check that the tool works at all.\n\nThese are based on an earlier patch by Jann Horn\n<jannh@google.com>.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5303-pack-corruption-resilience.sh | 59 +++++++++++++++++++++++++++\n 1 file changed, 59 insertions(+)\n\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 3634e258f8..912e659acf 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -311,4 +311,63 @@ test_expect_success \\\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n \n+# \\0 - empty base\n+# \\1 - one byte in result\n+# \\1 - one literal byte (X)\n+test_expect_success \\\n+    'apply good minimal delta' \\\n+    'printf \"\\0\\1\\1X\" > minimal_delta &&\n+     test-tool delta -p /dev/null minimal_delta /dev/null'\n+\n+# \\0 - empty base\n+# \\1 - 1 byte in result\n+# \\2 - two literal bytes (one too many)\n+test_expect_success \\\n+    'apply delta with too many literal bytes' \\\n+    'printf \"\\0\\1\\2XX\" > too_big_literal &&\n+     test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null'\n+\n+# \\5 - five bytes in base\n+# \\1 - one byte in result\n+# \\221 - copy, one byte offset, one byte size\n+#   \\0 - copy from offset 0\n+#   \\2 - copy two bytes (one too many)\n+test_expect_success \\\n+    'apply delta with too many copied bytes' \\\n+    'printf \"\\5\\1\\221\\0\\2\" > too_big_copy &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base too_big_copy /dev/null'\n+\n+# \\0 - empty base\n+# \\2 - two bytes in result\n+# \\2 - two literal bytes (we are short one)\n+test_expect_failure \\\n+    'apply delta with too few literal bytes' \\\n+    'printf \"\\0\\2\\2X\" > truncated_delta &&\n+     test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null'\n+\n+# \\0 - empty base\n+# \\1 - one byte in result\n+# \\221 - copy, one byte offset, one byte size\n+#   \\0 - copy from offset 0\n+#   \\1 - copy one byte (we are short one)\n+test_expect_success \\\n+    'apply delta with too few bytes in base' \\\n+    'printf \"\\0\\1\\221\\0\\1\" > truncated_base &&\n+     test_must_fail test-tool delta -p /dev/null truncated_base /dev/null'\n+\n+# \\5 - five bytes in base\n+# \\5 - five bytes in result\n+# \\1 - one literal byte (X)\n+# \\221 - copy, one byte offset, one byte size\n+#        (offset/size missing)\n+#\n+# Note that the literal byte is necessary to get past the uninteresting minimum\n+# delta size check.\n+test_expect_failure \\\n+    'apply delta with truncated copy parameters' \\\n+    'printf \"\\5\\5\\1X\\221\" > truncated_copy_delta &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'\n+\n test_done\n-- \n2.19.0.rc1.539.g3876d0831e\n\n"},{"id":"356940","messageId":"20180830070944.GC15420@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180830070548.GA15081@sigill.intra.peff.net","subject":"[PATCH 3/5] patch-delta: fix oob read","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T07:09:45Z","receivedAt":"2018-08-30T07:09:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jann Horn <jannh@google.com>\n\nIf `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the\n`memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`\ninto `dst_buf`.\n\nThis is not an exploitable bug because triggering the bug increments the\n`data` pointer beyond `top`, causing the `data != top` sanity check after\nthe loop to trigger and discard the destination buffer - which means that\nthe result of the out-of-bounds read is never used for anything.\n\nSigned-off-by: Jann Horn <jannh@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n patch-delta.c                         | 2 +-\n t/t5303-pack-corruption-resilience.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/patch-delta.c b/patch-delta.c\nindex 56e0a5ede2..b937afd2c9 100644\n--- a/patch-delta.c\n+++ b/patch-delta.c\n@@ -56,7 +56,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n \t\t\tout += cp_size;\n \t\t\tsize -= cp_size;\n \t\t} else if (cmd) {\n-\t\t\tif (cmd > size)\n+\t\t\tif (cmd > size || cmd > top - data)\n \t\t\t\tbreak;\n \t\t\tmemcpy(out, data, cmd);\n \t\t\tout += cmd;\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 912e659acf..7114c31ade 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -341,7 +341,7 @@ test_expect_success \\\n # \\0 - empty base\n # \\2 - two bytes in result\n # \\2 - two literal bytes (we are short one)\n-test_expect_failure \\\n+test_expect_success \\\n     'apply delta with too few literal bytes' \\\n     'printf \"\\0\\2\\2X\" > truncated_delta &&\n      test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null'\n-- \n2.19.0.rc1.539.g3876d0831e\n\n"},{"id":"356941","messageId":"20180830071026.GD15420@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180830070548.GA15081@sigill.intra.peff.net","subject":"[PATCH 4/5] patch-delta: consistently report corruption","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T07:10:26Z","receivedAt":"2018-08-30T07:10:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jann Horn <jannh@google.com>\n\nWhen applying a delta, if we see an opcode that cannot be\nfulfilled (e.g., asking to write more bytes than the\ndestination has left), we break out of our parsing loop but\ndon't signal an explicit error. We rely on the sanity check\nafter the loop to see if we have leftover delta bytes or\ndidn't fill our result buffer.\n\nThis can silently ignore corruption when the delta buffer\nends with a bogus command and the destination buffer is\nalready full. Instead, let's jump into the error handler\ndirectly when we see this case.\n\nNote that the tests also cover the \"bad opcode\" case, which\nalready handles this correctly.\n\nSigned-off-by: Jann Horn <jannh@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n patch-delta.c                         |  5 +++--\n t/t5303-pack-corruption-resilience.sh | 30 +++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/patch-delta.c b/patch-delta.c\nindex b937afd2c9..283fb4b759 100644\n--- a/patch-delta.c\n+++ b/patch-delta.c\n@@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n \t\t\tif (unsigned_add_overflows(cp_off, cp_size) ||\n \t\t\t    cp_off + cp_size > src_size ||\n \t\t\t    cp_size > size)\n-\t\t\t\tbreak;\n+\t\t\t\tgoto bad_length;\n \t\t\tmemcpy(out, (char *) src_buf + cp_off, cp_size);\n \t\t\tout += cp_size;\n \t\t\tsize -= cp_size;\n \t\t} else if (cmd) {\n \t\t\tif (cmd > size || cmd > top - data)\n-\t\t\t\tbreak;\n+\t\t\t\tgoto bad_length;\n \t\t\tmemcpy(out, data, cmd);\n \t\t\tout += cmd;\n \t\t\tdata += cmd;\n@@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n \n \t/* sanity check */\n \tif (data != top || size != 0) {\n+\t\tbad_length:\n \t\terror(\"delta replay has gone wild\");\n \t\tbad:\n \t\tfree(dst_buf);\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 7114c31ade..41dc947d3f 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -370,4 +370,34 @@ test_expect_failure \\\n      echo base >base &&\n      test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'\n \n+# \\0 - empty base\n+# \\1 - one byte in result\n+# \\1 - one literal byte (X)\n+# \\1 - trailing garbage command\n+test_expect_success \\\n+    'apply delta with trailing garbage literal' \\\n+    'printf \"\\0\\1\\1X\\1\" > tail_garbage_literal &&\n+     test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null'\n+\n+# \\5 - five bytes in base\n+# \\1 - one byte in result\n+# \\1 - one literal byte (X)\n+# \\221 - copy, one byte offset, one byte size\n+#   \\0 - copy from offset 0\n+#   \\1 - copy 1 byte\n+test_expect_success \\\n+    'apply delta with trailing garbage copy' \\\n+    'printf \"\\5\\1\\1X\\221\\0\\1\" > tail_garbage_copy &&\n+     echo base >base &&\n+     test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null'\n+\n+# \\0 - empty base\n+# \\1 - one byte in result\n+# \\1 - one literal byte (X)\n+# \\0 - bogus opcode\n+test_expect_success \\\n+    'apply delta with trailing garbage opcode' \\\n+    'printf \"\\0\\1\\1X\\0\" > tail_garbage_opcode &&\n+     test_must_fail test-tool delta -p /dev/null tail_garbage_opcode /dev/null'\n+\n test_done\n-- \n2.19.0.rc1.539.g3876d0831e\n\n"},{"id":"356942","messageId":"20180830071252.GE15420@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180830070548.GA15081@sigill.intra.peff.net","subject":"[PATCH 5/5] patch-delta: handle truncated copy parameters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T07:12:52Z","receivedAt":"2018-08-30T07:12:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a delta command instructing us to copy bytes\nfrom the base, we have to read the offset and size from the\ndelta stream. We do this without checking whether we're at\nthe end of the stream, meaning we may read past the end of\nthe buffer.\n\nIn practice this isn't exploitable in any interesting way\nbecause:\n\n  1. Deltas are always in packfiles, so we have at least a\n     20-byte trailer that we'll end up reading.\n\n  2. The worst case is that we try to perform a nonsense\n     copy from the base object into the result, based on\n     whatever was in the pack stream next. In most cases\n     this will simply fail due to our bounds-checks against\n     the base or the result.\n\n     But even if you carefully constructed a pack stream for\n     which it succeeds, it wouldn't perform any delta\n     operation that you couldn't have simply included in a\n     non-broken form.\n\nBut obviously it's poor form to read past the end of the\nbuffer we've been given. Unfortunately there's no easy way\nto do a single length check, since the number of bytes we\nneed depends on the number of bits set in the initial\ncommand byte. So we'll just check each byte as we parse. We\ncan hide the complexity in a macro; it's ugly, but not as\nugly as writing out each individual conditional.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n patch-delta.c                         | 21 ++++++++++++++-------\n t/t5303-pack-corruption-resilience.sh |  2 +-\n 2 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/patch-delta.c b/patch-delta.c\nindex 283fb4b759..b5c8594db6 100644\n--- a/patch-delta.c\n+++ b/patch-delta.c\n@@ -40,13 +40,20 @@ void *patch_delta(const void *src_buf, unsigned long src_size,\n \t\tcmd = *data++;\n \t\tif (cmd & 0x80) {\n \t\t\tunsigned long cp_off = 0, cp_size = 0;\n-\t\t\tif (cmd & 0x01) cp_off = *data++;\n-\t\t\tif (cmd & 0x02) cp_off |= (*data++ << 8);\n-\t\t\tif (cmd & 0x04) cp_off |= (*data++ << 16);\n-\t\t\tif (cmd & 0x08) cp_off |= ((unsigned) *data++ << 24);\n-\t\t\tif (cmd & 0x10) cp_size = *data++;\n-\t\t\tif (cmd & 0x20) cp_size |= (*data++ << 8);\n-\t\t\tif (cmd & 0x40) cp_size |= (*data++ << 16);\n+#define PARSE_CP_PARAM(bit, var, shift) do { \\\n+\t\t\tif (cmd & (bit)) { \\\n+\t\t\t\tif (data >= top) \\\n+\t\t\t\t\tgoto bad_length; \\\n+\t\t\t\tvar |= ((unsigned) *data++ << (shift)); \\\n+\t\t\t} } while (0)\n+\t\t\tPARSE_CP_PARAM(0x01, cp_off, 0);\n+\t\t\tPARSE_CP_PARAM(0x02, cp_off, 8);\n+\t\t\tPARSE_CP_PARAM(0x04, cp_off, 16);\n+\t\t\tPARSE_CP_PARAM(0x08, cp_off, 24);\n+\t\t\tPARSE_CP_PARAM(0x10, cp_size, 0);\n+\t\t\tPARSE_CP_PARAM(0x20, cp_size, 8);\n+\t\t\tPARSE_CP_PARAM(0x40, cp_size, 16);\n+#undef PARSE_CP_PARAM\n \t\t\tif (cp_size == 0) cp_size = 0x10000;\n \t\t\tif (unsigned_add_overflows(cp_off, cp_size) ||\n \t\t\t    cp_off + cp_size > src_size ||\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 41dc947d3f..b68bbeedcc 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -364,7 +364,7 @@ test_expect_success \\\n #\n # Note that the literal byte is necessary to get past the uninteresting minimum\n # delta size check.\n-test_expect_failure \\\n+test_expect_success \\\n     'apply delta with truncated copy parameters' \\\n     'printf \"\\5\\5\\1X\\221\" > truncated_copy_delta &&\n      echo base >base &&\n-- \n2.19.0.rc1.539.g3876d0831e\n"},{"id":"356972","messageId":"CAG48ez1uK9AhmodAVH_hWuzLfVQh1=mp2=WQcUaNoEQ8nMHtpw@mail.gmail.com","threadId":"49254","inReplyTo":"20180830070548.GA15081@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] handle corruption in patch-delta","fromName":"Jann Horn","fromEmail":"jannh@google.com","sentAt":"2018-08-30T13:25:10Z","receivedAt":"2018-08-30T13:25:39Z","isPatch":true,"sender":{"key":"jannh@google.com","avatar":"https://avatars.githubusercontent.com/u/499370?v=4"},"body":"On Thu, Aug 30, 2018 at 9:05 AM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:\n>\n> > If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the\n> > `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`\n> > into `dst_buf`.\n> >\n> > This is not an exploitable bug because triggering the bug increments the\n> > `data` pointer beyond `top`, causing the `data != top` sanity check after\n> > the loop to trigger and discard the destination buffer - which means that\n> > the result of the out-of-bounds read is never used for anything.\n> >\n> > Also, directly jump into the error handler instead of just breaking out of\n> > the loop - otherwise, data corruption would be silently ignored if the\n> > delta buffer ends with a command and the destination buffer is already\n> > full.\n>\n> Based on my earlier observations, here's a replacement patch series I\n> came up with. It has:\n[...]\n> I hope you don't mind me hacking up your patches a bit.\n\nNot at all. I'm happy that I don't have to write a v2 series.\n\n> Thanks again for your original report and patch.\n\nThanks for turning my patch into something decent so quickly!\n"},{"id":"356983","messageId":"nycvar.YSQ.7.76.1808301104520.1454@knanqh.ubzr","threadId":"49254","inReplyTo":"20180830070548.GA15081@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] handle corruption in patch-delta","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2018-08-30T15:23:42Z","receivedAt":"2018-08-30T15:23:50Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 30 Aug 2018, Jeff King wrote:\n\n> On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:\n> \n> > If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the\n> > `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`\n> > into `dst_buf`.\n> > \n> > This is not an exploitable bug because triggering the bug increments the\n> > `data` pointer beyond `top`, causing the `data != top` sanity check after\n> > the loop to trigger and discard the destination buffer - which means that\n> > the result of the out-of-bounds read is never used for anything.\n> > \n> > Also, directly jump into the error handler instead of just breaking out of\n> > the loop - otherwise, data corruption would be silently ignored if the\n> > delta buffer ends with a command and the destination buffer is already\n> > full.\n> \n> Based on my earlier observations, here's a replacement patch series I\n> came up with. It has:\n> \n>   [1/5]: test-delta: read input into a heap buffer\n> \n>     A simpler replacement for your patch 2 which avoids portability\n>     issues.\n> \n>   [2/5]: t5303: test some corrupt deltas\n> \n>     A more complete set of boundary tests based on the 4 cases I laid\n>     out, plus the cp_size problem I found.\n> \n>   [3/5]: patch-delta: fix oob read\n> \n>     Your actual fix.\n> \n>   [4/5]: patch-delta: consistently report corruption\n> \n>     Your related trailing-garbage fix. I split this into two in order to\n>     better demonstrate the cases this part covers.\n> \n>   [5/5]: patch-delta: handle truncated copy parameters\n> \n>     My fix for the cp_size read.\n> \n> I hope you don't mind me hacking up your patches a bit. Thanks again for\n> your original report and patch.\n\nLooks good to me (feels like traveling back in time).\n\nReviewed-by: Nicolas Pitre <nico@fluxnic.net>\n\n\n\n> \n> -Peff\n> \n"},{"id":"356994","messageId":"xmqq8t4n69gy.fsf@gitster-ct.c.googlers.com","threadId":"49254","inReplyTo":"20180830070932.GB15420@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-30T17:38:21Z","receivedAt":"2018-08-30T17:38:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +test_expect_success \\\n> +    'apply delta with too many copied bytes' \\\n> +    'printf \"\\5\\1\\221\\0\\2\" > too_big_copy &&\n> +     echo base >base &&\n> +     test_must_fail test-tool delta -p base too_big_copy /dev/null'\n\nWould \"echo base >base\" give us 5-byte long base even on Windows?\nOr the test does not care if it is either \"base\\n\" or \"base\\r\\n\"?\n\nJust double-checking.\n\n\n"},{"id":"356996","messageId":"20180830184201.GB14273@sigill.intra.peff.net","threadId":"49254","inReplyTo":"xmqq8t4n69gy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T18:42:01Z","receivedAt":"2018-08-30T18:42:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 30, 2018 at 10:38:21AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +test_expect_success \\\n> > +    'apply delta with too many copied bytes' \\\n> > +    'printf \"\\5\\1\\221\\0\\2\" > too_big_copy &&\n> > +     echo base >base &&\n> > +     test_must_fail test-tool delta -p base too_big_copy /dev/null'\n> \n> Would \"echo base >base\" give us 5-byte long base even on Windows?\n> Or the test does not care if it is either \"base\\n\" or \"base\\r\\n\"?\n> \n> Just double-checking.\n\nGood question. On the first one, I don't know. On the second one, yes,\nit does matter. We'd feed \"6\" to patch_delta(), and it would complain\nabout the mismatch before actually hitting the code we're trying to\nexercise. The test would still pass (the error result is the same either\nway), but would quietly not test what we wanted.\n\nMaybe something like this to be on the safe side?\n\n(note that we can leave the \\5 in the result size of the \"truncated copy\nparameters\" test; it really just needs to be larger than 1).\n\n---\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 912e659acf..e80934a18e 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -327,15 +327,15 @@ test_expect_success \\\n     'printf \"\\0\\1\\2XX\" > too_big_literal &&\n      test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null'\n \n-# \\5 - five bytes in base\n+# \\4 - four bytes in base\n # \\1 - one byte in result\n # \\221 - copy, one byte offset, one byte size\n #   \\0 - copy from offset 0\n #   \\2 - copy two bytes (one too many)\n test_expect_success \\\n     'apply delta with too many copied bytes' \\\n-    'printf \"\\5\\1\\221\\0\\2\" > too_big_copy &&\n-     echo base >base &&\n+    'printf \"\\4\\1\\221\\0\\2\" > too_big_copy &&\n+     printf base >base &&\n      test_must_fail test-tool delta -p base too_big_copy /dev/null'\n \n # \\0 - empty base\n@@ -356,7 +356,7 @@ test_expect_success \\\n     'printf \"\\0\\1\\221\\0\\1\" > truncated_base &&\n      test_must_fail test-tool delta -p /dev/null truncated_base /dev/null'\n \n-# \\5 - five bytes in base\n+# \\4 - four bytes in base\n # \\5 - five bytes in result\n # \\1 - one literal byte (X)\n # \\221 - copy, one byte offset, one byte size\n@@ -366,8 +366,8 @@ test_expect_success \\\n # delta size check.\n test_expect_failure \\\n     'apply delta with truncated copy parameters' \\\n-    'printf \"\\5\\5\\1X\\221\" > truncated_copy_delta &&\n-     echo base >base &&\n+    'printf \"\\4\\5\\1X\\221\" > truncated_copy_delta &&\n+     printf base >base &&\n      test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'\n \n test_done\n"},{"id":"356997","messageId":"20180830184459.GC14273@sigill.intra.peff.net","threadId":"49254","inReplyTo":"20180830184201.GB14273@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T18:44:59Z","receivedAt":"2018-08-30T18:45:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 30, 2018 at 02:42:01PM -0400, Jeff King wrote:\n\n> > Would \"echo base >base\" give us 5-byte long base even on Windows?\n> > Or the test does not care if it is either \"base\\n\" or \"base\\r\\n\"?\n> > \n> > Just double-checking.\n> \n> Good question. On the first one, I don't know. On the second one, yes,\n> it does matter. We'd feed \"6\" to patch_delta(), and it would complain\n> about the mismatch before actually hitting the code we're trying to\n> exercise. The test would still pass (the error result is the same either\n> way), but would quietly not test what we wanted.\n> \n> Maybe something like this to be on the safe side?\n\nThat could be squashed into patch 2. Patch 4 would need this one\nadditional case:\n\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 0c537958e7..e91d6f5770 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -379,7 +379,7 @@ test_expect_success \\\n     'printf \"\\0\\1\\1X\\1\" > tail_garbage_literal &&\n      test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null'\n \n-# \\5 - five bytes in base\n+# \\4 - four bytes in base\n # \\1 - one byte in result\n # \\1 - one literal byte (X)\n # \\221 - copy, one byte offset, one byte size\n@@ -387,8 +387,8 @@ test_expect_success \\\n #   \\1 - copy 1 byte\n test_expect_success \\\n     'apply delta with trailing garbage copy' \\\n-    'printf \"\\5\\1\\1X\\221\\0\\1\" > tail_garbage_copy &&\n-     echo base >base &&\n+    'printf \"\\4\\1\\1X\\221\\0\\1\" > tail_garbage_copy &&\n+     printf base >base &&\n      test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null'\n \n # \\0 - empty base\n\nI can re-roll, or even prepare a patch on top (it's sufficiently subtle\nthat it may merit calling out explicitly in a commit).\n\n-Peff\n"},{"id":"356999","messageId":"xmqqzhx34rjj.fsf@gitster-ct.c.googlers.com","threadId":"49254","inReplyTo":"20180830184459.GC14273@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-30T18:50:56Z","receivedAt":"2018-08-30T18:51:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I can re-roll, or even prepare a patch on top (it's sufficiently subtle\n> that it may merit calling out explicitly in a commit).\n\nYeah, I tend to agree with your reasoning to do it on top as a\nseparate patch.\n"},{"id":"357002","messageId":"20180830191339.GA19238@sigill.intra.peff.net","threadId":"49254","inReplyTo":"xmqqzhx34rjj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T19:13:39Z","receivedAt":"2018-08-30T19:13:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 30, 2018 at 11:50:56AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I can re-roll, or even prepare a patch on top (it's sufficiently subtle\n> > that it may merit calling out explicitly in a commit).\n> \n> Yeah, I tend to agree with your reasoning to do it on top as a\n> separate patch.\n\nHere it is, then.\n\n-- >8 --\nSubject: [PATCH 6/5] t5303: use printf to generate delta bases\n\nThe exact byte count of the delta base file is important.\nThe test-delta helper will feed it to patch_delta(), which\nwill barf if it doesn't match the size byte given in the\ndelta. Using \"echo\" may end up with unexpected line endings\non some platforms (e.g,. \"\\r\\n\" instead of just \"\\n\").\n\nThis actually wouldn't cause the test to fail (since we\nalready expect test-delta to complain about these bogus\ndeltas), but would mean that we're not exercising the code\nwe think we are.\n\nLet's use printf instead (which we already trust to give us\nbyte-perfect output when we generate the deltas).\n\nWhile we're here, let's tighten the 5-byte result size used\nin the \"truncated copy parameters\" test. This just needs to\nhave enough room to attempt to parse the bogus copy command,\nmeaning 2 is sufficient. Using 5 was arbitrary and just\ncopied from the base size; since those no longer match, it's\nsimply confusing. Let's use a more meaningful number.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5303-pack-corruption-resilience.sh | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex b68bbeedcc..41e6dc4dcf 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -327,15 +327,15 @@ test_expect_success \\\n     'printf \"\\0\\1\\2XX\" > too_big_literal &&\n      test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null'\n \n-# \\5 - five bytes in base\n+# \\4 - four bytes in base\n # \\1 - one byte in result\n # \\221 - copy, one byte offset, one byte size\n #   \\0 - copy from offset 0\n #   \\2 - copy two bytes (one too many)\n test_expect_success \\\n     'apply delta with too many copied bytes' \\\n-    'printf \"\\5\\1\\221\\0\\2\" > too_big_copy &&\n-     echo base >base &&\n+    'printf \"\\4\\1\\221\\0\\2\" > too_big_copy &&\n+     printf base >base &&\n      test_must_fail test-tool delta -p base too_big_copy /dev/null'\n \n # \\0 - empty base\n@@ -356,8 +356,8 @@ test_expect_success \\\n     'printf \"\\0\\1\\221\\0\\1\" > truncated_base &&\n      test_must_fail test-tool delta -p /dev/null truncated_base /dev/null'\n \n-# \\5 - five bytes in base\n-# \\5 - five bytes in result\n+# \\4 - four bytes in base\n+# \\2 - two bytes in result\n # \\1 - one literal byte (X)\n # \\221 - copy, one byte offset, one byte size\n #        (offset/size missing)\n@@ -366,8 +366,8 @@ test_expect_success \\\n # delta size check.\n test_expect_success \\\n     'apply delta with truncated copy parameters' \\\n-    'printf \"\\5\\5\\1X\\221\" > truncated_copy_delta &&\n-     echo base >base &&\n+    'printf \"\\4\\2\\1X\\221\" > truncated_copy_delta &&\n+     printf base >base &&\n      test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'\n \n # \\0 - empty base\n@@ -379,7 +379,7 @@ test_expect_success \\\n     'printf \"\\0\\1\\1X\\1\" > tail_garbage_literal &&\n      test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null'\n \n-# \\5 - five bytes in base\n+# \\4 - four bytes in base\n # \\1 - one byte in result\n # \\1 - one literal byte (X)\n # \\221 - copy, one byte offset, one byte size\n@@ -387,8 +387,8 @@ test_expect_success \\\n #   \\1 - copy 1 byte\n test_expect_success \\\n     'apply delta with trailing garbage copy' \\\n-    'printf \"\\5\\1\\1X\\221\\0\\1\" > tail_garbage_copy &&\n-     echo base >base &&\n+    'printf \"\\4\\1\\1X\\221\\0\\1\" > tail_garbage_copy &&\n+     printf base >base &&\n      test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null'\n \n # \\0 - empty base\n-- \n2.19.0.rc1.546.g3fcb3c0d7c\n\n"},{"id":"357078","messageId":"nycvar.QRO.7.76.6.1808311155410.71@tvgsbejvaqbjf.bet","threadId":"49254","inReplyTo":"xmqq8t4n69gy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-31T09:58:24Z","receivedAt":"2018-08-31T09:58:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 30 Aug 2018, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +test_expect_success \\\n> > +    'apply delta with too many copied bytes' \\\n> > +    'printf \"\\5\\1\\221\\0\\2\" > too_big_copy &&\n> > +     echo base >base &&\n> > +     test_must_fail test-tool delta -p base too_big_copy /dev/null'\n> \n> Would \"echo base >base\" give us 5-byte long base even on Windows?\n\nPlease note that Unix shell scripting is a foreign thing on Windows. As\nsuch, there is not really any \"native\" shell we can use [*1*], and\ntherefore we use MSYS2's Bash which outputs Unix line endings.\n\nCiao,\nDscho\n\nFootnote *1*: I keep trying (and failing) to find the time to work on the\npure-Win32 port of BusyBox, which would give us \"sort of a native Unix\nshell\". That probably *would* output CR/LF in this case. But as there are\nstill parts of the test suite that require Perl (which is not included in\nBusyBox), I think we are still a loooong way from running the test suite\nin a pure Win32 fashion. With all the time tax that incurs for us Windows\nusers.\n"},{"id":"357088","messageId":"xmqqh8ja360p.fsf@gitster-ct.c.googlers.com","threadId":"49254","inReplyTo":"nycvar.QRO.7.76.6.1808311155410.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-31T15:33:26Z","receivedAt":"2018-08-31T15:33:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> Would \"echo base >base\" give us 5-byte long base even on Windows?\n>\n> Please note that Unix shell scripting is a foreign thing on Windows. As\n> such, there is not really any \"native\" shell we can use [*1*], and\n\nYeah, I know that; otherwise I wouldn't have asked.  Because ...\n\n> therefore we use MSYS2's Bash which outputs Unix line endings.\n\n... I didn't know what MSYS folks chose, and/or if you have chosen\nto tweak their choice, and/or if you switched to somebody else's shell\n(e.g. busybox) and/or you chose to tweak what they do out of the box,\nit was worth asking and getting yes/no question.  You do not have to\ntell me why I should be asking.\n\nSo instead of typing 3 lines, you can just say \"yes we use echo that\nemulates Unix\".\n\nThanks.\n"},{"id":"357113","messageId":"20180831194727.GA5120@sigill.intra.peff.net","threadId":"49254","inReplyTo":"xmqqh8ja360p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-31T19:47:27Z","receivedAt":"2018-08-31T19:47:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 08:33:26AM -0700, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> Would \"echo base >base\" give us 5-byte long base even on Windows?\n> >\n> > Please note that Unix shell scripting is a foreign thing on Windows. As\n> > such, there is not really any \"native\" shell we can use [*1*], and\n> \n> Yeah, I know that; otherwise I wouldn't have asked.  Because ...\n> \n> > therefore we use MSYS2's Bash which outputs Unix line endings.\n> \n> ... I didn't know what MSYS folks chose, and/or if you have chosen\n> to tweak their choice, and/or if you switched to somebody else's shell\n> (e.g. busybox) and/or you chose to tweak what they do out of the box,\n> it was worth asking and getting yes/no question.  You do not have to\n> tell me why I should be asking.\n> \n> So instead of typing 3 lines, you can just say \"yes we use echo that\n> emulates Unix\".\n\nI actually found Dscho's response much more informative than a simple\nyes/no.\n\nAt any rate, it sounds like we are probably OK with echo, but I think it\nis still worth doing the defensive patch-on-top that I posted.\n\n-Peff\n"},{"id":"357133","messageId":"nycvar.QRO.7.76.6.1808312313210.71@tvgsbejvaqbjf.bet","threadId":"49254","inReplyTo":"xmqqh8ja360p.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-31T21:14:20Z","receivedAt":"2018-08-31T21:14:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 31 Aug 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> Would \"echo base >base\" give us 5-byte long base even on Windows?\n> >\n> > Please note that Unix shell scripting is a foreign thing on Windows. As\n> > such, there is not really any \"native\" shell we can use [*1*], and\n> \n> Yeah, I know that; otherwise I wouldn't have asked.  Because ...\n> \n> > therefore we use MSYS2's Bash which outputs Unix line endings.\n> \n> ... I didn't know what MSYS folks chose, and/or if you have chosen\n> to tweak their choice, and/or if you switched to somebody else's shell\n> (e.g. busybox) and/or you chose to tweak what they do out of the box,\n> it was worth asking and getting yes/no question.  You do not have to\n> tell me why I should be asking.\n> \n> So instead of typing 3 lines, you can just say \"yes we use echo that\n> emulates Unix\".\n> \n> Thanks.\n\nYou could just express your gratitude that I do more than just answer a\nquestion, and impart more knowledge that will help you be a better\nmaintainer.\n\nCiao,\nDscho\n"},{"id":"357137","messageId":"xmqqbm9i1aih.fsf@gitster-ct.c.googlers.com","threadId":"49254","inReplyTo":"20180831194727.GA5120@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-31T21:39:18Z","receivedAt":"2018-08-31T21:39:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> So instead of typing 3 lines, you can just say \"yes we use echo that\n>> emulates Unix\".\n>\n> I actually found Dscho's response much more informative than a simple\n> yes/no.\n>\n> At any rate, it sounds like we are probably OK with echo, but I think it\n> is still worth doing the defensive patch-on-top that I posted.\n\nActually, Dscho's footnote was much more informative than the \"echo\nis from UNIX, dammit\" that didn't convey anything useful---yes, we\nall know that echo is from UNIX, but what does he do to it?  Does he\nwant it to be like UNIX, or want to tweak it to suit Windows taste?\n\nReading the footnote again, from there, you can read that 'echo' may\ncurrently behave like UNIX, but he guarantees that it can change any\ntime without notice; in his mind, 'echo' in the ideal world in the\nfiture on Windows is to use CRLF.\n\nSo we must protect the test from his whim by using printf.\n\n\n\n"},{"id":"357138","messageId":"20180831214116.GA16999@sigill.intra.peff.net","threadId":"49254","inReplyTo":"nycvar.QRO.7.76.6.1808312313210.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-31T21:41:16Z","receivedAt":"2018-08-31T21:41:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 11:14:20PM +0200, Johannes Schindelin wrote:\n\n> On Fri, 31 Aug 2018, Junio C Hamano wrote:\n> [...]\n> > So instead of typing 3 lines, you can just say \"yes we use echo that\n> > emulates Unix\".\n> \n> You could just express your gratitude that I do more than just answer a\n> question, and impart more knowledge that will help you be a better\n> maintainer.\n\nIt is my observation as a third party that there has been a lot of\nbickering between you two lately (here and in other threads). Can you\nboth please try to make sure your comments are truly constructive?\n\nI don't want to get into details of who I think has been unreasonable\nwhen, or lay any kind of blame. I'm just concerned that exchanges like\nthis one worsen the general tone of the mailing list, and I'd like to:\n\n  - make it clear to other observers that there's at least one person\n    who hopes this is not the norm for this list\n\n  - make a general reminder that collaborating on the Internet is hard,\n    but it's worth spending the extra effort to consider how comments\n    will be taken by the intended receiver, as well as others on the\n    list\n\nYou're free to disagree, of course. And I'm sorry for being somewhat\nvague. I really want to avoid opening up a specific flamewar, and just\nmake a general call for people to remember to be friendly.\n\nIf vger allowed HTML mail, I would link to a picture of a unicorn\nrunning across a rainbow.\n\n-Peff\n"},{"id":"357139","messageId":"xmqq7ek619qu.fsf@gitster-ct.c.googlers.com","threadId":"49254","inReplyTo":"20180831214116.GA16999@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] t5303: test some corrupt deltas","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-31T21:55:53Z","receivedAt":"2018-08-31T21:55:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   - make it clear to other observers that there's at least one person\n>     who hopes this is not the norm for this list\n\nMake it at least two by counting me ;-)\n\n>   - make a general reminder that collaborating on the Internet is hard,\n>     but it's worth spending the extra effort to consider how comments\n>     will be taken by the intended receiver, as well as others on the\n>     list\n\nWell, in this particular case both of us know exactly how the sender\nwants the message to be taken by the other side.  At least I do.\n"}]}