{"thread":{"id":"66054","subject":"[PATCH resend] builtin/clone: fix segfault when using --revision on some servers","startedAt":"2026-07-23T14:43:45Z","lastAt":"2026-07-24T16:04:37Z","messageCount":6,"participants":["Adrian Friedli","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"548813","messageId":"20260723144318.69007-1-adrian.friedli@mt.com","threadId":"66054","inReplyTo":null,"subject":"[PATCH resend] builtin/clone: fix segfault when using --revision on some servers","fromName":"Adrian Friedli","fromEmail":"adrian.friedli@mt.com","sentAt":"2026-07-23T14:43:18Z","receivedAt":"2026-07-23T14:43:45Z","isPatch":true,"body":"Fix a segfault when a server advertises more refs than requested when\nusing the --revision argument.\n\nSigned-off-by: Adrian Friedli <adrian.friedli@mt.com>\n---\nThe segfault can be reproduced by e.g.\n\ngit clone --revision=refs/heads/main \\\nhttps://dev.azure.com/public-git/sample/_git/sample\n\nIn the good case the server respects\n`transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked\nlist `refs` returned by `transport_get_remote_refs()` only contains a\nsingle item, which is the ref requested with the --revision argument.\nBoth `remote_head` returned by `find_ref_by_name()` and\n`remote_head_points_at` returned by `guess_remote_head()` are NULL. The\nguard in `update_remote_refs()` skips a the affected code because\n`remote_head_points_at` is NULL.\n\nIn the bad case the server ignores\n`transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked\nlist `refs` returned by `transport_get_remote_refs()` contains many\nitems, amongst others \"HEAD\". `remote_head` returned by\n`find_ref_by_name()` is not NULL and `remote_head_points_at` returned by\n`guess_remote_head()` is not NULL but its field `peer_ref` is NULL.\nBecause `remote_head_points_at` is not NULL the guard in\n`update_remote_refs()` does not skip the affected code and\n`remote_head_points_at->peer_ref->name` is accessed, which causes a\nsegfault later on.\n\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9d08cd8722..bd0c6f5d56 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -557,7 +557,7 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\twrite_followtags(refs, msg);\n \t}\n \n-\tif (remote_head_points_at && !option_bare) {\n+\tif (remote_head_points_at && remote_head_points_at->peer_ref && !option_bare) {\n \t\tstruct strbuf head_ref = STRBUF_INIT;\n \t\tstrbuf_addstr(&head_ref, branch_top);\n \t\tstrbuf_addstr(&head_ref, \"HEAD\");\n-- \n2.55.0.379.g54b6532b97\n\n"},{"id":"548814","messageId":"xmqqmrvhlnjv.fsf@gitster.g","threadId":"66054","inReplyTo":"20260723144318.69007-1-adrian.friedli@mt.com","subject":"Re: [PATCH resend] builtin/clone: fix segfault when using --revision on some servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-23T15:43:00Z","receivedAt":"2026-07-23T15:43:03Z","isPatch":true,"body":"Adrian Friedli <adrian.friedli@mt.com> writes:\n\n> Fix a segfault when a server advertises more refs than requested when\n> using the --revision argument.\n>\n> Signed-off-by: Adrian Friedli <adrian.friedli@mt.com>\n> ---\n> The segfault can be reproduced by e.g.\n>\n> git clone --revision=refs/heads/main \\\n> https://dev.azure.com/public-git/sample/_git/sample\n\nThe following two paragraphs' worth of explanation deserves to be in\nthe log message:\n\n> In the good case the server respects\n> `transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked\n> list `refs` returned by `transport_get_remote_refs()` only contains a\n> single item, which is the ref requested with the --revision argument.\n> Both `remote_head` returned by `find_ref_by_name()` and\n> `remote_head_points_at` returned by `guess_remote_head()` are NULL. The\n> guard in `update_remote_refs()` skips a the affected code because\n> `remote_head_points_at` is NULL.\n>\n> In the bad case the server ignores\n> `transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked\n> list `refs` returned by `transport_get_remote_refs()` contains many\n> items, amongst others \"HEAD\". `remote_head` returned by\n> `find_ref_by_name()` is not NULL and `remote_head_points_at` returned by\n> `guess_remote_head()` is not NULL but its field `peer_ref` is NULL.\n> Because `remote_head_points_at` is not NULL the guard in\n> `update_remote_refs()` does not skip the affected code and\n> `remote_head_points_at->peer_ref->name` is accessed, which causes a\n> segfault later on.\n\nUsually, our commit log message begins with an observation of the\ncurrent behavior.  We would probably start the log message like\nthis:\n\n    Servers are expected to refrain from advertising excess refs,\n    honoring transport_ls_refs_options.ref_prefixes, when\n\n      $ git clone --revision=refs/heads/main $URL\n\n    contacts them, but when talking to a server that does not (e.g.,\n    <<the URL of the problematic repository goes here>>), the client\n    segfaults.\n\nand the above two paragraphs would flow perfectly after such an\nintroduction.  They clearly explain how the client gets confused by\nunusual server behavior.\n\nThe above write-up makes me wonder if there is a valid case where\nguess_remote_head() should return a non-NULL 'struct ref *' whose\n'.peer_ref' member is NULL.  Unless a non-NULL head that is a symref\nis given, in which case we firmly know where their 'HEAD' points,\nthe function seems to pick a randomly guessed ref out of the given\nlist of refs (supplied to its second parameter) and return a copy of\nit.  However, there does not seem to be any check to ensure that it\npicks a ref with its '.peer_ref' member set.  This may break other\ncode paths that consume the value returned from guess_remote_head()\nin the exact same way, no?\n\nI do not know offhand if that is the case, but if it is always wrong\nfor guess_remote_head() to return a guessed ref with NULL in its\n'.peer_ref' member, perhaps that would be a better location to make\nthis fix.  What do you think?\n\nIn any case, can we also add a test to prevent this fix from\nregressing in the future?\n\nThanks.\n\n\n>  builtin/clone.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 9d08cd8722..bd0c6f5d56 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -557,7 +557,7 @@ static void update_remote_refs(const struct ref *refs,\n>  \t\t\twrite_followtags(refs, msg);\n>  \t}\n>  \n> -\tif (remote_head_points_at && !option_bare) {\n> +\tif (remote_head_points_at && remote_head_points_at->peer_ref && !option_bare) {\n>  \t\tstruct strbuf head_ref = STRBUF_INIT;\n>  \t\tstrbuf_addstr(&head_ref, branch_top);\n>  \t\tstrbuf_addstr(&head_ref, \"HEAD\");\n"},{"id":"548815","messageId":"xmqqfr19lmau.fsf@gitster.g","threadId":"66054","inReplyTo":"xmqqmrvhlnjv.fsf@gitster.g","subject":"Re: [PATCH resend] builtin/clone: fix segfault when using --revision on some servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-23T16:10:01Z","receivedAt":"2026-07-23T16:10:08Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I do not know offhand if that is the case, but if it is always wrong\n> for guess_remote_head() to return a guessed ref with NULL in its\n> '.peer_ref' member, perhaps that would be a better location to make\n> this fix.  What do you think?\n\nNever mind, scratch that part.  If we are fetching without storing the\nresult in any remote-tracking ref, '.peer_ref' is legitimately NULL,\nand if we are storing, '.peer_ref' names the local ref where we store\nthe result.  This should not affect our guess as to which of their\nbranches may be pointed to by their 'HEAD'.\n\nSo this patch fixes the issue in the right place.  It would still be\nnice to have a new test to prevent future regressions, though.\n\n\nThanks.\n"},{"id":"548893","messageId":"20260724123735.666021-1-adrian.friedli@mt.com","threadId":"66054","inReplyTo":"xmqqfr19lmau.fsf@gitster.g","subject":"Re: [PATCH resend] builtin/clone: fix segfault when using --revision on some servers","fromName":"Adrian Friedli","fromEmail":"adrian.friedli@mt.com","sentAt":"2026-07-24T12:37:35Z","receivedAt":"2026-07-24T12:37:46Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> So this patch fixes the issue in the right place.  It would still be\n> nice to have a new test to prevent future regressions, though.\n\nThanks for your review.\n\nWhile implementing the test I discovered it is protocol version 0 which\ntriggers that behavior. I updated the log message and implemented a test.\n\nKind regards,\nAdrian\n"},{"id":"548894","messageId":"20260724124138.666877-1-adrian.friedli@mt.com","threadId":"66054","inReplyTo":"20260723144318.69007-1-adrian.friedli@mt.com","subject":"[PATCH v2] builtin/clone: fix segfault when using --revision with protocol v0","fromName":"Adrian Friedli","fromEmail":"adrian.friedli@mt.com","sentAt":"2026-07-24T12:41:38Z","receivedAt":"2026-07-24T12:42:25Z","isPatch":true,"body":"Servers supporting protocol v2 do not advertise excess refs and honor\n`transport_ls_refs_options.ref_prefixes` when\n\n    $ git clone --revision=refs/heads/main $URL\n\ncontacts them, but when talking to a server that does not support\nprotocol v2 the client segfaults. This can also be observed when v0 is\nenforced for example by\n\n    $ git -c protocol.version=0 clone --revision=refs/heads/main $URL\n\nIn the protocol v2 case the server honors\n`transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked\nlist `refs` returned by `transport_get_remote_refs()` only contains a\nsingle item, which is the ref requested with the --revision argument.\nBoth `remote_head` returned by `find_ref_by_name()` and\n`remote_head_points_at` returned by `guess_remote_head()` are NULL. The\nguard in `update_remote_refs()` skips a the affected code because\n`remote_head_points_at` is NULL.\n\nIn the protocol v0 case in `cmd_clone()` the linked list `refs` returned\nby `transport_get_remote_refs()` contains many items, amongst others\n\"HEAD\". `remote_head` returned by `find_ref_by_name()` is not NULL and\n`remote_head_points_at` returned by `guess_remote_head()` is not NULL\nbut its field `peer_ref` is NULL. Because `remote_head_points_at` is not\nNULL the guard in `update_remote_refs()` does not skip the affected code\nand `remote_head_points_at->peer_ref->name` is accessed, which causes a\nsegfault later on.\n\nSigned-off-by: Adrian Friedli <adrian.friedli@mt.com>\n---\n builtin/clone.c           | 2 +-\n t/t5621-clone-revision.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9d08cd8722..bd0c6f5d56 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -557,7 +557,7 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\twrite_followtags(refs, msg);\n \t}\n \n-\tif (remote_head_points_at && !option_bare) {\n+\tif (remote_head_points_at && remote_head_points_at->peer_ref && !option_bare) {\n \t\tstruct strbuf head_ref = STRBUF_INIT;\n \t\tstrbuf_addstr(&head_ref, branch_top);\n \t\tstrbuf_addstr(&head_ref, \"HEAD\");\ndiff --git a/t/t5621-clone-revision.sh b/t/t5621-clone-revision.sh\nindex db3b8cff55..54789423f8 100755\n--- a/t/t5621-clone-revision.sh\n+++ b/t/t5621-clone-revision.sh\n@@ -90,6 +90,14 @@ test_expect_success 'clone with --revision and --bare' '\n \ttest_must_fail git -C dst config remote.origin.fetch\n '\n \n+test_expect_success 'clone with --revision and protocol v0' '\n+\ttest_when_finished \"rm -rf dst\" &&\n+\tgit -c protocol.version=0 clone --no-local --revision=refs/heads/main . dst &&\n+\tgit rev-parse refs/heads/main >expect &&\n+\tgit -C dst rev-parse HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'clone with --revision being a short raw commit hash' '\n \ttest_when_finished \"rm -rf dst\" &&\n \toid=$(git rev-parse --short refs/heads/feature) &&\n\nRange-diff against v1:\n1:  f300b7b968 ! 1:  1a7cd98b7a builtin/clone: fix segfault when using --revision on some servers\n    @@ Metadata\n     Author: Adrian Friedli <adrian.friedli@mt.com>\n     \n      ## Commit message ##\n    -    builtin/clone: fix segfault when using --revision on some servers\n    +    builtin/clone: fix segfault when using --revision with protocol v0\n     \n    -    Fix a segfault when a server advertises more refs than requested when\n    -    using the --revision argument.\n    +    Servers supporting protocol v2 do not advertise excess refs and honor\n    +    `transport_ls_refs_options.ref_prefixes` when\n    +\n    +        $ git clone --revision=refs/heads/main $URL\n    +\n    +    contacts them, but when talking to a server that does not support\n    +    protocol v2 the client segfaults. This can also be observed when v0 is\n    +    enforced for example by\n    +\n    +        $ git -c protocol.version=0 clone --revision=refs/heads/main $URL\n    +\n    +    In the protocol v2 case the server honors\n    +    `transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the linked\n    +    list `refs` returned by `transport_get_remote_refs()` only contains a\n    +    single item, which is the ref requested with the --revision argument.\n    +    Both `remote_head` returned by `find_ref_by_name()` and\n    +    `remote_head_points_at` returned by `guess_remote_head()` are NULL. The\n    +    guard in `update_remote_refs()` skips a the affected code because\n    +    `remote_head_points_at` is NULL.\n    +\n    +    In the protocol v0 case in `cmd_clone()` the linked list `refs` returned\n    +    by `transport_get_remote_refs()` contains many items, amongst others\n    +    \"HEAD\". `remote_head` returned by `find_ref_by_name()` is not NULL and\n    +    `remote_head_points_at` returned by `guess_remote_head()` is not NULL\n    +    but its field `peer_ref` is NULL. Because `remote_head_points_at` is not\n    +    NULL the guard in `update_remote_refs()` does not skip the affected code\n    +    and `remote_head_points_at->peer_ref->name` is accessed, which causes a\n    +    segfault later on.\n     \n         Signed-off-by: Adrian Friedli <adrian.friedli@mt.com>\n     \n    @@ builtin/clone.c: static void update_remote_refs(const struct ref *refs,\n      \t\tstruct strbuf head_ref = STRBUF_INIT;\n      \t\tstrbuf_addstr(&head_ref, branch_top);\n      \t\tstrbuf_addstr(&head_ref, \"HEAD\");\n    +\n    + ## t/t5621-clone-revision.sh ##\n    +@@ t/t5621-clone-revision.sh: test_expect_success 'clone with --revision and --bare' '\n    + \ttest_must_fail git -C dst config remote.origin.fetch\n    + '\n    + \n    ++test_expect_success 'clone with --revision and protocol v0' '\n    ++\ttest_when_finished \"rm -rf dst\" &&\n    ++\tgit -c protocol.version=0 clone --no-local --revision=refs/heads/main . dst &&\n    ++\tgit rev-parse refs/heads/main >expect &&\n    ++\tgit -C dst rev-parse HEAD >actual &&\n    ++\ttest_cmp expect actual\n    ++'\n    ++\n    + test_expect_success 'clone with --revision being a short raw commit hash' '\n    + \ttest_when_finished \"rm -rf dst\" &&\n    + \toid=$(git rev-parse --short refs/heads/feature) &&\n-- \n2.55.0.379.g6d629a7221\n\n"},{"id":"548899","messageId":"xmqqldb08jcd.fsf@gitster.g","threadId":"66054","inReplyTo":"20260724123735.666021-1-adrian.friedli@mt.com","subject":"Re: [PATCH resend] builtin/clone: fix segfault when using --revision on some servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-24T16:04:34Z","receivedAt":"2026-07-24T16:04:37Z","isPatch":true,"body":"Adrian Friedli <adrian.friedli@mt.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> So this patch fixes the issue in the right place.  It would still be\n>> nice to have a new test to prevent future regressions, though.\n>\n> Thanks for your review.\n>\n> While implementing the test I discovered it is protocol version 0 which\n> triggers that behavior. I updated the log message and implemented a test.\n\nOh, the proposed log message reads wonderfully.  Thanks for digging\ndown to the root cause of the issue.\n"}]}