threads / patch / 27418

patchdo not read beyond end of malloc'd buffer

Subject: [PATCH] do not read beyond end of malloc'd buffer

## tl;dr

3 messages between May 20, 2011 and May 21, 2011. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Jim Meyering· May 20, 2011, 17:20 UTC · lore

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 <meyering@redhat.com>
---
 diff.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Show changes to diff.c +7 −3
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· May 20, 2011, 18:41 UTC · re: Jim Meyering · lore

Re: [PATCH] do not read beyond end of malloc'd buffer

Jim Meyering <jim@meyering.net> 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?

Show 10 quoted lines
>     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.
Show changes to t/t4034-diff-words.sh +26 −0
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· May 21, 2011, 09:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] do not read beyond end of malloc'd buffer

Junio C Hamano wrote:
Show 11 quoted lines
> Jim Meyering <jim@meyering.net> 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.

...
Show 5 quoted lines
> 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!

← back to recent threads