Re: [PATCH] grep: report missing left operand of --and

>
> On Mon, Jun 28 2021, René Scharfe wrote:
>
>> Git grep allows combining two patterns with --and.  It checks and
>> reports if the second pattern is missing when compiling the expression.
>> A missing first pattern, however, is only reported later at match time.
>> Thus no error is returned if no matching is done, e.g. because no file
>> matches the also given pathspec.
>>
>> When that happens we get an expression tree with an GREP_NODE_AND node
>> and a NULL pointer to the missing left child.  free_pattern_expr()
>> tries to dereference it during the cleanup at the end, which result in
>> a segmentation fault.
>>
>> Fix this by verifying the presence of the left operand at expression
>> compilation time.
>>
>> Reported-by: Matthew Hughes <matthewhughes934@gmail.com>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> Whether the check in match_expr_eval() can now be turned into a BUG is
>> left as an exercise for the reader. ;-)
>>
>>  grep.c          |  2 ++
>>  t/t7810-grep.sh | 10 ++++++++++
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/grep.c b/grep.c
>> index 8f91af1cb0..7d0ea4e956 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -655,6 +655,8 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)
>>  	struct grep_expr *x, *y, *z;
>>
>>  	x = compile_pattern_not(list);
>> +	if (!x)
>> +		die("Not a valid grep expression");
>>  	p = *list;
>>  	if (p && p->token == GREP_AND) {
>>  		if (!p->next)
>> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
>> index 5830733f3d..c581239674 100755
>> --- a/t/t7810-grep.sh
>> +++ b/t/t7810-grep.sh
>> @@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>>  . ./test-lib.sh
>>
>> +test_invalid_grep_expression() {
>> +	params="$@" &&
>> +	test_expect_success "invalid expression: grep $params" '
>> +		test_must_fail git grep $params -- nonexisting
>> +	'
>> +}
>> +
>>  cat >hello.c <<EOF
>>  #include <assert.h>
>>  #include <stdio.h>
>> @@ -89,6 +96,9 @@ test_expect_success 'grep should not segfault with a bad input' '
>>  	test_must_fail git grep "("
>>  '
>>
>> +test_invalid_grep_expression -e A --and
>> +test_invalid_grep_expression --and -e A
>> +
>>  for H in HEAD ''
>>  do
>>  	case "$H" in
>
> This seems like an incomplete fix, for the exact same thing with --or we
> silently return 1, as we would if we exited early in free_pattern_expr
> on !x, which aside from the segfault I think we should probably make a
> habit in our own free()-like functions.
>
> Whatever we're doing about the --and segfault it seems like we should do
> the same under --or, no?

> Your first test also passes before your fix, it's only the latter that
> segfaults. The first one emits:
>
>     fatal: --and not followed by pattern expression
>
> So having that in a leading patch to indicate no behavior was changed
> would be better.