[PATCH] grep: do not segfault when -f is used

[PATCH] grep: do not segfault when -f is used

[Matt Kraai]

"git grep" would segfault if its -f option was used because it would
try to use an uninitialized strbuf, so initialize the strbuf.

Signed-off-by: Matt Kraai <kraai@ftbfs.org>
---
 builtin-grep.c  |    2 +-
 t/t7002-grep.sh |    4 ++++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index 761799d..1df25b0 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -631,7 +631,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset)
 	struct grep_opt *grep_opt = opt->value;
 	FILE *patterns;
 	int lno = 0;
-	struct strbuf sb;
+	struct strbuf sb = STRBUF_INIT;
 
 	patterns = fopen(arg, "r");
 	if (!patterns)
diff --git a/t/t7002-grep.sh b/t/t7002-grep.sh
index ae56a36..762f815 100755
--- a/t/t7002-grep.sh
+++ b/t/t7002-grep.sh
@@ -44,6 +44,10 @@ test_expect_success 'grep should not segfault with a bad input' '
 	test_must_fail git grep "("
 '
 
+test_expect_success 'grep should not segfault with -f' '
+        test_must_fail git grep -f /dev/null
+'
+
 for H in HEAD ''
 do
 	case "$H" in
-- 
1.6.5

Re: [PATCH] grep: do not segfault when -f is used

[Johannes Sixt]

Matt Kraai schrieb:
> "git grep" would segfault if its -f option was used because it would
> try to use an uninitialized strbuf, so initialize the strbuf.

Thanks for noticing and for the patch.

But...

> +test_expect_success 'grep should not segfault with -f' '
> +        test_must_fail git grep -f /dev/null
> +'

there must be a better way to test whether grep -f behaves correctly.

-- Hannes

Re: [PATCH] grep: do not segfault when -f is used

[Matt Kraai]

On Fri, Oct 16, 2009 at 12:34:23PM +0200, Johannes Sixt wrote:
> Matt Kraai schrieb:
> > +test_expect_success 'grep should not segfault with -f' '
> > +        test_must_fail git grep -f /dev/null
> > +'
> 
> there must be a better way to test whether grep -f behaves correctly.

How about the following test cases instead?

test_expect_success 'grep -f, non-existent file' '
	test_must_fail git grep -f patterns
'

cat >expected <<EOF
file:foo mmap bar
file:foo_mmap bar
file:foo_mmap bar mmap
file:foo mmap bar_mmap
file:foo_mmap bar mmap baz
EOF

cat >pattern <<EOF
mmap
EOF

test_expect_success 'grep -f, one pattern' '
	git grep -f pattern >actual &&
	test_cmp expected actual
'

cat >expected <<EOF
file:foo mmap bar
file:foo_mmap bar
file:foo_mmap bar mmap
file:foo mmap bar_mmap
file:foo_mmap bar mmap baz
t/a/v:vvv
t/v:vvv
v:vvv
EOF

cat >patterns <<EOF
mmap
vvv
EOF

test_expect_success 'grep -f, multiple patterns' '
	git grep -f patterns >actual &&
	test_cmp expected actual
'

cat >expected <<EOF
file:foo mmap bar
file:foo_mmap bar
file:foo_mmap bar mmap
file:foo mmap bar_mmap
file:foo_mmap bar mmap baz
t/a/v:vvv
t/v:vvv
v:vvv
EOF

cat >patterns <<EOF

mmap

vvv

EOF

test_expect_success 'grep -f, ignore empty lines' '
	git grep -f patterns >actual &&
	test_cmp expected actual
'

-- 
Matt Kraai                                           http://ftbfs.org/

Re: [PATCH] grep: do not segfault when -f is used

[Johannes Sixt]

Matt Kraai schrieb:
> On Fri, Oct 16, 2009 at 12:34:23PM +0200, Johannes Sixt wrote:
>> there must be a better way to test whether grep -f behaves correctly.
> 
> How about the following test cases instead?

*MUCH* better! Now, if you could wrap them up in a patch...

-- Hannes

[PATCH] grep: do not segfault when -f is used

[Matt Kraai]

"git grep" would segfault if its -f option was used because it would
try to use an uninitialized strbuf, so initialize the strbuf.

Thanks to Johannes Sixt <j.sixt@viscovery.net> for the help with the
test cases.

Signed-off-by: Matt Kraai <kraai@ftbfs.org>
---
 builtin-grep.c  |    2 +-
 t/t7002-grep.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index 761799d..1df25b0 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -631,7 +631,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset)
 	struct grep_opt *grep_opt = opt->value;
 	FILE *patterns;
 	int lno = 0;
-	struct strbuf sb;
+	struct strbuf sb = STRBUF_INIT;
 
 	patterns = fopen(arg, "r");
 	if (!patterns)
diff --git a/t/t7002-grep.sh b/t/t7002-grep.sh
index ae56a36..ae5290a 100755
--- a/t/t7002-grep.sh
+++ b/t/t7002-grep.sh
@@ -213,6 +213,72 @@ test_expect_success 'grep -e A --and --not -e B' '
 	test_cmp expected actual
 '
 
+test_expect_success 'grep -f, non-existent file' '
+	test_must_fail git grep -f patterns
+'
+
+cat >expected <<EOF
+file:foo mmap bar
+file:foo_mmap bar
+file:foo_mmap bar mmap
+file:foo mmap bar_mmap
+file:foo_mmap bar mmap baz
+EOF
+
+cat >pattern <<EOF
+mmap
+EOF
+
+test_expect_success 'grep -f, one pattern' '
+	git grep -f pattern >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
+file:foo mmap bar
+file:foo_mmap bar
+file:foo_mmap bar mmap
+file:foo mmap bar_mmap
+file:foo_mmap bar mmap baz
+t/a/v:vvv
+t/v:vvv
+v:vvv
+EOF
+
+cat >patterns <<EOF
+mmap
+vvv
+EOF
+
+test_expect_success 'grep -f, multiple patterns' '
+	git grep -f patterns >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
+file:foo mmap bar
+file:foo_mmap bar
+file:foo_mmap bar mmap
+file:foo mmap bar_mmap
+file:foo_mmap bar mmap baz
+t/a/v:vvv
+t/v:vvv
+v:vvv
+EOF
+
+cat >patterns <<EOF
+
+mmap
+
+vvv
+
+EOF
+
+test_expect_success 'grep -f, ignore empty lines' '
+	git grep -f patterns >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<EOF
 y:y yy
 --
-- 
1.6.5

Re: [PATCH] grep: do not segfault when -f is used

[Junio C Hamano]

Matt Kraai <kraai@ftbfs.org> writes:

> "git grep" would segfault if its -f option was used because it would
> try to use an uninitialized strbuf, so initialize the strbuf.
>
> Thanks to Johannes Sixt <j.sixt@viscovery.net> for the help with the
> test cases.
>
> Signed-off-by: Matt Kraai <kraai@ftbfs.org>

Thanks, both of you.