| Message ID | pull.820.git.1608398598893.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | negative-refspec: fix segfault on : refspec | expand |
"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
On Sat, Dec 19, 2020 at 10:05 AM Junio C Hamano <gitster@pobox.com> wrote: > > Original author of the feature (Jacob) cc'ed for insight. > Hi, Sorry I missed this thread last couple months. > - 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. > Isn't this patch about how we somehow broke ":" on its own, not as a negative 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" ' + ( + 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