{"thread":{"id":"27418","subject":"[PATCH] do not read beyond end of malloc'd buffer","startedAt":"2011-05-20T17:20:12Z","lastAt":"2011-05-21T09:42:40Z","messageCount":3,"participants":["Jim Meyering","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"168326","messageId":"877h9lb86r.fsf@rho.meyering.net","threadId":"27418","inReplyTo":null,"subject":"[PATCH] do not read beyond end of malloc'd buffer","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-05-20T17:20:12Z","receivedAt":"2011-05-20T17:20:12Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"I was surprised to see \"git diff --word-diff\" output a ton of\ngarbage, and tracked it down to a bug that's triggered when the\ndiff.suppress-blank-empty config option to true and when at least\none of the context lines is empty.\n\nHere's a quick demo you can run in an empty directory:\n\n    printf 'a\\n\\n[-b-]{+c+}\\n' > exp\n    git init && git config diff.suppress-blank-empty true\n    printf 'a\\n\\nb\\n' > f && git add . && git commit -m. .\n    printf 'a\\n\\nc\\n' > f\n    git diff --word-diff | tail -3 > out\n    diff out exp\n\nBefore the patch, the git diff ... command would read from beyond\nthe end of a heap buffer, and \"out\" would contain far more than the\nexpected 5 bytes.\n\nHere's the patch:\n\n-- >8 --\nSubject: [PATCH] do not read beyond end of malloc'd buffer\n\nWith diff.suppress-blank-empty=true, \"git diff --word-diff\" would\noutput data that had been read from uninitialized heap memory.\nThe problem was that fn_out_consume did not account for the\npossibility of a line with length 1, i.e., the empty context line\nthat diff.suppress-blank-empty=true converts from \" \\n\" to \"\\n\".\nSince it assumed there would always be a prefix character (the space),\nit decremented \"len\" unconditionally, thus passing len=0 to emit_line,\nwhich would then blindly call emit_line_0 with len=-1 which would\npass that value on to fwrite as SIZE_MAX.  Boom.\n\nSigned-off-by: Jim Meyering <meyering@redhat.com>\n---\n diff.c |    9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ba5f7aa..57eea74 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1117,8 +1117,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\t\temit_line(ecbdata->opt, plain, reset, line, len);\n \t\t\tfputs(\"~\\n\", ecbdata->opt->file);\n \t\t} else {\n-\t\t\t/* don't print the prefix character */\n-\t\t\temit_line(ecbdata->opt, plain, reset, line+1, len-1);\n+\t\t\t/* If there is a prefix character, skip it.\n+\t\t\t   With diff_suppress_blank_empty, there may be none. */\n+\t\t\tif (line[0] != '\\n') {\n+\t\t\t      line++;\n+\t\t\t      len--;\n+\t\t\t}\n+\t\t\temit_line(ecbdata->opt, plain, reset, line, len);\n \t\t}\n \t\treturn;\n \t}\n--\n1.7.5.2.316.gbcebc8b\n"},{"id":"168330","messageId":"7vfwo92p19.fsf@alter.siamese.dyndns.org","threadId":"27418","inReplyTo":"877h9lb86r.fsf@rho.meyering.net","subject":"Re: [PATCH] do not read beyond end of malloc'd buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-20T18:41:06Z","receivedAt":"2011-05-20T18:41:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering <jim@meyering.net> writes:\n\n> I was surprised to see \"git diff --word-diff\" output a ton of\n> garbage, and tracked it down to a bug that's triggered when the\n> diff.suppress-blank-empty config option to true and when at least\n> one of the context lines is empty.\n\nHeh, I am not that surprised ;-) \n\nI think the real culprit is a year-old 882749a (diff: add --word-diff\noption that generalizes --color-words, 2010-04-14); it probably shows that\nnot many people use diff.s-b-e settings?\n\n>     printf 'a\\n\\n[-b-]{+c+}\\n' > exp\n>     git init && git config diff.suppress-blank-empty true\n>     printf 'a\\n\\nb\\n' > f && git add . && git commit -m. .\n>     printf 'a\\n\\nc\\n' > f\n>     git diff --word-diff | tail -3 > out\n>     diff out exp\n>\n> Before the patch, the git diff ... command would read from beyond\n> the end of a heap buffer, and \"out\" would contain far more than the\n> expected 5 bytes.\n\nIt is a bit unfortunate that we cannot make this into a test script, as it\ndepends on what is on the uninitialized part of the heap, which might\nhappen to be a NUL in which case the test would pass.\n\nRunning tests under the valgrind mode may catch issues, though.\n\nThanks. Will queue with this test squashed in.\n\ndiff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh\nindex 37aeab0..c374aa4 100755\n--- a/t/t4034-diff-words.sh\n+++ b/t/t4034-diff-words.sh\n@@ -307,4 +307,30 @@ test_language_driver python\n test_language_driver ruby\n test_language_driver tex\n \n+test_expect_success 'word-diff with diff.sbe' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/pre b/post\n+\tindex a1a53b5..bc8fe6d 100644\n+\t--- a/pre\n+\t+++ b/post\n+\t@@ -1,3 +1,3 @@\n+\ta\n+\n+\t[-b-]{+c+}\n+\tEOF\n+\tcat >pre <<-\\EOF &&\n+\ta\n+\n+\tb\n+\tEOF\n+\tcat >post <<-\\EOF &&\n+\ta\n+\n+\tc\n+\tEOF\n+\ttest_when_finished \"git config --unset diff.suppress-blank-empty\" &&\n+\tgit config diff.suppress-blank-empty true &&\n+\tword_diff --word-diff=plain\n+'\n+\n test_done\n"},{"id":"168371","messageId":"87ei3s75kf.fsf@rho.meyering.net","threadId":"27418","inReplyTo":"7vfwo92p19.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] do not read beyond end of malloc'd buffer","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2011-05-21T09:42:40Z","receivedAt":"2011-05-21T09:42:40Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering <jim@meyering.net> writes:\n>> I was surprised to see \"git diff --word-diff\" output a ton of\n>> garbage, and tracked it down to a bug that's triggered when the\n>> diff.suppress-blank-empty config option to true and when at least\n>> one of the context lines is empty.\n>\n> Heh, I am not that surprised ;-)\n>\n> I think the real culprit is a year-old 882749a (diff: add --word-diff\n> option that generalizes --color-words, 2010-04-14); it probably shows that\n> not many people use diff.s-b-e settings?\n\nThat must be the issue: too few people know to use diff.s-b-e.\nIf a few more of us were to set that option by running this command:\n\n    git config --global diff.suppress-blank-empty true\n\n(to suppress emission of trailing blanks on empty context lines)\nthen bugs like this would be exposed more quickly.\n\n...\n> It is a bit unfortunate that we cannot make this into a test script, as it\n> depends on what is on the uninitialized part of the heap, which might\n> happen to be a NUL in which case the test would pass.\n>\n> Running tests under the valgrind mode may catch issues, though.\n\nRight, since reading the trailing NUL byte after the\nend of the buffer would be detected.\n\n> Thanks. Will queue with this test squashed in.\n\nThanks for adding the test!\n"}]}