threads / patch / 21258

patchgrep: do not segfault when -f is used

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

## tl;dr

6 messages between Oct 16, 2009 and Oct 17, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Matt Kraai· Oct 16, 2009, 08:53 UTC · lore

"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(-)
Show changes to 2 files +5 −1

builtin-grep.c, t/t7002-grep.sh

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
Johannes Sixt· Oct 16, 2009, 10:34 UTC · re: Matt Kraai · lore

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

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
Matt Kraai· Oct 16, 2009, 13:39 UTC · re: Johannes Sixt · lore

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

On Fri, Oct 16, 2009 at 12:34:23PM +0200, Johannes Sixt wrote:
Show 6 quoted lines
> 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/
Johannes Sixt· Oct 16, 2009, 13:46 UTC · re: Matt Kraai · lore

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

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
Matt Kraai· Oct 16, 2009, 14:13 UTC · re: Johannes Sixt · lore

"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(-)
Show changes to 2 files +67 −1

builtin-grep.c, t/t7002-grep.sh

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
Junio C Hamano· Oct 17, 2009, 07:44 UTC · re: Matt Kraai · lore

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

Matt Kraai <kraai@ftbfs.org> writes:
Show 7 quoted lines
> "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.

← back to recent threads