{"thread":{"id":"65150","subject":"[PATCH] builtin/clone: fix segfault when using --revision on some servers","startedAt":"2026-03-06T11:10:32Z","lastAt":"2026-04-09T13:09:28Z","messageCount":4,"participants":["Adrian Friedli","Junio C Hamano","Friedli Adrian LCPF-CH"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"538068","messageId":"20260306111001.261916-1-adrian.friedli@mt.com","threadId":"65150","inReplyTo":null,"subject":"[PATCH] builtin/clone: fix segfault when using --revision on some servers","fromName":"Adrian Friedli","fromEmail":"adrian.friedli@mt.com","sentAt":"2026-03-06T11:10:01Z","receivedAt":"2026-03-06T11:10:32Z","isPatch":true,"body":"Fix a segfault when a server advertises more refs than requested when\nusing the --revision argument.\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\nExtend the guard in `update_remote_refs()` to also skip the block of\ncode if `remote_head_points_at->peer_ref` is NULL.\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\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 fba3c9c508..09219791da 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.53.0.394.g500c12b044\n\n"},{"id":"538104","messageId":"xmqqwlzozqgb.fsf@gitster.g","threadId":"65150","inReplyTo":"20260306111001.261916-1-adrian.friedli@mt.com","subject":"Re: [PATCH] builtin/clone: fix segfault when using --revision on some servers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-06T19:51:48Z","receivedAt":"2026-03-06T19:51:51Z","isPatch":true,"body":"Adrian Friedli <adrian.friedli@mt.com> writes:\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\nThe description makes it sound more like this code is perfectly\nfine, and the problem is in guess_remote_head() that reads the refs\nlist and includes such a bogus thing with no peer_ref in the result\nof its guessing.  There are 4 direct callers to guess_remote_head()\nincluding this one---wouldn't they also obtain a list with such a\nref entry?\n\nOr is guess_remote_head() correct in that some uses of its result do\nnot mind such a ref with no peer_ref, but only this code path wants\nto see a ref with peer_ref?  If that is the case, then shouldn't the\ncode this patch touches be looping over remote_head_points_at to see\nif there is one with a peer_ref and use that?  The original is\nassuming that remote_head_points_at that is not NULL has a valid and\nusable entry at the beginning of the list, but if that assumption\ndoes not hold and we are getting multiple hits, wouldn't it be\npossible that a good entry is hidden behind a bad one in the list of\nrefs?\n\nThanks.\n\n> Extend the guard in `update_remote_refs()` to also skip the block of\n> code if `remote_head_points_at->peer_ref` is NULL.\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>\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 fba3c9c508..09219791da 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":"538740","messageId":"DB8SPR01MB0009F61C462C819CDFF80ED7EA44A@DB8SPR01MB0009.eurprd03.prod.outlook.com","threadId":"65150","inReplyTo":"xmqqwlzozqgb.fsf@gitster.g","subject":"Re: [PATCH] builtin/clone: fix segfault when using --revision on some servers","fromName":"Friedli Adrian LCPF-CH","fromEmail":"adrian.friedli@mt.com","sentAt":"2026-03-12T12:34:38Z","receivedAt":"2026-03-12T12:34:41Z","isPatch":true,"body":"Hi\n\nThanks for your response.\n\n> > In the bad case the server ignores\n> > `transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the\n> > linked list `refs` returned by `transport_get_remote_refs()` contains\n> > many items, amongst others \"HEAD\". `remote_head` returned by\n> > `find_ref_by_name()` is not NULL and `remote_head_points_at` returned\n> > by `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> \n> The description makes it sound more like this code is perfectly fine, and the\n> problem is in guess_remote_head() that reads the refs list and includes such a\n> bogus thing with no peer_ref in the result of its guessing.  There are 4 direct\n> callers to guess_remote_head() including this one---wouldn't they also obtain\n> a list with such a ref entry?\n\nI traced the 3 other callers to guess_remote_head() and none of them has a\nproblem if peer_ref is NULL. In get_expanded_map() there is even a condition\n`if (cpy->peer_ref)`, which indicates peer_ref is allowed to be NULL.\n\n> Or is guess_remote_head() correct in that some uses of its result do not mind\n> such a ref with no peer_ref, but only this code path wants to see a ref with\n> peer_ref?  If that is the case, then shouldn't the code this patch touches be\n> looping over remote_head_points_at to see if there is one with a peer_ref and\n> use that?  The original is assuming that remote_head_points_at that is not\n> NULL has a valid and usable entry at the beginning of the list, but if that\n> assumption does not hold and we are getting multiple hits, wouldn't it be\n> possible that a good entry is hidden behind a bad one in the list of refs?\n\nIn this path, guess_remote_head() is called without the\nREMOTE_GUESS_HEAD_ALL flag, which makes it return only the most likely ref.\n\nKind regards,\nAdrian\n"},{"id":"541262","messageId":"20260409130904.1811-1-adrian.friedli@mt.com","threadId":"65150","inReplyTo":"DB8SPR01MB0009F61C462C819CDFF80ED7EA44A@DB8SPR01MB0009.eurprd03.prod.outlook.com","subject":"Re: [PATCH] builtin/clone: fix segfault when using --revision on some servers","fromName":"Adrian Friedli","fromEmail":"adrian.friedli@mt.com","sentAt":"2026-04-09T13:09:04Z","receivedAt":"2026-04-09T13:09:28Z","isPatch":true,"body":"> > > In the bad case the server ignores\n> > > `transport_ls_refs_options.ref_prefixes` and in `cmd_clone()` the\n> > > linked list `refs` returned by `transport_get_remote_refs()` contains\n> > > many items, amongst others \"HEAD\". `remote_head` returned by\n> > > `find_ref_by_name()` is not NULL and `remote_head_points_at` returned\n> > > by `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> >\n> > The description makes it sound more like this code is perfectly fine, and the\n> > problem is in guess_remote_head() that reads the refs list and includes such a\n> > bogus thing with no peer_ref in the result of its guessing.  There are 4 direct\n> > callers to guess_remote_head() including this one---wouldn't they also obtain\n> > a list with such a ref entry?\n> \n> I traced the 3 other callers to guess_remote_head() and none of them has a\n> problem if peer_ref is NULL. In get_expanded_map() there is even a condition\n> `if (cpy->peer_ref)`, which indicates peer_ref is allowed to be NULL.\n\nThe details of the 4 callers to guess_remote_head()\n\nbuiltin/fetch.c:set_head()\nbuiltin/remote.c:get_head_names()\n\n  refs from guess_remote_head() are only used with ref->name\n\nbuiltin/clone.c:wanted_peer_refs()\n\n  Does not use REMOTE_GUESS_HEAD_ALL flag in guess_remote_head() call. So only one ref is returned.\n\n  calls get_fetch_map() with refs from guess_remote_head().\n    may call get_expanded_map()\n      safe if peer_ref is NULL, has a guard on the copy of ref `if (cpy->peer_ref)`.\n    may call get_remote_ref()\n      calls get_remote_ref()\n        calls find_ref_by_name_abbrev()\n\t  only looks at name and next\n\nbuiltin/clone.c:cmd_clone()\n\n  Does not use REMOTE_GUESS_HEAD_ALL flag in guess_remote_head() call. So only one ref is returned.\n\n  may call write_refspec_config() with refs from guess_remote_head()\n    only uses name from refs\n\n  calls update_remote_refs() with refs from guess_remote_head() \n  -> thats the failing one, no other uses\n\n  may call update_head() with refs from guess_remote_head()\n    only uses name and old_oid from refs\n\nKind regards\nAdrian\n"}]}