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

[Junio C Hamano <gitster@pobox.com>]

Adrian Friedli <adrian.friedli@mt.com> writes:

> 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.

The description makes it sound more like this code is perfectly
fine, and the problem is in guess_remote_head() that reads the refs
list and includes such a bogus thing with no peer_ref in the result
of its guessing.  There are 4 direct callers to guess_remote_head()
including this one---wouldn't they also obtain a list with such a
ref entry?

Or is guess_remote_head() correct in that some uses of its result do
not mind such a ref with no peer_ref, but only this code path wants
to see a ref with peer_ref?  If that is the case, then shouldn't the
code this patch touches be looping over remote_head_points_at to see
if there is one with a peer_ref and use that?  The original is
assuming that remote_head_points_at that is not NULL has a valid and
usable entry at the beginning of the list, but if that assumption
does not hold and we are getting multiple hits, wouldn't it be
possible that a good entry is hidden behind a bad one in the list of
refs?

Thanks.

> Extend the guard in `update_remote_refs()` to also skip the block of
> code if `remote_head_points_at->peer_ref` is NULL.
>
> Signed-off-by: Adrian Friedli <adrian.friedli@mt.com>
> ---
> The segfault can be reproduced by e.g.
>
> git clone --revision=refs/heads/main \
> https://dev.azure.com/public-git/sample/_git/sample
>
>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index fba3c9c508..09219791da 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");