[v2,0/2] negative-refspec: fix segfault on : refspec

Message

John Cai via GitGitGadget Dec. 19, 2020, 9:58 p.m. UTC

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