* [PATCH] do not read beyond end of malloc'd buffer
@ 2011-05-20 17:20 Jim Meyering
2011-05-20 18:41 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: Jim Meyering @ 2011-05-20 17:20 UTC (permalink / raw)
To: git list
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] do not read beyond end of malloc'd buffer
2011-05-20 17:20 [PATCH] do not read beyond end of malloc'd buffer Jim Meyering
@ 2011-05-20 18:41 ` Junio C Hamano
2011-05-21 9:42 ` Jim Meyering
0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2011-05-20 18:41 UTC (permalink / raw)
To: Jim Meyering; +Cc: git list
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?
> 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] do not read beyond end of malloc'd buffer
2011-05-20 18:41 ` Junio C Hamano
@ 2011-05-21 9:42 ` Jim Meyering
0 siblings, 0 replies; 3+ messages in thread
From: Jim Meyering @ 2011-05-21 9:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git list
Junio C Hamano wrote:
> 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.
...
> 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!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-05-21 9:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-20 17:20 [PATCH] do not read beyond end of malloc'd buffer Jim Meyering
2011-05-20 18:41 ` Junio C Hamano
2011-05-21 9:42 ` Jim Meyering
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.