Re: [PATCH v3 2/4] remote: use remote_state parameter internally

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2021, 20:31 UTC
Message-ID: <xmqqtuhbo2tn.fsf@gitster.g>
In-Reply-To: <xmqqzgr3o4yw.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> This made my "git push" to k.org and other places over ssh segfault
> when their tip and what I am attempting to push are identical.  I
> haven't spent more time than just to bisect the history down to
> identify this commit as the possible culprit.

(gdb) bt
#0  0x000055555579a785 in pushremote_for_branch (branch=0x0, explicit=0x7fffffffcf84)
    at remote.c:564
#1  0x000055555579a5c2 in remotes_remote_get_1 (remote_state=0x5555559782a0, name=0x0,
    get_default=0x55555579a742 <pushremote_for_branch>) at remote.c:518
#2  0x000055555579a6d0 in remotes_pushremote_get (remote_state=0x5555559782a0, name=0x0)
    at remote.c:542
#3  0x000055555579a740 in repo_pushremote_get (repo=0x555555974b80 <the_repo>, name=0x0)
    at remote.c:554
#4  0x000055555560aa9d in pushremote_get (name=0x0) at ./remote.h:135
#5  0x000055555560c5ce in cmd_push (argc=0, argv=0x7fffffffdc70, prefix=0x0)
    at builtin/push.c:611
#6  0x000055555557396a in run_builtin (p=0x555555941f78 <commands+2136>, argc=3,
    argv=0x7fffffffdc70) at git.c:461
#7  0x0000555555573d79 in handle_builtin (argc=3, argv=0x7fffffffdc70) at git.c:713
#8  0x0000555555573fe6 in run_argv (argcp=0x7fffffffdafc, argv=0x7fffffffdaf0) at git.c:780
#9  0x000055555557448f in cmd_main (argc=3, argv=0x7fffffffdc70) at git.c:911
#10 0x000055555565b2ae in main (argc=6, argv=0x7fffffffdc58) at common-main.c:52

The direct culprit is this part:

    const char *pushremote_for_branch(struct branch *branch, int *explicit)
    {
            if (branch && branch->pushremote_name) {
                    if (explicit)
                            *explicit = 1;
                    return branch->pushremote_name;
            }
            if (branch->remote_state->pushremote_name) {

where the second if() statement used to check "pushremote_name", but now unconditionally dereferences "branch".

The caller is remote_get_1(); this funciton was called with "current_branch", which can be NULL until you have a repository and you've called read_config(), but otherwise shouldn't be.

I think somebody is not setting up the remote_state correctly? When the user wants to just use the repository-wide pushremote, not having the current_branch would not matter, but if the pushremote for the current branch is different from the repository-wide one, the code would silently push to a wrong remote.

In any case, any public facing entry point, like pushremote_get() that is directly called from cmd_push() with just a name, should auto vivify an instance of struct remote_state and populate its members as needed, I think, and in this particular case, I suspect that it forgets to initialize the current_branch and other members by calling read_config(), just like other entry points like repo_remote_get() do.