{"thread":{"id":"41518","subject":"[PATCH 0/3] out-of-bounds access from corrupted .idx files","startedAt":"2016-02-25T14:20:05Z","lastAt":"2016-02-27T18:25:42Z","messageCount":10,"participants":["Jeff King","Johannes Sixt","Junio C Hamano","Nguyễn Thái Ngọc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"279338","messageId":"20160225142004.GA17678@sigill.intra.peff.net","threadId":"41518","inReplyTo":null,"subject":"[PATCH 0/3] out-of-bounds access from corrupted .idx files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-25T14:20:05Z","receivedAt":"2016-02-25T14:20:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series teaches Git to detect a few problems with corrupted .idx\nfiles, and adds tests for some more cases.  There's conceptually some\noverlap with t5300, but I don't think it was covering any of these cases\nexplicitly.\n\nThere are two real bugs that could cause segfaults or bus errors via\nbogus reads (but never writes). On top of that, these are all problems\nin .idx files, which are usually generated locally. So I don't think\nthere's anything particularly security interesting here. You'd need a\nsituation where you convince somebody to read your .idx files (so maybe\na multi-user server), and then I don't see how you'd turn it into remote\ncode execution.\n\nI think with these patches, fuzzing .idx files should never result in\nany memory problems (though of course git will die()).  Famous last\nwords, of course. I stopped short of poking at other file formats, which\nmight have similar issues. Obvious candidates are .bitmap files, and the\non-disk $GIT_DIR/index.\n\n  [1/3]: t5313: test bounds-checks of corrupted/malicious pack/idx files\n  [2/3]: nth_packed_object_offset: bounds-check extended offset\n  [3/3]: use_pack: handle signed off_t overflow\n\n-Peff\n"},{"id":"279339","messageId":"20160225142112.GA17811@sigill.intra.peff.net","threadId":"41518","inReplyTo":"20160225142004.GA17678@sigill.intra.peff.net","subject":"[PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-25T14:21:12Z","receivedAt":"2016-02-25T14:21:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our on-disk .pack and .idx files may reference other data by\noffset. We should make sure that we are not fooled by\ncorrupt data into accessing memory outside of our mmap'd\nboundaries.\n\nThis patch adds a series of tests for offsets found in .pack\nand .idx files. For the most part we get this right, but\nthere are two tests of .idx files marked as failures: we do\nnot bounds-check offsets in the v2 index's extended offset\ntable, nor do we handle .idx offsets that overflow a signed\noff_t.\n\nWith these tests, we should have good coverage of all\noffsets found in these files. Note that this doesn't cover\n.bitmap files, which may have similar bugs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5313-pack-bounds-checks.sh | 179 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 179 insertions(+)\n create mode 100755 t/t5313-pack-bounds-checks.sh\n\ndiff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\nnew file mode 100755\nindex 0000000..efc5197\n--- /dev/null\n+++ b/t/t5313-pack-bounds-checks.sh\n@@ -0,0 +1,179 @@\n+#!/bin/sh\n+\n+test_description='bounds-checking of access to mmapped on-disk file formats'\n+. ./test-lib.sh\n+\n+clear_base () {\n+\ttest_when_finished 'restore_base' &&\n+\trm -f $base\n+}\n+\n+restore_base () {\n+\tcp base-backup/* .git/objects/pack/\n+}\n+\n+do_pack () {\n+\tpack_objects=$1; shift\n+\tsha1=$(\n+\t\tfor i in $pack_objects\n+\t\tdo\n+\t\t\techo $i\n+\t\tdone | git pack-objects \"$@\" .git/objects/pack/pack\n+\t) &&\n+\tpack=.git/objects/pack/pack-$sha1.pack &&\n+\tidx=.git/objects/pack/pack-$sha1.idx &&\n+\tchmod +w $pack $idx &&\n+\ttest_when_finished 'rm -f \"$pack\" \"$idx\"'\n+}\n+\n+munge () {\n+\tprintf \"$3\" | dd of=\"$1\" bs=1 conv=notrunc seek=$2\n+}\n+\n+# Offset in a v2 .idx to its initial and extended offset tables. For an index\n+# with \"nr\" objects, this is:\n+#\n+#   magic(4) + version(4) + fan-out(4*256) + sha1s(20*nr) + crc(4*nr),\n+#\n+# for the initial, and another ofs(4*nr) past that for the extended.\n+#\n+ofs_table () {\n+\techo $((4 + 4 + 4*256 + 20*$1 + 4*$1))\n+}\n+extended_table () {\n+\techo $(($(ofs_table \"$1\") + 4*$1))\n+}\n+\n+test_expect_success 'set up base packfile and variables' '\n+\t# the hash of this content starts with ff, which\n+\t# makes some later computations much simpler\n+\techo 74 >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\tgit repack -ad &&\n+\tbase=$(echo .git/objects/pack/*) &&\n+\tchmod +w $base &&\n+\tmkdir base-backup &&\n+\tcp $base base-backup/ &&\n+\tobject=$(git rev-parse HEAD:file)\n+'\n+\n+test_expect_success 'pack/index object count mismatch' '\n+\tdo_pack $object &&\n+\tmunge $pack 8 \"\\377\\0\\0\\0\" &&\n+\tclear_base &&\n+\n+\t# We enumerate the objects from the completely-fine\n+\t# .idx, but notice later that the .pack is bogus\n+\t# and fail to show any data.\n+\techo \"$object missing\" >expect &&\n+\tgit cat-file --batch-all-objects --batch-check >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# ...and here fail to load the object (without segfaulting),\n+\t# but fallback to a good copy if available.\n+\ttest_must_fail git cat-file blob $object &&\n+\trestore_base &&\n+\tgit cat-file blob $object >actual &&\n+\ttest_cmp file actual &&\n+\n+\t# ...and make sure that index-pack --verify, which has its\n+\t# own reading routines, does not segfault.\n+\ttest_must_fail git index-pack --verify $pack\n+'\n+\n+test_expect_success 'matched bogus object count' '\n+\tdo_pack $object &&\n+\tmunge $pack 8 \"\\377\\0\\0\\0\" &&\n+\tmunge $idx $((255 * 4)) \"\\377\\0\\0\\0\" &&\n+\tclear_base &&\n+\n+\t# Unlike above, we should notice early that the .idx is totally\n+\t# bogus, and not even enumerate its contents.\n+\t>expect &&\n+\tgit cat-file --batch-all-objects --batch-check >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# But as before, we can do the same object-access checks.\n+\ttest_must_fail git cat-file blob $object &&\n+\trestore_base &&\n+\tgit cat-file blob $object >actual &&\n+\ttest_cmp file actual &&\n+\n+\ttest_must_fail git index-pack --verify $pack\n+'\n+\n+# Note that we cannot check the fallback case for these\n+# further .idx tests, as we notice the problem in functions\n+# whose interface doesn't allow an error return (like use_pack()),\n+# and thus we just die().\n+#\n+# There's also no point in doing enumeration tests, as\n+# we are munging offsets here, which are about looking up\n+# specific objects.\n+\n+test_expect_success 'bogus object offset (v1)' '\n+\tdo_pack $object --index-version=1 &&\n+\tmunge $idx $((4 * 256)) \"\\377\\0\\0\\0\" &&\n+\tclear_base &&\n+\ttest_must_fail git cat-file blob $object &&\n+\ttest_must_fail git index-pack --verify $pack\n+'\n+\n+test_expect_success 'bogus object offset (v2, no msb)' '\n+\tdo_pack $object --index-version=2 &&\n+\tmunge $idx $(ofs_table 1) \"\\0\\377\\0\\0\" &&\n+\tclear_base &&\n+\ttest_must_fail git cat-file blob $object &&\n+\ttest_must_fail git index-pack --verify $pack\n+'\n+\n+test_expect_failure 'bogus offset into v2 extended table' '\n+\tdo_pack $object --index-version=2 &&\n+\tmunge $idx $(ofs_table 1) \"\\377\\0\\0\\0\" &&\n+\tclear_base &&\n+\ttest_must_fail git cat-file blob $object &&\n+\ttest_must_fail git index-pack --verify $pack\n+'\n+\n+test_expect_failure 'bogus offset inside v2 extended table' '\n+\t# We need two objects here, so we can plausibly require\n+\t# an extended table (if the first object were larger than 2^31).\n+\tdo_pack \"$object $(git rev-parse HEAD)\" --index-version=2 &&\n+\n+\t# We have to make extra room for the table, so we cannot\n+\t# just munge in place as usual.\n+\t{\n+\t\tdd if=$idx bs=1 count=$(($(ofs_table 2) + 4)) &&\n+\t\tprintf \"\\200\\0\\0\\0\" &&\n+\t\tprintf \"\\377\\0\\0\\0\\0\\0\\0\\0\" &&\n+\t\tdd if=$idx bs=1 skip=$(extended_table 2)\n+\t} >tmp &&\n+\tmv tmp \"$idx\" &&\n+\tclear_base &&\n+\ttest_must_fail git cat-file blob $object &&\n+\ttest_must_fail git index-pack --verify $pack\n+'\n+\n+test_expect_success 'bogus OFS_DELTA in packfile' '\n+\t# Generate a pack with a delta in it.\n+\tbase=$(test-genrandom foo 3000 | git hash-object --stdin -w) &&\n+\tdelta=$(test-genrandom foo 2000 | git hash-object --stdin -w) &&\n+\tdo_pack \"$base $delta\" --delta-base-offset &&\n+\trm -f .git/objects/??/* &&\n+\n+\t# Double check that we have the delta we expect.\n+\techo $base >expect &&\n+\techo $delta | git cat-file --batch-check=\"%(deltabase)\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Now corrupt it. We assume the varint size for the delta is small\n+\t# enough to fit in the first byte (which it should be, since it\n+\t# is a pure deletion from the base), and that original ofs_delta\n+\t# takes 2 bytes (which it should, as it should be ~3000).\n+\tofs=$(git show-index <$idx | grep $delta | cut -d\" \" -f1) &&\n+\tmunge $pack $(($ofs + 1)) \"\\177\\377\" &&\n+\ttest_must_fail git cat-file blob $delta >/dev/null\n+'\n+\n+test_done\n-- \n2.7.2.695.gf3fde8e\n"},{"id":"279340","messageId":"20160225142252.GB17811@sigill.intra.peff.net","threadId":"41518","inReplyTo":"20160225142004.GA17678@sigill.intra.peff.net","subject":"[PATCH 2/3] nth_packed_object_offset: bounds-check extended offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-25T14:22:52Z","receivedAt":"2016-02-25T14:22:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a pack .idx file has a corrupted offset for an object, we\nmay try to access an offset in the .idx or .pack file that\nis larger than the file's size.  For the .pack case, we have\nuse_pack() to protect us, which realizes the access is out\nof bounds. But if the corrupted value asks us to look in the\n.idx file's secondary 64-bit offset table, we blindly add it\nto the mmap'd index data and access arbitrary memory.\n\nWe can fix this with a simple bounds-check compared to the\nsize we found when we opened the .idx file.\n\nNote that there's similar code in index-pack that is\ntriggered only during \"index-pack --verify\". To support\nboth, we pull the bounds-check into a separate function,\nwhich dies when it sees a corrupted file.\n\nIt would be nice if we could return an error, so that the\npack code could try to find a good copy of the object\nelsewhere. Currently nth_packed_object_offset doesn't have\nany way to return an error, but it could probably use \"0\" as\na sentinel value (since no object can start there). This is\nthe minimal fix, and we can improve the resilience later on\ntop.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c          |  1 +\n cache.h                       | 10 ++++++++++\n sha1_file.c                   | 15 +++++++++++++++\n t/t5313-pack-bounds-checks.sh |  2 +-\n 4 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 6a01509..ad6a1d7 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1514,6 +1514,7 @@ static void read_v2_anomalous_offsets(struct packed_git *p,\n \t\tif (!(off & 0x80000000))\n \t\t\tcontinue;\n \t\toff = off & 0x7fffffff;\n+\t\tcheck_pack_index_ptr(p, &idx2[off * 2]);\n \t\tif (idx2[off * 2])\n \t\t\tcontinue;\n \t\t/*\ndiff --git a/cache.h b/cache.h\nindex 83b688c..25a7040 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1370,6 +1370,16 @@ extern void clear_delta_base_cache(void);\n extern struct packed_git *add_packed_git(const char *path, size_t path_len, int local);\n \n /*\n+ * Make sure that a pointer access into an mmap'd index file is within bounds,\n+ * and can provide at least 8 bytes of data.\n+ *\n+ * Note that this is only necessary for variable-length segments of the file\n+ * (like the 64-bit extended offset table), as we compare the size to the\n+ * fixed-length parts when we open the file.\n+ */\n+extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n+\n+/*\n  * Return the SHA-1 of the nth object within the specified packfile.\n  * Open the index if it is not already open.  The return value points\n  * at the SHA-1 within the mmapped index.  Return NULL if there is an\ndiff --git a/sha1_file.c b/sha1_file.c\nindex aab1872..d2ffd92 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2446,6 +2446,20 @@ const unsigned char *nth_packed_object_sha1(struct packed_git *p,\n \t}\n }\n \n+void check_pack_index_ptr(const struct packed_git *p, const void *vptr)\n+{\n+\tconst unsigned char *ptr = vptr;\n+\tconst unsigned char *start = p->index_data;\n+\tconst unsigned char *end = start + p->index_size;\n+\tif (ptr < start)\n+\t\tdie(\"offset before start of pack index for %s (corrupt index?)\",\n+\t\t    p->pack_name);\n+\t/* No need to check for underflow; .idx files must be at least 8 bytes */\n+\tif (ptr >= end - 8)\n+\t\tdie(\"offset beyond end of pack index for %s (truncated index?)\",\n+\t\t    p->pack_name);\n+}\n+\n off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n {\n \tconst unsigned char *index = p->index_data;\n@@ -2459,6 +2473,7 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n \t\tif (!(off & 0x80000000))\n \t\t\treturn off;\n \t\tindex += p->num_objects * 4 + (off & 0x7fffffff) * 8;\n+\t\tcheck_pack_index_ptr(p, index);\n \t\treturn (((uint64_t)ntohl(*((uint32_t *)(index + 0)))) << 32) |\n \t\t\t\t   ntohl(*((uint32_t *)(index + 4)));\n \t}\ndiff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\nindex efc5197..0717746 100755\n--- a/t/t5313-pack-bounds-checks.sh\n+++ b/t/t5313-pack-bounds-checks.sh\n@@ -128,7 +128,7 @@ test_expect_success 'bogus object offset (v2, no msb)' '\n \ttest_must_fail git index-pack --verify $pack\n '\n \n-test_expect_failure 'bogus offset into v2 extended table' '\n+test_expect_success 'bogus offset into v2 extended table' '\n \tdo_pack $object --index-version=2 &&\n \tmunge $idx $(ofs_table 1) \"\\377\\0\\0\\0\" &&\n \tclear_base &&\n-- \n2.7.2.695.gf3fde8e\n"},{"id":"279341","messageId":"20160225142326.GC17811@sigill.intra.peff.net","threadId":"41518","inReplyTo":"20160225142004.GA17678@sigill.intra.peff.net","subject":"[PATCH 3/3] use_pack: handle signed off_t overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-25T14:23:26Z","receivedAt":"2016-02-25T14:23:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A v2 pack index file can specify an offset within a packfile\nof up to 2^64-1 bytes. On a system with a signed 64-bit\noff_t, we can represent only up to 2^63-1. This means that a\ncorrupted .idx file can end up with a negative offset in the\npack code. Our bounds-checking use_pack function looks for\ntoo-large offsets, but not for ones that have wrapped around\nto negative. Let's do so, which fixes an out-of-bounds\naccess demonstrated in t5313.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c                   | 2 ++\n t/t5313-pack-bounds-checks.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d2ffd92..9d223e6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1076,6 +1076,8 @@ unsigned char *use_pack(struct packed_git *p,\n \t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n \tif (offset > (p->pack_size - 20))\n \t\tdie(\"offset beyond end of packfile (truncated pack?)\");\n+\tif (offset < 0)\n+\t\tdie(\"offset before end of packfile (broken .idx?)\");\n \n \tif (!win || !in_window(win, offset)) {\n \t\tif (win)\ndiff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\nindex 0717746..a8a587a 100755\n--- a/t/t5313-pack-bounds-checks.sh\n+++ b/t/t5313-pack-bounds-checks.sh\n@@ -136,7 +136,7 @@ test_expect_success 'bogus offset into v2 extended table' '\n \ttest_must_fail git index-pack --verify $pack\n '\n \n-test_expect_failure 'bogus offset inside v2 extended table' '\n+test_expect_success 'bogus offset inside v2 extended table' '\n \t# We need two objects here, so we can plausibly require\n \t# an extended table (if the first object were larger than 2^31).\n \tdo_pack \"$object $(git rev-parse HEAD)\" --index-version=2 &&\n-- \n2.7.2.695.gf3fde8e\n"},{"id":"279365","messageId":"56CF523A.8050208@kdbg.org","threadId":"41518","inReplyTo":"20160225142112.GA17811@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-02-25T19:12:58Z","receivedAt":"2016-02-25T19:12:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.02.2016 um 15:21 schrieb Jeff King:\n> +munge () {\n> +\tprintf \"$3\" | dd of=\"$1\" bs=1 conv=notrunc seek=$2\n> +}\n\nInstead of adding another call of dd, would it be an option to insert\nthe following patch at the front of this series and then use\ntest_overwrite_bytes?\n\n---- 8< ----\nFrom: Johannes Sixt <j6t@kdbg.org>\nSubject: [PATCH] tests: overwrite bytes in files using a perl script instead of dd\n\nThe dd in my build environment on Windows crashes unpredictably. Work it\naround by rewriting most instances with a helper function that uses perl\nbehind the scenes.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n t/t1060-object-corruption.sh          |  2 +-\n t/t5300-pack-object.sh                |  8 ++++----\n t/t5302-pack-index.sh                 |  5 +++--\n t/t5303-pack-corruption-resilience.sh |  2 +-\n t/test-lib-functions.sh               | 16 ++++++++++++++++\n 5 files changed, 25 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex 3f87051..e3c5de8 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -12,7 +12,7 @@ obj_to_file() {\n corrupt_byte() {\n \tobj_file=$(obj_to_file \"$1\") &&\n \tchmod +w \"$obj_file\" &&\n-\tprintf '\\0' | dd of=\"$obj_file\" bs=1 seek=\"$2\" conv=notrunc\n+\tprintf '\\0' | test_overwrite_bytes \"$obj_file\" \"$2\"\n }\n \n test_expect_success 'setup corrupt repo' '\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex fc2be63..f45a101 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -226,7 +226,7 @@ test_expect_success \\\n test_expect_success \\\n     'verify-pack catches a corrupted pack signature' \\\n     'cat test-1-${packname_1}.pack >test-3.pack &&\n-     echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=2 &&\n+     echo | test_overwrite_bytes test-3.pack 2 &&\n      if git verify-pack test-3.idx\n      then false\n      else :;\n@@ -235,7 +235,7 @@ test_expect_success \\\n test_expect_success \\\n     'verify-pack catches a corrupted pack version' \\\n     'cat test-1-${packname_1}.pack >test-3.pack &&\n-     echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=7 &&\n+     echo | test_overwrite_bytes test-3.pack 7 &&\n      if git verify-pack test-3.idx\n      then false\n      else :;\n@@ -244,7 +244,7 @@ test_expect_success \\\n test_expect_success \\\n     'verify-pack catches a corrupted type/size of the 1st packed object data' \\\n     'cat test-1-${packname_1}.pack >test-3.pack &&\n-     echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=12 &&\n+     echo | test_overwrite_bytes test-3.pack 12 &&\n      if git verify-pack test-3.idx\n      then false\n      else :;\n@@ -255,7 +255,7 @@ test_expect_success \\\n     'l=$(wc -c <test-3.idx) &&\n      l=$(expr $l - 20) &&\n      cat test-1-${packname_1}.pack >test-3.pack &&\n-     printf \"%20s\" \"\" | dd of=test-3.idx count=20 bs=1 conv=notrunc seek=$l &&\n+     printf \"%20s\" \"\" | test_overwrite_bytes test-3.idx $l &&\n      if git verify-pack test-3.pack\n      then false\n      else :;\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex c2fc584..5a82f19 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -225,8 +225,9 @@ test_expect_success \\\n      obj=$(git hash-object file_001) &&\n      nr=$(index_obj_nr \".git/objects/pack/pack-${pack1}.idx\" $obj) &&\n      chmod +w \".git/objects/pack/pack-${pack1}.idx\" &&\n-     printf xxxx | dd of=\".git/objects/pack/pack-${pack1}.idx\" conv=notrunc \\\n-        bs=1 count=4 seek=$((8 + 256 * 4 + $(wc -l <obj-list) * 20 + $nr * 4)) &&\n+     printf xxxx |\n+\t\ttest_overwrite_bytes \".git/objects/pack/pack-${pack1}.idx\" \\\n+\t\t\t$((8 + 256 * 4 + $(wc -l <obj-list) * 20 + $nr * 4)) &&\n      ( while read obj\n        do git cat-file -p $obj >/dev/null || exit 1\n        done <obj-list ) &&\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 5940ce2..9d2e437 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -51,7 +51,7 @@ do_corrupt_object() {\n     ofs=$(git show-index < ${pack}.idx | grep $1 | cut -f1 -d\" \") &&\n     ofs=$(($ofs + $2)) &&\n     chmod +w ${pack}.pack &&\n-    dd of=${pack}.pack bs=1 conv=notrunc seek=$ofs &&\n+    test_overwrite_bytes \"${pack}.pack\" \"$ofs\" &&\n     test_must_fail git verify-pack ${pack}.pack\n }\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex ec6125d..f7ba047 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -880,6 +880,22 @@ test_skip_or_die () {\n \tesac\n }\n \n+# Overwrite bytes at an offset in a file\n+# $1 ... the file to modify\n+# $2 ... byte offset into file\n+# stdin ... new bytes\n+test_overwrite_bytes () {\n+\tperl -e '\n+\t\t$fname = shift @ARGV;\n+\t\t$offset = shift @ARGV;\n+\t\t$bytes = <>;\n+\t\topen my $fh, \"+<\", $fname\tor die \"open $fname: $!\\n\";\n+\t\tseek($fh, $offset, 0)\t\tor die \"seek $fname: $!\\n\";\n+\t\tsyswrite($fh, $bytes)\t\tor die \"write $fname: $!\\n\";\n+\t\tclose $fh\t\t\tor die \"close $fname: $!\\n\";\n+\t' \"$@\"\n+}\n+\n # The following mingw_* functions obey POSIX shell syntax, but are actually\n # bash scripts, and are meant to be used only with bash on Windows.\n \n-- \n2.7.0.118.g90056ae\n"},{"id":"279380","messageId":"xmqq4mcwo7lb.fsf@gitster.mtv.corp.google.com","threadId":"41518","inReplyTo":"56CF523A.8050208@kdbg.org","subject":"Re: [PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-25T20:31:12Z","receivedAt":"2016-02-25T20:31:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 25.02.2016 um 15:21 schrieb Jeff King:\n>> +munge () {\n>> +\tprintf \"$3\" | dd of=\"$1\" bs=1 conv=notrunc seek=$2\n>> +}\n>\n> Instead of adding another call of dd, would it be an option to insert\n> the following patch at the front of this series and then use\n> test_overwrite_bytes?\n\nIt would be an option, but I'd want to merge this to 'next' today\nwithout waiting for a reroll.  Change from dd to custom script can\nbe done as a follow-up topic after the dust settles, if necessary.\n\nThanks.\n"},{"id":"279392","messageId":"20160225220724.GB10267@sigill.intra.peff.net","threadId":"41518","inReplyTo":"xmqq4mcwo7lb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-25T22:07:24Z","receivedAt":"2016-02-25T22:07:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 25, 2016 at 12:31:12PM -0800, Junio C Hamano wrote:\n\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> > Am 25.02.2016 um 15:21 schrieb Jeff King:\n> >> +munge () {\n> >> +\tprintf \"$3\" | dd of=\"$1\" bs=1 conv=notrunc seek=$2\n> >> +}\n> >\n> > Instead of adding another call of dd, would it be an option to insert\n> > the following patch at the front of this series and then use\n> > test_overwrite_bytes?\n> \n> It would be an option, but I'd want to merge this to 'next' today\n> without waiting for a reroll.  Change from dd to custom script can\n> be done as a follow-up topic after the dust settles, if necessary.\n\nThat's fine with me, and in general I am fine with the conversion from\ndd to perl. I have to look up the dd command line options each time\nanyway (was it \"skip\" or \"seek\"...?). ;)\n\nBut note that the use of \"dd\" in the \"bogus offset inside extended\ntable\" test in my patch is different from the others. It copies a set\namount of bytes, and inserts some custom data, and then copies more.\n\nI think you could replace the first with \"head -c\", and the latter with\n\"tail -c\". In the latter you'd have to compute the distance from the end\nrather than the seek offset, but we know that already: it's 20 + 20 for\nthe trailing sha1s. Like this, I think:\n\ndiff --git a/t/t5313-pack-bounds-checks.sh b/t/t5313-pack-bounds-checks.sh\nindex a8a587a..20bc0a3 100755\n--- a/t/t5313-pack-bounds-checks.sh\n+++ b/t/t5313-pack-bounds-checks.sh\n@@ -144,10 +144,10 @@ test_expect_success 'bogus offset inside v2 extended table' '\n \t# We have to make extra room for the table, so we cannot\n \t# just munge in place as usual.\n \t{\n-\t\tdd if=$idx bs=1 count=$(($(ofs_table 2) + 4)) &&\n+\t\thead -c $(($(ofs_table 2) + 4)) $idx &&\n \t\tprintf \"\\200\\0\\0\\0\" &&\n \t\tprintf \"\\377\\0\\0\\0\\0\\0\\0\\0\" &&\n-\t\tdd if=$idx bs=1 skip=$(extended_table 2)\n+\t\ttail -c 40 $idx\n \t} >tmp &&\n \tmv tmp \"$idx\" &&\n \tclear_base &&\n"},{"id":"279630","messageId":"1456559373-13589-1-git-send-email-pclouds@gmail.com","threadId":"41518","inReplyTo":"20160225142004.GA17678@sigill.intra.peff.net","subject":"[PATCH 4/3] sha1_file.c: mark strings for translation","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-27T07:49:33Z","receivedAt":"2016-02-27T07:49:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Since jk/pack-idx-corruption-safety is already in 'next', can we add\n this patch on top? Surrounding strings are handled separately [1] by\n another series.\n\n [1] http://thread.gmane.org/gmane.comp.version-control.git/287661/focus=287678\n\n sha1_file.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 4a3a032..b8da68b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1042,7 +1042,7 @@ unsigned char *use_pack(struct packed_git *p,\n \tif (offset > (p->pack_size - 20))\n \t\tdie(\"offset beyond end of packfile (truncated pack?)\");\n \tif (offset < 0)\n-\t\tdie(\"offset before end of packfile (broken .idx?)\");\n+\t\tdie(_(\"offset before end of packfile (broken .idx?)\"));\n \n \tif (!win || !in_window(win, offset)) {\n \t\tif (win)\n@@ -2367,11 +2367,11 @@ void check_pack_index_ptr(const struct packed_git *p, const void *vptr)\n \tconst unsigned char *start = p->index_data;\n \tconst unsigned char *end = start + p->index_size;\n \tif (ptr < start)\n-\t\tdie(\"offset before start of pack index for %s (corrupt index?)\",\n+\t\tdie(_(\"offset before start of pack index for %s (corrupt index?)\"),\n \t\t    p->pack_name);\n \t/* No need to check for underflow; .idx files must be at least 8 bytes */\n \tif (ptr >= end - 8)\n-\t\tdie(\"offset beyond end of pack index for %s (truncated index?)\",\n+\t\tdie(_(\"offset beyond end of pack index for %s (truncated index?)\"),\n \t\t    p->pack_name);\n }\n \n-- \n2.8.0.rc0.205.g7ec8cf1\n"},{"id":"279649","messageId":"xmqq37sehwzu.fsf@gitster.mtv.corp.google.com","threadId":"41518","inReplyTo":"1456559373-13589-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 4/3] sha1_file.c: mark strings for translation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-27T17:41:09Z","receivedAt":"2016-02-27T17:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  Since jk/pack-idx-corruption-safety is already in 'next', can we add\n>  this patch on top? Surrounding strings are handled separately [1] by\n>  another series.\n>\n>  [1] http://thread.gmane.org/gmane.comp.version-control.git/287661/focus=287678\n\nThanks, I think this makes sense.  Peff--I do not think I missed a\nreason we shouldn't take this?\n\n>\n>  sha1_file.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 4a3a032..b8da68b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1042,7 +1042,7 @@ unsigned char *use_pack(struct packed_git *p,\n>  \tif (offset > (p->pack_size - 20))\n>  \t\tdie(\"offset beyond end of packfile (truncated pack?)\");\n>  \tif (offset < 0)\n> -\t\tdie(\"offset before end of packfile (broken .idx?)\");\n> +\t\tdie(_(\"offset before end of packfile (broken .idx?)\"));\n>  \n>  \tif (!win || !in_window(win, offset)) {\n>  \t\tif (win)\n> @@ -2367,11 +2367,11 @@ void check_pack_index_ptr(const struct packed_git *p, const void *vptr)\n>  \tconst unsigned char *start = p->index_data;\n>  \tconst unsigned char *end = start + p->index_size;\n>  \tif (ptr < start)\n> -\t\tdie(\"offset before start of pack index for %s (corrupt index?)\",\n> +\t\tdie(_(\"offset before start of pack index for %s (corrupt index?)\"),\n>  \t\t    p->pack_name);\n>  \t/* No need to check for underflow; .idx files must be at least 8 bytes */\n>  \tif (ptr >= end - 8)\n> -\t\tdie(\"offset beyond end of pack index for %s (truncated index?)\",\n> +\t\tdie(_(\"offset beyond end of pack index for %s (truncated index?)\"),\n>  \t\t    p->pack_name);\n>  }\n"},{"id":"279658","messageId":"20160227182542.GA12822@sigill.intra.peff.net","threadId":"41518","inReplyTo":"xmqq37sehwzu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/3] sha1_file.c: mark strings for translation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-27T18:25:42Z","receivedAt":"2016-02-27T18:25:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 27, 2016 at 09:41:09AM -0800, Junio C Hamano wrote:\n\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n> \n> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> > ---\n> >  Since jk/pack-idx-corruption-safety is already in 'next', can we add\n> >  this patch on top? Surrounding strings are handled separately [1] by\n> >  another series.\n> >\n> >  [1] http://thread.gmane.org/gmane.comp.version-control.git/287661/focus=287678\n> \n> Thanks, I think this makes sense.  Peff--I do not think I missed a\n> reason we shouldn't take this?\n\nNo, seems fine to me.\n\n-Peff\n"}]}