Re: [PATCH] negative-refspec: fix segfault on : refspec

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 19, 2020, 18:05 UTC
Message-ID: <xmqqy2htoen9.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.820.git.1608398598893.gitgitgadget@gmail.com>

"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 9 quoted lines

> From: Nipunn Koorapati <nipunn@dropbox.com>
>
> Previously, if remote.origin.push was set to ":",
> git would segfault during a push operation, due to bad
> parsing logic in query_matches_negative_refspec. Per
> bisect, the bug was introduced in:
> c0192df630 (refspec: add support for negative refspecs, 2020-09-30)
>
> Added testing for this case in fetch-negative-refspec

Thanks.

Our local convention in this project is to write about the status-quo without the patch under discussion in the present tense, and describe the fix as if we are giving orders to the codebase to become like so (or giving orders to the monkeys sitting in front of the keyboard to update the code). I'd explain the "problem description" part of the above perhaps like so:

	The logic added to check for negative pathspec match by
	c0192df630 (refspec: add support for negative refspecs,
	2020-09-30) looks at refspec->src assuming it never is NULL,
	but when remote.origin.push is set to ":" (i.e. "matching"),
	refspec->src is NULL, causing a segfauilt.
	
But stepping back a bit, a "matching" push is saying "if we have
branch 'hello', and they also have branch 'hello', push ours to
theirs".  So if the query is asking about 'hello' (e.g. needle is
'hello'), shouldn't a refspec ":" have the same effect as a refspec
"hello:hello", instead of getting ignored like this patch does?

Original author of the feature (Jacob) cc'ed for insight.

 - Can we have refspec->src==NULL in cases other than where
   refspec->matching is true?  If not, then perhaps the patch should
   insert, before the problematic "else if" clause, something like

		if (match_name_with_pattern(...))
			string_list_append_nodup(...);
   +	} else if (refspec->matching) {
   +		... behaviour for the matching case ...
   +	} else if (refspec->src == NULL) {
   +		BUG("refspec->src cannot be null here");
	} else {
		if (!strcmp(needle, refspec->src))

 - We'd need to decide if ignoring is the right behaviour for the
   matching refspec.  I do not recall what we decided the logic of
   the function should be offhand.

>     We found this issue when rolling out git 2.29 at Dropbox - as several
>     folks had "push = :" in their configuration. I based my diff off the
>     master branch, but also confirmed that it patches cleanly onto maint -
>     if the maintainers would like to also fix the segfault on 2.29

Yes, it is very much appreciated you were considerate to base the patch on the maintenance track. We want the code to do with the right thing with ":" matching refspec.

Show 25 quoted lines

> diff --git a/remote.c b/remote.c
> index 9f2450cb51b..8ab8d25294c 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -751,9 +751,8 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
>  
>  			if (match_name_with_pattern(key, needle, value, &expn_name))
>  				string_list_append_nodup(&reversed, expn_name);
> -		} else {
> -			if (!strcmp(needle, refspec->src))
> -				string_list_append(&reversed, refspec->src);
> +		} else if (refspec->src != NULL && !strcmp(needle, refspec->src)) {
> +			string_list_append(&reversed, refspec->src);
>  		}
>  	}
>  
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> index 8c61e28fec8..4960378e0b7 100755
> --- a/t/t5582-fetch-negative-refspec.sh
> +++ b/t/t5582-fetch-negative-refspec.sh
> @@ -186,4 +186,14 @@ test_expect_success "fetch --prune with negative refspec" '
>  	)
>  '
>  
> +test_expect_success "push with empty refspec" '

s/empty/matching/ (see "git push --help" and look for "The special refspec :").

Show 12 quoted lines

> +	(
> +		cd two &&
> +		git config remote.one.push : &&
> +		# Fails w/ tip behind counterpart - but should not segfault
> +		test_must_fail git push one master &&
> +		git config --unset remote.one.push
> +	)
> +'
> +
>  test_done
>
> base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707