[PATCH v2] builtin/clone: fix segfault when using --revision with protocol v0

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 <[email protected]>
---
 builtin/clone.c           | 2 +-
 t/t5621-clone-revision.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 9d08cd8722..bd0c6f5d56 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");
diff --git a/t/t5621-clone-revision.sh b/t/t5621-clone-revision.sh
index db3b8cff55..54789423f8 100755
--- a/t/t5621-clone-revision.sh
+++ b/t/t5621-clone-revision.sh
@@ -90,6 +90,14 @@ test_expect_success 'clone with --revision and --bare' '
 	test_must_fail git -C dst config remote.origin.fetch
 '
 
+test_expect_success 'clone with --revision and protocol v0' '
+	test_when_finished "rm -rf dst" &&
+	git -c protocol.version=0 clone --no-local --revision=refs/heads/main . dst &&
+	git rev-parse refs/heads/main >expect &&
+	git -C dst rev-parse HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'clone with --revision being a short raw commit hash' '
 	test_when_finished "rm -rf dst" &&
 	oid=$(git rev-parse --short refs/heads/feature) &&

Range-diff against v1:
1:  f300b7b968 ! 1:  1a7cd98b7a builtin/clone: fix segfault when using --revision on some servers
    @@ Metadata
     Author: Adrian Friedli <[email protected]>
     
      ## Commit message ##
    -    builtin/clone: fix segfault when using --revision on some servers
    +    builtin/clone: fix segfault when using --revision with protocol v0
     
    -    Fix a segfault when a server advertises more refs than requested when
    -    using the --revision argument.
    +    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 <[email protected]>
     
    @@ builtin/clone.c: static void update_remote_refs(const struct ref *refs,
      		struct strbuf head_ref = STRBUF_INIT;
      		strbuf_addstr(&head_ref, branch_top);
      		strbuf_addstr(&head_ref, "HEAD");
    +
    + ## t/t5621-clone-revision.sh ##
    +@@ t/t5621-clone-revision.sh: test_expect_success 'clone with --revision and --bare' '
    + 	test_must_fail git -C dst config remote.origin.fetch
    + '
    + 
    ++test_expect_success 'clone with --revision and protocol v0' '
    ++	test_when_finished "rm -rf dst" &&
    ++	git -c protocol.version=0 clone --no-local --revision=refs/heads/main . dst &&
    ++	git rev-parse refs/heads/main >expect &&
    ++	git -C dst rev-parse HEAD >actual &&
    ++	test_cmp expect actual
    ++'
    ++
    + test_expect_success 'clone with --revision being a short raw commit hash' '
    + 	test_when_finished "rm -rf dst" &&
    + 	oid=$(git rev-parse --short refs/heads/feature) &&
-- 
2.55.0.379.g6d629a7221