# [PATCH] do not read beyond end of malloc'd buffer 3 messages from 2011-05-20 to 2011-05-21. Participants: Jim Meyering, Junio C Hamano. Thread: https://gitlist.dev/t/27418 ## Jim Meyering, 2011-05-20 17:20 Subject: [PATCH] do not read beyond end of malloc'd buffer Message-ID: <877h9lb86r.fsf@rho.meyering.net> URL: https://gitlist.dev/e/877h9lb86r.fsf%40rho.meyering.net ``` I was surprised to see "git diff --word-diff" output a ton of garbage, and tracked it down to a bug that's triggered when the diff.suppress-blank-empty config option to true and when at least one of the context lines is empty. Here's a quick demo you can run in an empty directory: printf 'a\n\n[-b-]{+c+}\n' > exp git init && git config diff.suppress-blank-empty true printf 'a\n\nb\n' > f && git add . && git commit -m. . printf 'a\n\nc\n' > f git diff --word-diff | tail -3 > out diff out exp Before the patch, the git diff ... command would read from beyond the end of a heap buffer, and "out" would contain far more than the expected 5 bytes. Here's the patch: -- >8 -- Subject: [PATCH] do not read beyond end of malloc'd buffer With diff.suppress-blank-empty=true, "git diff --word-diff" would output data that had been read from uninitialized heap memory. The problem was that fn_out_consume did not account for the possibility of a line with length 1, i.e., the empty context line that diff.suppress-blank-empty=true converts from " \n" to "\n". Since it assumed there would always be a prefix character (the space), it decremented "len" unconditionally, thus passing len=0 to emit_line, which would then blindly call emit_line_0 with len=-1 which would pass that value on to fwrite as SIZE_MAX. Boom. Signed-off-by: Jim Meyering --- diff.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/diff.c b/diff.c index ba5f7aa..57eea74 100644 --- a/diff.c +++ b/diff.c @@ -1117,8 +1117,13 @@ static void fn_out_consume(void *priv, char *line, unsigned long len) emit_line(ecbdata->opt, plain, reset, line, len); fputs("~\n", ecbdata->opt->file); } else { - /* don't print the prefix character */ - emit_line(ecbdata->opt, plain, reset, line+1, len-1); + /* If there is a prefix character, skip it. + With diff_suppress_blank_empty, there may be none. */ + if (line[0] != '\n') { + line++; + len--; + } + emit_line(ecbdata->opt, plain, reset, line, len); } return; } -- 1.7.5.2.316.gbcebc8b ``` ## Junio C Hamano, 2011-05-20 18:41 Subject: Re: [PATCH] do not read beyond end of malloc'd buffer Message-ID: <7vfwo92p19.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfwo92p19.fsf%40alter.siamese.dyndns.org In-Reply-To: <877h9lb86r.fsf@rho.meyering.net> ``` Jim Meyering writes: > I was surprised to see "git diff --word-diff" output a ton of > garbage, and tracked it down to a bug that's triggered when the > diff.suppress-blank-empty config option to true and when at least > one of the context lines is empty. Heh, I am not that surprised ;-) I think the real culprit is a year-old 882749a (diff: add --word-diff option that generalizes --color-words, 2010-04-14); it probably shows that not many people use diff.s-b-e settings? > printf 'a\n\n[-b-]{+c+}\n' > exp > git init && git config diff.suppress-blank-empty true > printf 'a\n\nb\n' > f && git add . && git commit -m. . > printf 'a\n\nc\n' > f > git diff --word-diff | tail -3 > out > diff out exp > > Before the patch, the git diff ... command would read from beyond > the end of a heap buffer, and "out" would contain far more than the > expected 5 bytes. It is a bit unfortunate that we cannot make this into a test script, as it depends on what is on the uninitialized part of the heap, which might happen to be a NUL in which case the test would pass. Running tests under the valgrind mode may catch issues, though. Thanks. Will queue with this test squashed in. diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh index 37aeab0..c374aa4 100755 --- a/t/t4034-diff-words.sh +++ b/t/t4034-diff-words.sh @@ -307,4 +307,30 @@ test_language_driver python test_language_driver ruby test_language_driver tex +test_expect_success 'word-diff with diff.sbe' ' + cat >expect <<-\EOF && + diff --git a/pre b/post + index a1a53b5..bc8fe6d 100644 + --- a/pre + +++ b/post + @@ -1,3 +1,3 @@ + a + + [-b-]{+c+} + EOF + cat >pre <<-\EOF && + a + + b + EOF + cat >post <<-\EOF && + a + + c + EOF + test_when_finished "git config --unset diff.suppress-blank-empty" && + git config diff.suppress-blank-empty true && + word_diff --word-diff=plain +' + test_done ``` ## Jim Meyering, 2011-05-21 09:42 Subject: Re: [PATCH] do not read beyond end of malloc'd buffer Message-ID: <87ei3s75kf.fsf@rho.meyering.net> URL: https://gitlist.dev/e/87ei3s75kf.fsf%40rho.meyering.net In-Reply-To: <7vfwo92p19.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Jim Meyering writes: >> I was surprised to see "git diff --word-diff" output a ton of >> garbage, and tracked it down to a bug that's triggered when the >> diff.suppress-blank-empty config option to true and when at least >> one of the context lines is empty. > > Heh, I am not that surprised ;-) > > I think the real culprit is a year-old 882749a (diff: add --word-diff > option that generalizes --color-words, 2010-04-14); it probably shows that > not many people use diff.s-b-e settings? That must be the issue: too few people know to use diff.s-b-e. If a few more of us were to set that option by running this command: git config --global diff.suppress-blank-empty true (to suppress emission of trailing blanks on empty context lines) then bugs like this would be exposed more quickly. ... > It is a bit unfortunate that we cannot make this into a test script, as it > depends on what is on the uninitialized part of the heap, which might > happen to be a NUL in which case the test would pass. > > Running tests under the valgrind mode may catch issues, though. Right, since reading the trailing NUL byte after the end of the buffer would be detected. > Thanks. Will queue with this test squashed in. Thanks for adding the test! ```