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

[Adrian Friedli <adrian.friedli@mt.com>]

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

The details of the 4 callers to guess_remote_head()

builtin/fetch.c:set_head()
builtin/remote.c:get_head_names()

  refs from guess_remote_head() are only used with ref->name

builtin/clone.c:wanted_peer_refs()

  Does not use REMOTE_GUESS_HEAD_ALL flag in guess_remote_head() call. So only one ref is returned.

  calls get_fetch_map() with refs from guess_remote_head().
    may call get_expanded_map()
      safe if peer_ref is NULL, has a guard on the copy of ref `if (cpy->peer_ref)`.
    may call get_remote_ref()
      calls get_remote_ref()
        calls find_ref_by_name_abbrev()
	  only looks at name and next

builtin/clone.c:cmd_clone()

  Does not use REMOTE_GUESS_HEAD_ALL flag in guess_remote_head() call. So only one ref is returned.

  may call write_refspec_config() with refs from guess_remote_head()
    only uses name from refs

  calls update_remote_refs() with refs from guess_remote_head() 
  -> thats the failing one, no other uses

  may call update_head() with refs from guess_remote_head()
    only uses name and old_oid from refs

Kind regards
Adrian