[v3,2/3] negative-refspec: fix segfault on : refspec

Commit Message

Nipunn Koorapati Dec. 21, 2020, 2:05 a.m. UTC

From: Nipunn Koorapati <[e-mail]>

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

Signed-off-by: Nipunn Koorapati <[e-mail]>
---
 remote.c                          | 10 +++++++---
 t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

Comments

Eric Sunshine Dec. 21, 2020, 7:20 a.m. UTC | #1

On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget
<[e-mail]> wrote:
> 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

s/Added testing/Add test/

> both individually and in combination with a negative refspec
>
> Signed-off-by: Nipunn Koorapati <[e-mail]>
> ---
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> @@ -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
> +'

Nit: It is understood implicitly that Git should not segfault (or
indeed any software). That's also implied by use of test_must_fail()
which explicitly distinguishes expected failures from unexpected
failures (where segfault falls in the category of unexpected failure).
Therefore, it doesn't really add value to say "but should not
segfault" in the comment.

Same observation applies to the other similarly-worded comments in
this patch. Not alone worth a re-roll, but perhaps worth changing if
you do re-roll.

Patch

diff --git a/remote.c b/remote.c
index 9f2450cb51b..7323694b163 100644
--- a/remote.c
+++ b/remote.c
@@ -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);
 		}
 	}
 
diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
index 8c61e28fec8..30209e98a62 100755
--- a/t/t5582-fetch-negative-refspec.sh
+++ b/t/t5582-fetch-negative-refspec.sh
@@ -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