{"thread":{"id":"47138","subject":"Invalid memory access in `git apply`","startedAt":"2017-11-08T16:58:29Z","lastAt":"2017-11-16T18:50:48Z","messageCount":5,"participants":["mqudsi@neosmart.net","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"332084","messageId":"0101015f9c91871f-2f750aec-6877-4e29-9c15-c8399670dd48-000000@us-west-2.amazonses.com","threadId":"47138","inReplyTo":null,"subject":"Invalid memory access in `git apply`","fromName":"","fromEmail":"mqudsi@neosmart.net","sentAt":"2017-11-08T16:58:23Z","receivedAt":"2017-11-08T16:58:29Z","isPatch":false,"sender":{"key":"mqudsi@neosmart.net","avatar":"https://gravatar.com/avatar/c2643dd7c6df61aed49d9f3d917ac6d61cafbbda5f9b1619f50d3b749dca415a?d=mp&s=160"},"body":"**Resending as it seems that the attachments caused the last email to wind up\nin a black hole**\n\nThere seems to be bug in the `git apply` that leads to out-of-bounds memory\naccess when --ignore-space-change is combined with --inaccurate-eof and\napplying a patch.\n\nOn occasion, this can lead to error output like the following:\n\n\t mqudsi@ZBook ~> git apply --ignore-space-change --ignore-whitespace\n\t --allow-overlap --inaccurate-eof without_whitespace.diff\n\t *** Error in `git': malloc(): memory corruption: 0x0000000002543530 ***\n\t ======= Backtrace: =========\n\t /lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7fdda79c77e5]\n\t /lib/x86_64-linux-gnu/libc.so.6(+0x8213e)[0x7fdda79d213e]\n\t /lib/x86_64-linux-gnu/libc.so.6(__libc_malloc+0x54)[0x7fdda79d4184]\n\t /lib/x86_64-linux-gnu/libc.so.6(_IO_file_doallocate+0x55)[0x7fdda79bd1d5]\n\t /lib/x86_64-linux-gnu/libc.so.6(_IO_doallocbuf+0x34)[0x7fdda79cb594]\n\t /lib/x86_64-linux-gnu/libc.so.6(_IO_file_overflow+0x1c8)[0x7fdda79ca8f8]\n\t /lib/x86_64-linux-gnu/libc.so.6(_IO_file_xsputn+0xad)[0x7fdda79c928d]\n\t /lib/x86_64-linux-gnu/libc.so.6(fputs+0x98)[0x7fdda79be0c8]\n\t git[0x5386cd]\n\t git[0x538714]\n\t git[0x538940]\n\t git[0x40e220]\n\t git[0x410a10]\n\t git[0x41256e]\n\t git[0x412df7]\n\t git[0x415935]\n\t git[0x406436]\n\t git[0x40555c]\n\nThe original file being patched (clipboard.vim) and the patch file that I had\nattempted to apply (without_whitespace.diff) are attached, along with the\nfull, unabridged output of the memory map as a result of the out-of-bounds\naccess (memory_map.txt).\n\nThe memory map output was generated under git 2.7.4; repeated attempts to\nreproduce the memory map dump with both 2.7.4 and 2.15 produce the following\noutput:\n\n\t mqudsi@ZBook ~/.c/nvim> git apply --ignore-space-change --inaccurate-eof\n\t --whitespace=fix without_whitespace.diff\n\t fatal: BUG: caller miscounted postlen: asked 248, orig = 251, used = 249\n\nMahmoud Al-Qudsi\nNeoSmart Technologies\n\n--Attachments--\n\n* clipboard.vim: http://termbin.com/u25t\n* without_whitespace.diff: http://termbin.com/bu9y\n* memory_map.txt: http://termbin.com/cboz\n\n\n"},{"id":"332288","messageId":"fdc7b058-e891-6d74-adad-f7e9a853e420@web.de","threadId":"47138","inReplyTo":"0101015f9c91871f-2f750aec-6877-4e29-9c15-c8399670dd48-000000@us-west-2.amazonses.com","subject":"Re: Invalid memory access in `git apply`","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-11T14:10:12Z","receivedAt":"2017-11-11T14:10:24Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.11.2017 um 17:58 schrieb mqudsi@neosmart.net:\n> **Resending as it seems that the attachments caused the last email to wind up\n> in a black hole**\n> \n> There seems to be bug in the `git apply` that leads to out-of-bounds memory\n> access when --ignore-space-change is combined with --inaccurate-eof and\n> applying a patch.\n> \n> On occasion, this can lead to error output like the following:\n> \n> \t mqudsi@ZBook ~> git apply --ignore-space-change --ignore-whitespace\n> \t --allow-overlap --inaccurate-eof without_whitespace.diff\n> \t *** Error in `git': malloc(): memory corruption: 0x0000000002543530 ***\n> \t ======= Backtrace: =========\n> \t /lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7fdda79c77e5]\n> \t /lib/x86_64-linux-gnu/libc.so.6(+0x8213e)[0x7fdda79d213e]\n> \t /lib/x86_64-linux-gnu/libc.so.6(__libc_malloc+0x54)[0x7fdda79d4184]\n> \t /lib/x86_64-linux-gnu/libc.so.6(_IO_file_doallocate+0x55)[0x7fdda79bd1d5]\n> \t /lib/x86_64-linux-gnu/libc.so.6(_IO_doallocbuf+0x34)[0x7fdda79cb594]\n> \t /lib/x86_64-linux-gnu/libc.so.6(_IO_file_overflow+0x1c8)[0x7fdda79ca8f8]\n> \t /lib/x86_64-linux-gnu/libc.so.6(_IO_file_xsputn+0xad)[0x7fdda79c928d]\n> \t /lib/x86_64-linux-gnu/libc.so.6(fputs+0x98)[0x7fdda79be0c8]\n> \t git[0x5386cd]\n> \t git[0x538714]\n> \t git[0x538940]\n> \t git[0x40e220]\n> \t git[0x410a10]\n> \t git[0x41256e]\n> \t git[0x412df7]\n> \t git[0x415935]\n> \t git[0x406436]\n> \t git[0x40555c]\n> \n> The original file being patched (clipboard.vim) and the patch file that I had\n> attempted to apply (without_whitespace.diff) are attached, along with the\n> full, unabridged output of the memory map as a result of the out-of-bounds\n> access (memory_map.txt).\n> \n> The memory map output was generated under git 2.7.4; repeated attempts to\n> reproduce the memory map dump with both 2.7.4 and 2.15 produce the following\n> output:\n> \n> \t mqudsi@ZBook ~/.c/nvim> git apply --ignore-space-change --inaccurate-eof\n> \t --whitespace=fix without_whitespace.diff\n> \t fatal: BUG: caller miscounted postlen: asked 248, orig = 251, used = 249\n> \n> Mahmoud Al-Qudsi\n> NeoSmart Technologies\n> \n> --Attachments--\n> \n> * clipboard.vim: http://termbin.com/u25t\n> * without_whitespace.diff: http://termbin.com/bu9y\n> * memory_map.txt: http://termbin.com/cboz\n\nThank you for reporting the issue!\n\nThere seem to be at least two bugs in git apply and two problems on your\nend. You don't seem need the option --inaccurate-eof and it's causing\ntrouble for you; I suggest to leave it out.\n\nAnd the second hunk of your diff doesn't apply because the \"endif\"\ncontext line doesn't match the \"endif\" line in clipboard.vim which has\nno leading whitespace. --ignore-space-change ignores changes in the\nnumber of whitespace characters, but that number cannot be 0 on only one\nside.\n\nIf you adjust the diff by removing the tab from that context line or add\none or more spaces in clipboard.vim before the last \"endif\" then it will\napply without --inaccurate-eof.\n\n\nOne of the bugs is that fuzzy_matchlines() does out-of-bounds reads in\nsome cases. You should only notice it with a tool like Valgrind, ASan\nor perhaps a hardened malloc(3). I'll send a separate patch for that.\n\nThe second bug is that --inaccurate-eof triggers a sanity check when\nused together with --ignore-space-change. Here's a simpler reproduction\nrecipe:\n\n git init repo\n cd repo\n\n echo 1 >a\n git add a\n git commit -m initial\n\n echo 2 >a\n git diff >a.diff\n\t\n git reset --hard\n git apply --ignore-space-change --inaccurate-eof a.diff\n\nWhich yields this error message:\n\n fatal: BUG: caller miscounted postlen: asked 1, orig = 1, used = 2\n\nPerhaps the first thing we'd need would be a couple of tests showing\nthe expected behavior of git apply --inaccurate-eof with and without\ntrailing newlines..\n\nRené\n"},{"id":"332289","messageId":"6ff27de7-ac22-3f2f-1f3b-2e0e6f10605a@web.de","threadId":"47138","inReplyTo":"0101015f9c91871f-2f750aec-6877-4e29-9c15-c8399670dd48-000000@us-west-2.amazonses.com","subject":"[PATCH] apply: avoid out-of-bounds access in fuzzy_matchlines()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-11T14:10:19Z","receivedAt":"2017-11-11T14:10:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"fuzzy_matchlines() uses a pointers to the first and last characters of\ntwo lines to keep track while matching them. This makes it impossible\nto deal with empty strings. It accesses characters before the start of\nempty lines. It can also access characters after the end when checking\nfor trailing whitespace in the main loop.\n\nAvoid that by using pointers to the first character and the one *after*\nthe last one. This is well-defined as long as the latter is not\ndereferenced. Basically rewrite the function based on that premise; it\nbecomes much simpler as a result. There is no need to check for\nleading whitespace outside of the main loop anymore.\n\nReported-by: Mahmoud Al-Qudsi \nSigned-off-by: Rene Scharfe \n---\n apply.c | 59 ++++++++++++++++++++---------------------------------------\n 1 file changed, 20 insertions(+), 39 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex d676debd59..b8087bd29c 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -300,52 +300,33 @@ static uint32_t hash_line(const char *cp, size_t len)\n static int fuzzy_matchlines(const char *s1, size_t n1,\n \t\t\t const char *s2, size_t n2)\n {\n-\tconst char *last1 = s1 + n1 - 1;\n-\tconst char *last2 = s2 + n2 - 1;\n-\tint result = 0;\n+\tconst char *end1 = s1 + n1;\n+\tconst char *end2 = s2 + n2;\n \n \t/* ignore line endings */\n-\twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n-\t\tlast1--;\n-\twhile ((*last2 == '\\r') || (*last2 == '\\n'))\n-\t\tlast2--;\n-\n-\t/* skip leading whitespaces, if both begin with whitespace */\n-\tif (s1 <= last1 && s2 <= last2 && isspace(*s1) && isspace(*s2)) {\n-\t\twhile (isspace(*s1) && (s1 <= last1))\n-\t\t\ts1++;\n-\t\twhile (isspace(*s2) && (s2 <= last2))\n-\t\t\ts2++;\n-\t}\n-\t/* early return if both lines are empty */\n-\tif ((s1 > last1) && (s2 > last2))\n-\t\treturn 1;\n-\twhile (!result) {\n-\t\tresult = *s1++ - *s2++;\n-\t\t/*\n-\t\t * Skip whitespace inside. We check for whitespace on\n-\t\t * both buffers because we don't want \"a b\" to match\n-\t\t * \"ab\"\n-\t\t */\n-\t\tif (isspace(*s1) && isspace(*s2)) {\n-\t\t\twhile (isspace(*s1) && s1 <= last1)\n+\twhile (s1 < end1 && (end1[-1] == '\\r' || end1[-1] == '\\n'))\n+\t\tend1--;\n+\twhile (s2 < end2 && (end2[-1] == '\\r' || end2[-1] == '\\n'))\n+\t\tend2--;\n+\n+\twhile (s1 < end1 && s2 < end2) {\n+\t\tif (isspace(*s1)) {\n+\t\t\t/*\n+\t\t\t * Skip whitespace. We check on both buffers\n+\t\t\t * because we don't want \"a b\" to match \"ab\".\n+\t\t\t */\n+\t\t\tif (!isspace(*s2))\n+\t\t\t\treturn 0;\n+\t\t\twhile (s1 < end1 && isspace(*s1))\n \t\t\t\ts1++;\n-\t\t\twhile (isspace(*s2) && s2 <= last2)\n+\t\t\twhile (s2 < end2 && isspace(*s2))\n \t\t\t\ts2++;\n-\t\t}\n-\t\t/*\n-\t\t * If we reached the end on one side only,\n-\t\t * lines don't match\n-\t\t */\n-\t\tif (\n-\t\t ((s2 > last2) && (s1 <= last1)) ||\n-\t\t ((s1 > last1) && (s2 <= last2)))\n+\t\t} else if (*s1++ != *s2++)\n \t\t\treturn 0;\n-\t\tif ((s1 > last1) && (s2 > last2))\n-\t\t\tbreak;\n \t}\n \n-\treturn !result;\n+\t/* If we reached the end on one side only, lines don't match. */\n+\treturn s1 == end1 && s2 == end2;\n }\n \n static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)\n-- \n2.15.0\n"},{"id":"332308","messageId":"xmqqinegcdfo.fsf@gitster.mtv.corp.google.com","threadId":"47138","inReplyTo":"6ff27de7-ac22-3f2f-1f3b-2e0e6f10605a@web.de","subject":"Re: [PATCH] apply: avoid out-of-bounds access in fuzzy_matchlines()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-12T04:45:47Z","receivedAt":"2017-11-12T04:45:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> fuzzy_matchlines() uses a pointers to the first and last characters of\n> two lines to keep track while matching them. This makes it impossible\n> to deal with empty strings. It accesses characters before the start of\n> empty lines. It can also access characters after the end when checking\n> for trailing whitespace in the main loop.\n>\n> Avoid that by using pointers to the first character and the one *after*\n> the last one. This is well-defined as long as the latter is not\n> dereferenced. Basically rewrite the function based on that premise; it\n> becomes much simpler as a result. There is no need to check for\n> leading whitespace outside of the main loop anymore.\n\nI recall vaguely that we were bitten by a bug or two due to another\ninstance of that deviates from the usual \"close on the\nleft end, open on the right end\" convention somewhere in the system\nrecently?\n\nI think the fix of the function is correct, but at the same time, we\nwould want to clean it up after this fix lands by replacing the\nfunction with the line comparison function we already have in the\nxdiff/ layer, so that we can (1) reduce the code duplication and (2)\nmore importantly, do not have to be constrained by the (mistakenly\nnarrow) policy decision we currently seem to have to support only\n\"ignore-whitespace-change\" and nothing else. Of course, that should\nnot be done as part of this fix. It is strictly a #leftoverbits item.\n\nThanks.\n\n> Reported-by: Mahmoud Al-Qudsi \n> Signed-off-by: Rene Scharfe \n> ---\n> apply.c | 59 ++++++++++++++++++++---------------------------------------\n> 1 file changed, 20 insertions(+), 39 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index d676debd59..b8087bd29c 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -300,52 +300,33 @@ static uint32_t hash_line(const char *cp, size_t len)\n> static int fuzzy_matchlines(const char *s1, size_t n1,\n> \t\t\t const char *s2, size_t n2)\n> {\n> -\tconst char *last1 = s1 + n1 - 1;\n> -\tconst char *last2 = s2 + n2 - 1;\n> -\tint result = 0;\n> +\tconst char *end1 = s1 + n1;\n> +\tconst char *end2 = s2 + n2;\n> \n> \t/* ignore line endings */\n> -\twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n> -\t\tlast1--;\n> -\twhile ((*last2 == '\\r') || (*last2 == '\\n'))\n> -\t\tlast2--;\n> -\n> -\t/* skip leading whitespaces, if both begin with whitespace */\n> -\tif (s1 <= last1 && s2 <= last2 && isspace(*s1) && isspace(*s2)) {\n> -\t\twhile (isspace(*s1) && (s1 <= last1))\n> -\t\t\ts1++;\n> -\t\twhile (isspace(*s2) && (s2 <= last2))\n> -\t\t\ts2++;\n> -\t}\n> -\t/* early return if both lines are empty */\n> -\tif ((s1 > last1) && (s2 > last2))\n> -\t\treturn 1;\n> -\twhile (!result) {\n> -\t\tresult = *s1++ - *s2++;\n> -\t\t/*\n> -\t\t * Skip whitespace inside. We check for whitespace on\n> -\t\t * both buffers because we don't want \"a b\" to match\n> -\t\t * \"ab\"\n> -\t\t */\n> -\t\tif (isspace(*s1) && isspace(*s2)) {\n> -\t\t\twhile (isspace(*s1) && s1 <= last1)\n> +\twhile (s1 < end1 && (end1[-1] == '\\r' || end1[-1] == '\\n'))\n> +\t\tend1--;\n> +\twhile (s2 < end2 && (end2[-1] == '\\r' || end2[-1] == '\\n'))\n> +\t\tend2--;\n> +\n> +\twhile (s1 < end1 && s2 < end2) {\n> +\t\tif (isspace(*s1)) {\n> +\t\t\t/*\n> +\t\t\t * Skip whitespace. We check on both buffers\n> +\t\t\t * because we don't want \"a b\" to match \"ab\".\n> +\t\t\t */\n> +\t\t\tif (!isspace(*s2))\n> +\t\t\t\treturn 0;\n> +\t\t\twhile (s1 < end1 && isspace(*s1))\n> \t\t\t\ts1++;\n> -\t\t\twhile (isspace(*s2) && s2 <= last2)\n> +\t\t\twhile (s2 < end2 && isspace(*s2))\n> \t\t\t\ts2++;\n> -\t\t}\n> -\t\t/*\n> -\t\t * If we reached the end on one side only,\n> -\t\t * lines don't match\n> -\t\t */\n> -\t\tif (\n> -\t\t ((s2 > last2) && (s1 <= last1)) ||\n> -\t\t ((s1 > last1) && (s2 <= last2)))\n> +\t\t} else if (*s1++ != *s2++)\n> \t\t\treturn 0;\n> -\t\tif ((s1 > last1) && (s2 > last2))\n> -\t\t\tbreak;\n> \t}\n> \n> -\treturn !result;\n> +\t/* If we reached the end on one side only, lines don't match. */\n> +\treturn s1 == end1 && s2 == end2;\n> }\n> \n> static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)\n"},{"id":"332717","messageId":"acc527e8-f617-6230-9872-0b23ac24f8e5@web.de","threadId":"47138","inReplyTo":"0101015f9c91871f-2f750aec-6877-4e29-9c15-c8399670dd48-000000@us-west-2.amazonses.com","subject":"[PATCH] apply: update line lengths for --inaccurate-eof","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-16T18:50:31Z","receivedAt":"2017-11-16T18:50:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Some diff implementations don't report missing newlines at the end of\nfiles. Applying such a patch can cause a newline character to be\nadded inadvertently. The option --inaccurate-eof of git apply can be\nused to remove trailing newlines if needed.\n\napply_one_fragment() cuts it off from the buffers for preimage and\npostimage. Before it does, it builds an array with the lengths of each\nline for both. Make sure to update the length of the last line in\nthese line info structures as well to keep them consistent with their\nrespective buffer.\n\nWithout this fix the added test fails; git apply dies and reports:\n\n fatal: BUG: caller miscounted postlen: asked 1, orig = 1, used = 2\n\nThat sanity check is only called if whitespace changes are ignored.\n\nReported-by: Mahmoud Al-Qudsi \nSigned-off-by: Rene Scharfe \n---\n apply.c | 2 ++\n t/t4107-apply-ignore-whitespace.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/apply.c b/apply.c\nindex b8087bd29c..321a9fa68d 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2953,6 +2953,8 @@ static int apply_one_fragment(struct apply_state *state,\n \t newlines.len > 0 && newlines.buf[newlines.len - 1] == '\\n') {\n \t\told--;\n \t\tstrbuf_setlen(&newlines, newlines.len - 1);\n+\t\tpreimage.line_allocated[preimage.nr - 1].len--;\n+\t\tpostimage.line_allocated[postimage.nr - 1].len--;\n \t}\n \n \tleading = frag->leading;\ndiff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh\nindex 9e29b5262d..ac72eeaf27 100755\n--- a/t/t4107-apply-ignore-whitespace.sh\n+++ b/t/t4107-apply-ignore-whitespace.sh\n@@ -178,4 +178,18 @@ test_expect_success 'patch5 fails (--no-ignore-whitespace)' '\n \ttest_must_fail git apply --no-ignore-whitespace patch5.patch\n '\n \n+test_expect_success 'apply --ignore-space-change --inaccurate-eof' '\n+\techo 1 >file &&\n+\tgit apply --ignore-space-change --inaccurate-eof <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1 +1 @@\n+\t-1\n+\t+2\n+\tEOF\n+\tprintf 2 >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_done\n-- \n2.15.0\n"}]}