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

> 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");

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

> > > 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?
> 
> I traced the 3 other callers to guess_remote_head() and none of them has a
> problem if peer_ref is NULL. In get_expanded_map() there is even a condition
> `if (cpy->peer_ref)`, which indicates peer_ref is allowed to be NULL.