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

Adrian Friedli [off-list ref] writes:

Fix a segfault when a server advertises more refs than requested when
using the --revision argument.

Signed-off-by: Adrian Friedli <redacted>
---
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.