grep.c: remove "extended" in favor of "pattern_expression", fix segfault

Since 79d3696cfb4 (git-grep: boolean expression on pattern matching.,
2006-06-30) the "pattern_expression" member has been used for complex
queries (AND/OR...), with "pattern_list" being used for the simple OR
queries. Since then we've used both "pattern_expression" and its
associated boolean "extended" member to see if we have a complex
expression.

Since f41fb662f57 (revisions API: have release_revisions() release
"grep_filter", 2022-04-13) we've had a subtle bug relating to that: If
we supplied options that were only used for "complex queries", but
didn't supply the query itself we'd set "opt->extended", but would
have a NULL "pattern_expression". As a result these would segfault as
we tried to call "free_grep_patterns()" from "release_revisions()":

	git -P log -1 --invert-grep
	git -P log -1 --all-match

The root cause of this is that we were conflating the state management
we needed in "compile_grep_patterns()" itself with whether or not we
had an "opt->pattern_expression" later on.

In this cases as we're going through "compile_grep_patterns()" we have
no "opt->pattern_list" but have "opt->no_body_match" or
"opt->all_match". So we'd set "opt->extended = 1", but not "return" on
"opt->extended" as that's an "else if" in the same "if" statement.

That behavior is intentional and required, as the common case is that
we have an "opt->pattern_list" that we're about to parse into the
"opt->pattern_expression".

But we don't need to keep track of this "extended" flag beyond the
state management in compile_grep_patterns() itself. It needs it, but
once we're out of that function we can rely on
"opt->pattern_expression" being non-NULL instead for using these
extended patterns.

As 79d3696cfb4 itself shows we've assumed that there's a one-to-one
mapping between the two since the very beginning. I.e. "match_line()"
would check "opt->extended" to see if it should call "match_expr()",
and the first thing we do in that function is assume that we have a
"opt->pattern_expression". We'd then call "match_expr_eval()", which
would have died if that "opt->pattern_expression" was NULL.

The "die" was added in c922b01f54c (grep: fix segfault when "git grep
'('" is given, 2009-04-27), and can now be removed as it's now clearly
unreachable. We still do the right thing in the case that prompted
that fix:

	git grep '('
	fatal: unmatched parenthesis

Arguably neither the "--invert-grep" option added in [1] nor the
earlier "--all-match" option added in [2] were intended to be used
stand-alone, and another approach[3] would be to error out in those
cases. But since we've been treating them as a NOOP when given without
--grep for a long time let's keep doing that.

We could also return in "free_pattern_expr()" if the argument is
non-NULL, as an alternative fix for this segfault does [4]. That would
be more elegant in making the "free_*()" function behave like
"free()", but it would also remove a sanity check: The
"free_pattern_expr()" function calls itself recursively, and only the
top-level is allowed to be NULL, let's not conflate those two
conditions.

1. 22dfa8a23de (log: teach --invert-grep option, 2015-01-12)
2. 0ab7befa31d (grep --all-match, 2006-09-27)
3. https://lore.kernel.org/git/patch-1.1-f4b90799fce-20221010T165711Z-avarab@gmail.com/
4. http://lore.kernel.org/git/7e094882c2a71894416089f894557a9eae07e8f8.1665423686.git.me@ttaylorr.com

Reported-by: orygaw <orygaw@protonmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/grep.c b/grep.c
index 82eb7da1022ecc0c08a83ca7bc46e18602ff275b..27e1db0fe1ec8de26bbb0b3b7e20463fd1f3e9ce 100644 (file)
--- a/grep.c
+++ b/grep.c
@@ -708,6 +708,7 @@ void compile_grep_patterns(struct grep_opt *opt)
 {
        struct grep_pat *p;
        struct grep_expr *header_expr = prep_header_patterns(opt);
+       int extended = 0;
 
        for (p = opt->pattern_list; p; p = p->next) {
                switch (p->token) {
@@ -717,14 +718,14 @@ void compile_grep_patterns(struct grep_opt *opt)
                        compile_regexp(p, opt);
                        break;
                default:
-                       opt->extended = 1;
+                       extended = 1;
                        break;
                }
        }
 
        if (opt->all_match || opt->no_body_match || header_expr)
-               opt->extended = 1;
-       else if (!opt->extended)
+               extended = 1;
+       else if (!extended)
                return;
 
        p = opt->pattern_list;
@@ -790,7 +791,7 @@ void free_grep_patterns(struct grep_opt *opt)
                free(p);
        }
 
-       if (!opt->extended)
+       if (!opt->pattern_expression)
                return;
        free_pattern_expr(opt->pattern_expression);
 }
@@ -971,8 +972,6 @@ static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,
 {
        int h = 0;
 
-       if (!x)
-               die("Not a valid grep expression");
        switch (x->node) {
        case GREP_NODE_TRUE:
                h = 1;
@@ -1052,7 +1051,7 @@ static int match_line(struct grep_opt *opt,
        struct grep_pat *p;
        int hit = 0;
 
-       if (opt->extended)
+       if (opt->pattern_expression)
                return match_expr(opt, bol, eol, ctx, col, icol,
                                  collect_hits);
 
@@ -1370,7 +1369,7 @@ static int should_lookahead(struct grep_opt *opt)
 {
        struct grep_pat *p;
 
-       if (opt->extended)
+       if (opt->pattern_expression)
                return 0; /* punt for too complex stuff */
        if (opt->invert)
                return 0;

diff --git a/grep.h b/grep.h
index c722d25ed9d272fd333162a64ba367bc19ff77da..1cf9fee67e43dcf2d53833512d1a4df0f95c8ffd 100644 (file)
--- a/grep.h
+++ b/grep.h
@@ -151,7 +151,6 @@ struct grep_opt {
 #define GREP_BINARY_TEXT       2
        int binary;
        int allow_textconv;
-       int extended;
        int use_reflog_filter;
        int relative;
        int pathname;

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 6e66352558212e9a40404ef892a0944ab3d09db3..1a13921fa48450c3e95247e889e884805a699210 100755 (executable)
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -249,6 +249,15 @@ test_expect_success 'log --grep' '
        test_cmp expect actual
 '
 
+for noop_opt in --invert-grep --all-match
+do
+       test_expect_success "log $noop_opt without --grep is a NOOP" '
+               git log >expect &&
+               git log $noop_opt >actual &&
+               test_cmp expect actual
+       '
+done
+
 cat > expect << EOF
 second
 initial