Re: [PATCH v2 1/2] negative-refspec: fix segfault on : refspec

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 20, 2020, 02:57 UTC
Message-ID: <CAPig+cTBn6fPgkjaf=fYXi4XrHqc5Kf-ZJiMhxvdCDsMBuLTDQ@mail.gmail.com>
In-Reply-To: <e42200b644adab3ad78bf23e0258466287dbae70.1608415117.git.gitgitgadget@gmail.com>

On Sat, Dec 19, 2020 at 5:00 PM Nipunn Koorapati via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 7 quoted lines

> 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

A couple minor comments below...

Show 9 quoted lines

> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/remote.c b/remote.c
> @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
> +               } 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");

I realize that you copied Junio's example, but style on this project is to write this as:

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

Show 16 quoted lines

> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> @@ -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
> +       )
> +'

If anything in this test fails prior to the final `git config --unset`, then that cleanup command won't be executed, which might negatively impact tests which follow. To ensure cleanup whether the test succeeds or fails, use test_config(). Unfortunately, test_config() has the limitation that it can't be used in subshells, so you may have to restructure the test a bit, perhaps like this:

    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
    )

Driving the test with a for-loop and taking advantage of -C to avoid the subshell is also an option:

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