git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com>,
	Jacob Keller <jacob.keller@gmail.com>
Cc: git@vger.kernel.org, Nipunn Koorapati <nipunn1313@gmail.com>,
	Nipunn Koorapati <nipunn@dropbox.com>
Subject: Re: [PATCH] negative-refspec: fix segfault on : refspec
Date: Sat, 19 Dec 2020 10:05:14 -0800
Message-ID: <xmqqy2htoen9.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <pull.820.git.1608398598893.gitgitgadget@gmail.com> (Nipunn Koorapati via GitGitGadget's message of "Sat, 19 Dec 2020 17:23:18 +0000")

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

> 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.

> 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 :").

> +	(
> +		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

  reply	other threads:[~2020-12-19 18:07 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-19 17:23 Nipunn Koorapati via GitGitGadget
2020-12-19 18:05 ` Junio C Hamano [this message]
2021-02-19  9:28   ` Jacob Keller
2020-12-19 21:58 ` [PATCH v2 0/2] " Nipunn Koorapati via GitGitGadget
2020-12-19 21:58   ` [PATCH v2 1/2] " Nipunn Koorapati via GitGitGadget
2020-12-20  2:57     ` Eric Sunshine
2020-12-19 21:58   ` [PATCH v2 2/2] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-21  2:05   ` [PATCH v3 0/3] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-21  2:05     ` [PATCH v3 1/3] test-lib-functions: handle --add in test_config Nipunn Koorapati via GitGitGadget
2020-12-21  7:07       ` Eric Sunshine
2020-12-21 19:00         ` Junio C Hamano
2020-12-21 20:08           ` Eric Sunshine
2020-12-22  0:00             ` Nipunn Koorapati
2020-12-22  0:13               ` Eric Sunshine
2020-12-22  2:25                 ` Nipunn Koorapati
2020-12-22  5:19                   ` Eric Sunshine
2020-12-21  2:05     ` [PATCH v3 2/3] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-21  7:20       ` Eric Sunshine
2020-12-21  2:05     ` [PATCH v3 3/3] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-22  1:11     ` [PATCH v4 0/2] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-22  1:11       ` [PATCH v4 1/2] " Nipunn Koorapati via GitGitGadget
2020-12-22  2:08         ` Junio C Hamano
2020-12-22  2:28           ` Junio C Hamano
2020-12-22  1:11       ` [PATCH v4 2/2] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-22  3:58       ` [PATCH v5 0/2] negative-refspec: fix segfault on : refspec Nipunn Koorapati via GitGitGadget
2020-12-22  3:58         ` [PATCH v5 1/2] " Nipunn Koorapati via GitGitGadget
2021-02-19  9:32           ` Jacob Keller
2020-12-22  3:58         ` [PATCH v5 2/2] negative-refspec: improve comment on query_matches_negative_refspec Nipunn Koorapati via GitGitGadget
2020-12-22  6:48         ` [PATCH v5 0/2] negative-refspec: fix segfault on : refspec Junio C Hamano
2020-12-23 23:56           ` Nipunn Koorapati
2020-12-24  0:00             ` Junio C Hamano
2021-01-11 20:22               ` Nipunn Koorapati
2021-01-12  2:01                 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqy2htoen9.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jacob.keller@gmail.com \
    --cc=nipunn1313@gmail.com \
    --cc=nipunn@dropbox.com \
    --subject='Re: [PATCH] negative-refspec: fix segfault on : refspec' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git