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

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

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.

 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

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.

Junio C Hamano <gitster@pobox.com> writes:

Never mind, scratch that part. If we are fetching without storing the result in any remote-tracking ref, '.peer_ref' is legitimately NULL, and if we are storing, '.peer_ref' names the local ref where we store the result. This should not affect our guess as to which of their branches may be pointed to by their 'HEAD'.

So this patch fixes the issue in the right place. It would still be nice to have a new test to prevent future regressions, though.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Thanks for your review.

While implementing the test I discovered it is protocol version 0 which triggers that behavior. I updated the log message and implemented a test.

Kind regards, Adrian

Servers supporting protocol v2 do not advertise excess refs and honor `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 support protocol v2 the client segfaults. This can also be observed when v0 is enforced for example by

    $ git -c protocol.version=0 clone --revision=refs/heads/main $URL

In the protocol v2 case the server honors `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 protocol v0 case 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.

Signed-off-by: Adrian Friedli <adrian.friedli@mt.com>
---
 builtin/clone.c           | 2 +-
 t/t5621-clone-revision.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

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

Oh, the proposed log message reads wonderfully. Thanks for digging down to the root cause of the issue.