Re: [PATCH resend] builtin/clone: fix segfault when using --revision on some servers

Adrian Friedli <[email protected]> writes:

> Fix a segfault when a server advertises more refs than requested when
> using the --revision argument.
>
> Signed-off-by: Adrian Friedli <[email protected]>
> ---
> The segfault can be reproduced by e.g.
>
> git clone --revision=refs/heads/main \
> https://dev.azure.com/public-git/sample/_git/sample

The following two paragraphs' worth of explanation deserves to be in
the log message:

> In the good case the server respects
> `transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked
> list `refs` returned by `transport_get_remote_refs()` only contains a
> single item, which is the ref requested with the --revision argument.
> Both `remote_head` returned by `find_ref_by_name()` and
> `remote_head_points_at` returned by `guess_remote_head()` are NULL. The
> guard in `update_remote_refs()` skips a the affected code because
> `remote_head_points_at` is NULL.
>
> In the bad case the server ignores
> `transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked
> list `refs` returned by `transport_get_remote_refs()` contains many
> items, amongst others "HEAD". `remote_head` returned by
> `find_ref_by_name()` is not NULL and `remote_head_points_at` returned by
> `guess_remote_head()` is not NULL but its field `peer_ref` is NULL.
> Because `remote_head_points_at` is not NULL the guard in
> `update_remote_refs()` does not skip the affected code and
> `remote_head_points_at->peer_ref->name` is accessed, which causes a
> segfault later on.

Usually, our commit log message begins with an observation of the
current behavior.  We would probably start the log message like
this:

    Servers are expected to refrain from advertising excess refs,
    honoring transport_ls_refs_options.ref_prefixes, when

      $ git clone --revision=refs/heads/main $URL

    contacts them, but when talking to a server that does not (e.g.,
    <<the URL of the problematic repository goes here>>), the client
    segfaults.

and the above two paragraphs would flow perfectly after such an
introduction.  They clearly explain how the client gets confused by
unusual server behavior.

The above write-up makes me wonder if there is a valid case where
guess_remote_head() should return a non-NULL 'struct ref *' whose
'.peer_ref' member is NULL.  Unless a non-NULL head that is a symref
is given, in which case we firmly know where their 'HEAD' points,
the function seems to pick a randomly guessed ref out of the given
list of refs (supplied to its second parameter) and return a copy of
it.  However, there does not seem to be any check to ensure that it
picks a ref with its '.peer_ref' member set.  This may break other
code paths that consume the value returned from guess_remote_head()
in the exact same way, no?

I do not know offhand if that is the case, but if it is always wrong
for guess_remote_head() to return a guessed ref with NULL in its
'.peer_ref' member, perhaps that would be a better location to make
this fix.  What do you think?

In any case, can we also add a test to prevent this fix from
regressing in the future?

Thanks.


>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 9d08cd8722..bd0c6f5d56 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -557,7 +557,7 @@ static void update_remote_refs(const struct ref *refs,
>  			write_followtags(refs, msg);
>  	}
>  
> -	if (remote_head_points_at && !option_bare) {
> +	if (remote_head_points_at && remote_head_points_at->peer_ref && !option_bare) {
>  		struct strbuf head_ref = STRBUF_INIT;
>  		strbuf_addstr(&head_ref, branch_top);
>  		strbuf_addstr(&head_ref, "HEAD");