negative-refspec: fix segfault on : refspec

From: Nipunn Koorapati <[email protected]>

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

---
negative-refspec: fix segfault on : refspec

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)

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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/820

 remote.c                          |  5 ++---
 t/t5582-fetch-negative-refspec.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

index 9f2450cb51b..8ab8d25294c 100644
@@ -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);
 		}
 	}
 

index 8c61e28fec8..4960378e0b7 100755
@@ -186,4 +186,14 @@ test_expect_success "fetch --prune with negative refspec" '
 	)
 '
 
+test_expect_success "push with empty 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

gitgitgadget

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.

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.

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)

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

Update since Patch series V1:

* Handled matching refspec explicitly
* Added testing for "+:" case
* Added comment explaining how the two loops work together

It may be wise to add additional testing for a case with a matching refspec
+ negative refspec with expected behavior

Nipunn Koorapati (2):
negative-refspec: fix segfault on : refspec
negative-refspec: improve comment on query_matches_negative_refspec

 remote.c                          | 16 +++++++++++++---
 t/t5582-fetch-negative-refspec.sh | 15 +++++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)

base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/820

Range-diff vs v1:

1: 743e848653f ! 1: e42200b644a negative-refspec: fix segfault on : refspec

     @@ Metadata
       ## Commit message ##
          negative-refspec: fix segfault on : refspec
      
     -    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)
     +    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 is never NULL, however when
     +    remote.origin.push is set to ":", then refspec->src is NULL,
     +    causing a segfault within strcmp
      
          Added testing for this case in fetch-negative-refspec
      
     @@ remote.c: static int query_matches_negative_refspec(struct refspec *rs, struct r
      -		} else {
      -			if (!strcmp(needle, refspec->src))
      -				string_list_append(&reversed, refspec->src);
     -+		} else if (refspec->src != NULL && !strcmp(needle, refspec->src)) {
     ++		} else if (refspec->matching) {
     ++			/* For the special matching refspec, any query should match */
     ++			string_list_append(&reversed, needle);
     ++		} else if (refspec->src == NULL) {
     ++			BUG("refspec->src should not be null here");
     ++		} else if (!strcmp(needle, refspec->src)) {
      +			string_list_append(&reversed, refspec->src);
       		}
       	}
     @@ t/t5582-fetch-negative-refspec.sh: test_expect_success "fetch --prune with negat
       	)
       '
       
     -+test_expect_success "push with empty refspec" '
     ++test_expect_success "push with matching ':' 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 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
      +	)
      +'

-: ----------- > 2: 8da8d9cd1c5 negative-refspec: improve comment on query_matches_negative_refspec

gitgitgadget

From: Nipunn Koorapati <[email protected]>

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 is never NULL, however when remote.origin.push is set to ":", then refspec->src is NULL, causing a segfault within strcmp

Added testing for this case in fetch-negative-refspec

---
 remote.c                          | 10 +++++++---
 t/t5582-fetch-negative-refspec.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

index 9f2450cb51b..cbb3113b105 100644
@@ -751,9 +751,13 @@ 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->matching) {
+			/* For the special matching refspec, any query should match */
+			string_list_append(&reversed, needle);
+		} else if (refspec->src == NULL) {
+			BUG("refspec->src should not be null here");
+		} else if (!strcmp(needle, refspec->src)) {
+			string_list_append(&reversed, refspec->src);
 		}
 	}
 

index 8c61e28fec8..58b42fabd97 100755
@@ -186,4 +186,19 @@ test_expect_success "fetch --prune with negative refspec" '
 	)
 '
 
+test_expect_success "push with matching ':' 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 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

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Comment did not adequately explain how the two loops work together to achieve the goal of querying for matching of any negative refspec.

---
 remote.c | 6 ++++++
 1 file changed, 6 insertions(+)

index cbb3113b105..6cdaa8da75a 100644
@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
 	 * item uses the destination. To handle this, we apply pattern
 	 * refspecs in reverse to figure out if the query source matches any
 	 * of the negative refspecs.
+	 *
+	 * The first loop finds and expands all positive refspecs
+	 * matched by the queried ref.
+	 *
+	 * The second loop checks if any of the results of the first loop
+	 * match any negative refspec.
 	 */
 	for (i = 0; i < rs->nr; i++) {
 		struct refspec_item *refspec = &rs->items[i];

gitgitgadget

A couple minor comments below...

    } else if (!refspec->src) {
        ...

    test_config remote.one.push : &&
    (
        cd two &&
        test_must_fail git push one master &&

        git config remote.one.push +: &&
        test_must_fail git push one master
    )

    for v in : +:
    do
        test_config -C two remote.one.push $v &&
        test_must_fail git -C two push one master || return 1
    done

From: Nipunn Koorapati <[email protected]>

Comment did not adequately explain how the two loops work together to achieve the goal of querying for matching of any negative refspec.

---
 remote.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 7323694b163..c3f85c17ca7 100644
@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
 	 * item uses the destination. To handle this, we apply pattern
 	 * refspecs in reverse to figure out if the query source matches any
 	 * of the negative refspecs.
+	 *
+	 * The first loop finds and expands all positive refspecs
+	 * matched by the queried ref.
+	 *
+	 * The second loop checks if any of the results of the first loop
+	 * match any negative refspec.
 	 */
 	for (i = 0; i < rs->nr; i++) {
 		struct refspec_item *refspec = &rs->items[i];

gitgitgadget

From: Nipunn Koorapati <[email protected]>

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 is never NULL, however when remote.origin.push is set to ":", then refspec->src is NULL, causing a segfault within strcmp

Tell git to handle matching refspec by adding the needle to the set of positively matched refspecs, since matching ":" refspecs match anything as src.

Added testing for matching refspec pushes fetch-negative-refspec both individually and in combination with a negative refspec

---
 remote.c                          | 10 +++++++---
 t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

index 9f2450cb51b..7323694b163 100644
@@ -751,9 +751,13 @@ 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->matching) {
+			/* For the special matching refspec, any query should match */
+			string_list_append(&reversed, needle);
+		} else if (!refspec->src) {
+			BUG("refspec->src should not be null here");
+		} else if (!strcmp(needle, refspec->src)) {
+			string_list_append(&reversed, refspec->src);
 		}
 	}
 

index 8c61e28fec8..30209e98a62 100755
@@ -186,4 +186,26 @@ test_expect_success "fetch --prune with negative refspec" '
 	)
 '
 
+test_expect_success "push with matching ':' refspec" '
+	test_config -C two remote.one.push : &&
+	# Fails w/ tip behind counterpart - but should not segfault
+	test_must_fail git -C two push one
+'
+
+test_expect_success "push with matching '+:' refspec" '
+	test_config -C two remote.one.push +: &&
+	# Fails w/ tip behind counterpart - but should not segfault
+	test_must_fail git -C two push one
+'
+
+test_expect_success "push with matching and negative refspec" '
+	test_config -C two --add remote.one.push : &&
+	# Fails to push master w/ tip behind counterpart
+	test_must_fail git -C two push one &&
+
+	# If master is in negative refspec, then the command will succeed
+	test_config -C two --add remote.one.push ^refs/heads/master &&
+	git -C two push one
+'
+
 test_done

gitgitgadget

If remote.origin.push was set to ":", git segfaults 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)

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

Update since Patch series V1:

* Handled matching refspec explicitly
* Added testing for "+:" case
* Added comment explaining how the two loops work together

Update since Patch series V2

* style suggestion in remote.c
* Use test_config
* Add test for a case with a matching refspec + negative refspec
* Fix test_config to work with --add
* Updated commit message to describe what git is told to do instead of segfaulting

Nipunn Koorapati (3):
test-lib-functions: handle --add in test_config
negative-refspec: fix segfault on : refspec
negative-refspec: improve comment on query_matches_negative_refspec

 remote.c                          | 16 +++++++++++++---
 t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++++++
 t/test-lib-functions.sh           |  9 ++++++++-
 3 files changed, 43 insertions(+), 4 deletions(-)

base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/820

Range-diff vs v2:

 -:  ----------- > 1:  733c674bd19 test-lib-functions: handle --add in test_config
 1:  e42200b644a ! 2:  20cff2f5c59 negative-refspec: fix segfault on : refspec
     @@ Commit message
          remote.origin.push is set to ":", then refspec->src is NULL,
          causing a segfault within strcmp
      
     -    Added testing for this case in fetch-negative-refspec
     +    Tell git to handle matching refspec by adding the needle to the
     +    set of positively matched refspecs, since matching ":" refspecs
     +    match anything as src.
     +
     +    Added testing for matching refspec pushes fetch-negative-refspec
     +    both individually and in combination with a negative refspec
      
          Signed-off-by: Nipunn Koorapati <[email protected]>
      
     @@ remote.c: static int query_matches_negative_refspec(struct refspec *rs, struct r
      +		} else if (refspec->matching) {
      +			/* For the special matching refspec, any query should match */
      +			string_list_append(&reversed, needle);
     -+		} else if (refspec->src == NULL) {
     ++		} else if (!refspec->src) {
      +			BUG("refspec->src should not be null here");
      +		} else if (!strcmp(needle, refspec->src)) {
      +			string_list_append(&reversed, refspec->src);
     @@ t/t5582-fetch-negative-refspec.sh: test_expect_success "fetch --prune with negat
       '
       
      +test_expect_success "push with matching ':' refspec" '
     -+	(
     -+		cd two &&
     -+		git config remote.one.push : &&
     -+		# Fails w/ tip behind counterpart - but should not segfault
     -+		test_must_fail git push one master &&
     ++	test_config -C two remote.one.push : &&
     ++	# Fails w/ tip behind counterpart - but should not segfault
     ++	test_must_fail git -C two push one
     ++'
     ++
     ++test_expect_success "push with matching '+:' refspec" '
     ++	test_config -C two remote.one.push +: &&
     ++	# Fails w/ tip behind counterpart - but should not segfault
     ++	test_must_fail git -C two push one
     ++'
      +
     -+		git config remote.one.push +: &&
     -+		# Fails w/ tip behind counterpart - but should not segfault
     -+		test_must_fail git push one master &&
     ++test_expect_success "push with matching and negative refspec" '
     ++	test_config -C two --add remote.one.push : &&
     ++	# Fails to push master w/ tip behind counterpart
     ++	test_must_fail git -C two push one &&
      +
     -+		git config --unset remote.one.push
     -+	)
     ++	# If master is in negative refspec, then the command will succeed
     ++	test_config -C two --add remote.one.push ^refs/heads/master &&
     ++	git -C two push one
      +'
      +
       test_done

2: 8da8d9cd1c5 = 3: 0fd4e9f7459 negative-refspec: improve comment on query_matches_negative_refspec

gitgitgadget

From: Nipunn Koorapati <[email protected]>

test_config fails to unset the configuration variable when using --add, as it tries to run git config --unset-all --add

Tell test_config to invoke test_unconfig with the arg $2 when the arg $1 is --add

---
 t/test-lib-functions.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index 999982fe4a9..1fdd7129d51 100644
@@ -381,6 +381,7 @@ test_unconfig () {
 		config_dir=$1
 		shift
 	fi
+	echo git ${config_dir:+-C "$config_dir"} config --unset-all "$@"
 	git ${config_dir:+-C "$config_dir"} config --unset-all "$@"
 	config_status=$?
 	case "$config_status" in

@@ -400,7 +401,13 @@ test_config () {
 		config_dir=$1
 		shift
 	fi
-	test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" &&
+
+	first_arg=$1
+	if test "$1" = --add; then
+		first_arg=$2
+	fi
+
+	test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'" &&
 	git ${config_dir:+-C "$config_dir"} config "$@"
 }
 

gitgitgadget

s/Added testing/Add test/

That would be my preference, too. Thanks for carefully and patiently reviewing.

I forgot to mention that it likely would be a good idea to at least mention in the commit message why test_config() is not being used for that particular case. Perhaps saying something along the lines of "one test handles config cleanup manually since test_config() is not prepared to take arbitrary options such as --add" -- or something along those lines -- would be sufficient. Alternatively, an in-code comment within the test explaining the open-coding might be more helpful to people reading the code in the future.

I found that since test_unconfig uses --unset-all, I can write a test as such

    test_config -C two remote.one.push +: &&
    test_must_fail git -C two push one &&
    git -C two config --add remote.one.push ^refs/heads/master &&
    git -C two push one

The unconfig of the test_config will --unset-all remote.one.push. I can use this technique and add a comment to that extent.

--Nipunn

Yes, you could do that, though it is somewhat subtle and increases cognitive load since the reader has to reason about it a bit more -- and perhaps study the internal implementation of test_config() -- to convince himself or herself that the different methods of setting configuration (test_config() vs. git config) used in the same test is intentional and works as intended.

The example presented earlier, on the other hand, in which cleanup is explicit via test_when_finished "test_unconfig ..." does not suffer from such increased cognitive load since it uses git config consistently to set configuration rather than a mix of git config and test_config(). This sort of consideration is important not just for reviewers, but for people who need to understand the code down the road. For this reason, I think I favor the version in which the cleanup is explicit. (But that's just my opinion...)

If remote.origin.push was set to ":", git segfaults 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)

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

Update since Patch series V1:

* Handled matching refspec explicitly
* Added testing for "+:" case
* Added comment explaining how the two loops work together

Update since Patch series V2

* style suggestion in remote.c
* Use test_config
* Add test for a case with a matching refspec + negative refspec
* Fix test_config to work with --add
* Updated commit message to describe what git is told to do instead of segfaulting

Update since Patch series V3

* Removed commit modifying test_config
* Remove segfault-related comments in test
* Consolidate the three tests to two tests (1st and 3rd test overlapped in functionality)
* Base the patch series on the maint branch - since the bug affects 2.29.2

Appreciate the reviews from Junio and Eric! Happy Holidays!

Nipunn Koorapati (2):
negative-refspec: fix segfault on : refspec
negative-refspec: improve comment on query_matches_negative_refspec

 remote.c                          | 16 +++++++++++++---
 t/t5582-fetch-negative-refspec.sh | 24 ++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 3 deletions(-)

base-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/820

Range-diff vs v3:

 1:  733c674bd19 < -:  ----------- test-lib-functions: handle --add in test_config
 2:  20cff2f5c59 ! 1:  e59ff29bdef negative-refspec: fix segfault on : refspec

@@ Commit message
(refspec: add support for negative refspecs, 2020-09-30) looks at refspec->src assuming it is never NULL, however when remote.origin.push is set to ":", then refspec->src is NULL,
- causing a segfault within strcmp
+ causing a segfault within strcmp.

Tell git to handle matching refspec by adding the needle to the set of positively matched refspecs, since matching ":" refspecs match anything as src.

- Added testing for matching refspec pushes fetch-negative-refspec
- both individually and in combination with a negative refspec
+ Add test for matching refspec pushes fetch-negative-refspec
+ both individually and in combination with a negative refspec.

@@ t/t5582-fetch-negative-refspec.sh: test_expect_success "fetch --prune with negat
)
'

-+test_expect_success "push with matching ':' refspec" '
++test_expect_success "push with matching : and negative refspec" '
+ test_config -C two remote.one.push : &&
-+ # Fails w/ tip behind counterpart - but should not segfault
-+ test_must_fail git -C two push one
-+'
++ # Fails to push master w/ tip behind counterpart
++ test_must_fail git -C two push one &&
+
-+test_expect_success "push with matching '+:' refspec" '
-+ test_config -C two remote.one.push +: &&
-+ # Fails w/ tip behind counterpart - but should not segfault
-+ test_must_fail git -C two push one
++ # If master is in negative refspec, then the command will not attempt
++ # to push and succeed.
++ # We do not need test_config here as we are updating remote.one.push
++ # again. The teardown of the first test_config will do --unset-all
++ git -C two config --add remote.one.push ^refs/heads/master &&
++ git -C two push one
+'
+
-+test_expect_success "push with matching and negative refspec" '
-+ test_config -C two --add remote.one.push : &&
++test_expect_success "push with matching +: and negative refspec" '
++ test_config -C two remote.one.push +: &&
+ # Fails to push master w/ tip behind counterpart
+ test_must_fail git -C two push one &&
+
-+ # If master is in negative refspec, then the command will succeed
-+ test_config -C two --add remote.one.push ^refs/heads/master &&
++ # If master is in negative refspec, then the command will not attempt
++ # to push and succeed
++ git -C two config --add remote.one.push ^refs/heads/master &&
+ git -C two push one
+'
+
3: 0fd4e9f7459 = 2: 20575407cc0 negative-refspec: improve comment on query_matches_negative_refspec

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Comment did not adequately explain how the two loops work together to achieve the goal of querying for matching of any negative refspec.

---
 remote.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 4f1a4099f1a..4d150a316ed 100644
@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
 	 * item uses the destination. To handle this, we apply pattern
 	 * refspecs in reverse to figure out if the query source matches any
 	 * of the negative refspecs.
+	 *
+	 * The first loop finds and expands all positive refspecs
+	 * matched by the queried ref.
+	 *
+	 * The second loop checks if any of the results of the first loop
+	 * match any negative refspec.
 	 */
 	for (i = 0; i < rs->nr; i++) {
 		struct refspec_item *refspec = &rs->items[i];

gitgitgadget

From: Nipunn Koorapati <[email protected]>

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 is never NULL, however when remote.origin.push is set to ":", then refspec->src is NULL, causing a segfault within strcmp.

Tell git to handle matching refspec by adding the needle to the set of positively matched refspecs, since matching ":" refspecs match anything as src.

Add test for matching refspec pushes fetch-negative-refspec both individually and in combination with a negative refspec.

---
 remote.c                          | 10 +++++++---
 t/t5582-fetch-negative-refspec.sh | 24 ++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

index 8be67f0892b..4f1a4099f1a 100644
@@ -751,9 +751,13 @@ 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->matching) {
+			/* For the special matching refspec, any query should match */
+			string_list_append(&reversed, needle);
+		} else if (!refspec->src) {
+			BUG("refspec->src should not be null here");
+		} else if (!strcmp(needle, refspec->src)) {
+			string_list_append(&reversed, refspec->src);
 		}
 	}
 

index 8c61e28fec8..a4960c586b1 100755
@@ -186,4 +186,28 @@ test_expect_success "fetch --prune with negative refspec" '
 	)
 '
 
+test_expect_success "push with matching : and negative refspec" '
+	test_config -C two remote.one.push : &&
+	# Fails to push master w/ tip behind counterpart
+	test_must_fail git -C two push one &&
+
+	# If master is in negative refspec, then the command will not attempt
+	# to push and succeed.
+	# We do not need test_config here as we are updating remote.one.push
+	# again. The teardown of the first test_config will do --unset-all
+	git -C two config --add remote.one.push ^refs/heads/master &&
+	git -C two push one
+'
+
+test_expect_success "push with matching +: and negative refspec" '
+	test_config -C two remote.one.push +: &&
+	# Fails to push master w/ tip behind counterpart
+	test_must_fail git -C two push one &&
+
+	# If master is in negative refspec, then the command will not attempt
+	# to push and succeed
+	git -C two config --add remote.one.push ^refs/heads/master &&
+	git -C two push one
+'
+
 test_done

gitgitgadget

Ideally, we should make sure that the next person who reads "git show" output of the commit that would result from the patch would not have to ask any of the "?" asked in the review above. Let me see if I can come up with a suggestion to get us closer to that goal.

	... goes and hacks ...

Perhaps squash the following into this step?

Thanks.

 t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

index a4960c586b..bed67cf92d 100755
@@ -187,8 +187,13 @@ test_expect_success "fetch --prune with negative refspec" '
 '
 
 test_expect_success "push with matching : and negative refspec" '
+	# Repositories two and one have branches other than master"
+	# but they have no overlap---"master" is the only one that
+	# is shared between them.  And the master branch at two is
+	# behind the master branch at one by one commit.
 	test_config -C two remote.one.push : &&
-	# Fails to push master w/ tip behind counterpart
+
+	# A matching push tries to update master, fails due to non-ff
 	test_must_fail git -C two push one &&
 
 	# If master is in negative refspec, then the command will not attempt

@@ -196,18 +201,27 @@ test_expect_success "push with matching : and negative refspec" '
 	# We do not need test_config here as we are updating remote.one.push
 	# again. The teardown of the first test_config will do --unset-all
 	git -C two config --add remote.one.push ^refs/heads/master &&
-	git -C two push one
+
+	# With "master" excluded, this push is a no-op.  Nothing gets
+	# pushed and it succeeds.
+	git -C two push -v one
 '
 
 test_expect_success "push with matching +: and negative refspec" '
+	# The same set-up as above, whose side-effect was a no-op.
 	test_config -C two remote.one.push +: &&
-	# Fails to push master w/ tip behind counterpart
+
+	# The push refuses to update the "master" branch that is checked
+	# out in the "one" repository, even when it is forced with +:
 	test_must_fail git -C two push one &&
 
 	# If master is in negative refspec, then the command will not attempt
 	# to push and succeed
 	git -C two config --add remote.one.push ^refs/heads/master &&
-	git -C two push one
+
+	# With "master" excluded, this push is a no-op.  Nothing gets
+	# pushed and it succeeds.
+	git -C two push -v one
 '
 
 test_done

Totally sympathize with wanting to reduce the cognitive load of reading the test suite.
As a reader of the test, I found both implementation choices nonobvious - and requiring
some diving into test_config and test_unconfig (namely the --unset-all behavior).
A comment on the git config stating that it's there because test_config doesn't yet support
--add seems equally clarifying as inserting a comment next to the test_unconfig usage.
That being said, I suspect anyone in the future poking around with this will have to sourcedive
through test_config and test_unconfig to make sense of it.

I'll switch it over to the test_unconfig option on the next reroll if requested.

--Nipunn

If remote.origin.push was set to ":", git segfaults 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)

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

Update since Patch series V1:

* Handled matching refspec explicitly
* Added testing for "+:" case
* Added comment explaining how the two loops work together

Update since Patch series V2

* style suggestion in remote.c
* Use test_config
* Add test for a case with a matching refspec + negative refspec
* Fix test_config to work with --add
* Updated commit message to describe what git is told to do instead of segfaulting

Update since Patch series V3

* Removed commit modifying test_config
* Remove segfault-related comments in test
* Consolidate the three tests to two tests (1st and 3rd test overlapped in functionality)
* Base the patch series on the maint branch - since the bug affects 2.29.2

Update since Patch series V4

* Squashed in Junio's patch to handle non-master named branches
* Explicitly use test_unconfig

Appreciate the reviews from Junio and Eric! Happy Holidays!

Nipunn Koorapati (2):
negative-refspec: fix segfault on : refspec
negative-refspec: improve comment on query_matches_negative_refspec

 remote.c                          | 16 ++++++++--
 t/t5582-fetch-negative-refspec.sh | 51 +++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 3 deletions(-)

base-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/820

Range-diff vs v4:

1: e59ff29bdef ! 1: 48c79dc3d84 negative-refspec: fix segfault on : refspec
@@ t/t5582-fetch-negative-refspec.sh: test_expect_success "fetch --prune with negat
'

+test_expect_success "push with matching : and negative refspec" '
-+ test_config -C two remote.one.push : &&
-+ # Fails to push master w/ tip behind counterpart
++ # Manually handle cleanup, since test_config is not
++ # prepared to take arbitrary options like --add
++ test_when_finished "test_unconfig -C two remote.one.push" &&
++
++ # For convenience, we use "master" to refer to the name of
++ # the branch created by default in the following.
++ #
++ # Repositories two and one have branches other than "master"
++ # but they have no overlap---"master" is the only one that
++ # is shared between them. And the master branch at two is
++ # behind the master branch at one by one commit.
++ git -C two config --add remote.one.push : &&
++
++ # A matching push tries to update master, fails due to non-ff
+ test_must_fail git -C two push one &&
+
++ # "master" may actually not be "master"---find it out.
++ current=$(git symbolic-ref HEAD) &&
++
+ # If master is in negative refspec, then the command will not attempt
+ # to push and succeed.
-+ # We do not need test_config here as we are updating remote.one.push
-+ # again. The teardown of the first test_config will do --unset-all
-+ git -C two config --add remote.one.push ^refs/heads/master &&
-+ git -C two push one
++ git -C two config --add remote.one.push "^$current" &&
++
++ # With "master" excluded, this push is a no-op. Nothing gets
++ # pushed and it succeeds.
++ git -C two push -v one
+'
+
+test_expect_success "push with matching +: and negative refspec" '
-+ test_config -C two remote.one.push +: &&
-+ # Fails to push master w/ tip behind counterpart
++ test_when_finished "test_unconfig -C two remote.one.push" &&
++
++ # The same set-up as above, whose side-effect was a no-op.
++ git -C two config --add remote.one.push +: &&
++
++ # The push refuses to update the "master" branch that is checked
++ # out in the "one" repository, even when it is forced with +:
+ test_must_fail git -C two push one &&
+
++ # "master" may actually not be "master"---find it out.
++ current=$(git symbolic-ref HEAD) &&
++
+ # If master is in negative refspec, then the command will not attempt
+ # to push and succeed
-+ git -C two config --add remote.one.push ^refs/heads/master &&
-+ git -C two push one
++ git -C two config --add remote.one.push "^$current" &&
++
++ # With "master" excluded, this push is a no-op. Nothing gets
++ # pushed and it succeeds.
++ git -C two push -v one
+'
+
test_done
2: 20575407cc0 = 2: 1f9af0e991c negative-refspec: improve comment on query_matches_negative_refspec

gitgitgadget

From: Nipunn Koorapati <[email protected]>

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 is never NULL, however when remote.origin.push is set to ":", then refspec->src is NULL, causing a segfault within strcmp.

Tell git to handle matching refspec by adding the needle to the set of positively matched refspecs, since matching ":" refspecs match anything as src.

Add test for matching refspec pushes fetch-negative-refspec both individually and in combination with a negative refspec.

---
 remote.c                          | 10 ++++--
 t/t5582-fetch-negative-refspec.sh | 51 +++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 3 deletions(-)

index 8be67f0892b..4f1a4099f1a 100644
@@ -751,9 +751,13 @@ 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->matching) {
+			/* For the special matching refspec, any query should match */
+			string_list_append(&reversed, needle);
+		} else if (!refspec->src) {
+			BUG("refspec->src should not be null here");
+		} else if (!strcmp(needle, refspec->src)) {
+			string_list_append(&reversed, refspec->src);
 		}
 	}
 

gitgitgadget

From: Nipunn Koorapati <[email protected]>

Comment did not adequately explain how the two loops work together to achieve the goal of querying for matching of any negative refspec.

---
 remote.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 4f1a4099f1a..4d150a316ed 100644
@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
 	 * item uses the destination. To handle this, we apply pattern
 	 * refspecs in reverse to figure out if the query source matches any
 	 * of the negative refspecs.
+	 *
+	 * The first loop finds and expands all positive refspecs
+	 * matched by the queried ref.
+	 *
+	 * The second loop checks if any of the results of the first loop
+	 * match any negative refspec.
 	 */
 	for (i = 0; i < rs->nr; i++) {
 		struct refspec_item *refspec = &rs->items[i];

gitgitgadget

Indeed, it's not entirely uncommon to have to dig into the test functions when writing tests. My bigger concern was someone coming along and thinking that the mixed use of test_config() and manual git config in the same test was a mistake, and want to fix that mistake, which would lead the person down the same rabbit hole. Explicit cleanup via test_unconfig() and consistent use of git config within the test, on the other hand, does not look accidental, so the reader would be less likely to want to "fix the mistake". The comment you added in the re-roll above the manual cleanup saves the reader the trouble of having to dive into the implementation of test_config(), which is a nice bonus.

Thanks, will replace. Happy holidays to you, too.

Is this something we want to merge into the 2.29 maint branch? It is a segfault for anyone pushing with matching refspecs on 2.29 At what point does git stop patching bugs in the maint branch?

--Nipunn

--Nipunn

Eventually by backporting, but a fix typically goes to the current development track first so it would happen after 2.30 is finished, I would think.

I wanted to bump this idea - now that it appears that 2.30 is complete and the new maint branch. Given that this patch makes matching-refspec unusable in 2.29, would it make sense to backport a fix to the 2.29 release? If that seems risky/unwanted, is there some practice of documenting known (serious) bugs in past releases?

Thanks
--Nipunn

Yes, it does make sense.

If we were to spend engineering effort to cut a 2.29.1, however, we'd better make sure not just this fix but all the other fixes relevant to the 2.29 track that are already well tested are included in it. We just issued 2.30 and not many people are using it to exercise a relatively new negative pathspec feature yet, so it probably is a good idea to spend a weeks or two to enumerate what other things we want to be in the 2.29 maintenance track.

I personally do not have time for doing that myself right now, but luckily it is something contributors like you can step in to help ;-)

Thanks.

Hi,

Sorry I missed this thread last couple months.

Isn't this patch about how we somehow broke ":" on its own, not as a negative refspec?

This seems like the right approach to me. Thanks for the fix, and the tests so we don't break it on accident again in the future.

belated, but....