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

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

[Adrian Friedli]

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

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.

Extend the guard in `update_remote_refs()` to also skip the block of
code if `remote_head_points_at->peer_ref` is NULL.

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

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

diff --git a/builtin/clone.c b/builtin/clone.c
index fba3c9c508..09219791da 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");
-- 
2.53.0.394.g500c12b044

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

[Junio C Hamano]

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

> 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.

The description makes it sound more like this code is perfectly
fine, and the problem is in guess_remote_head() that reads the refs
list and includes such a bogus thing with no peer_ref in the result
of its guessing.  There are 4 direct callers to guess_remote_head()
including this one---wouldn't they also obtain a list with such a
ref entry?

Or is guess_remote_head() correct in that some uses of its result do
not mind such a ref with no peer_ref, but only this code path wants
to see a ref with peer_ref?  If that is the case, then shouldn't the
code this patch touches be looping over remote_head_points_at to see
if there is one with a peer_ref and use that?  The original is
assuming that remote_head_points_at that is not NULL has a valid and
usable entry at the beginning of the list, but if that assumption
does not hold and we are getting multiple hits, wouldn't it be
possible that a good entry is hidden behind a bad one in the list of
refs?

Thanks.

> Extend the guard in `update_remote_refs()` to also skip the block of
> code if `remote_head_points_at->peer_ref` is NULL.
>
> 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
>
>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index fba3c9c508..09219791da 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");

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

[Friedli Adrian LCPF-CH]

Hi

Thanks for your response.

> > 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.
> 
> The description makes it sound more like this code is perfectly fine, and the
> problem is in guess_remote_head() that reads the refs list and includes such a
> bogus thing with no peer_ref in the result of its guessing.  There are 4 direct
> callers to guess_remote_head() including this one---wouldn't they also obtain
> a list with such a ref entry?

I traced the 3 other callers to guess_remote_head() and none of them has a
problem if peer_ref is NULL. In get_expanded_map() there is even a condition
`if (cpy->peer_ref)`, which indicates peer_ref is allowed to be NULL.

> Or is guess_remote_head() correct in that some uses of its result do not mind
> such a ref with no peer_ref, but only this code path wants to see a ref with
> peer_ref?  If that is the case, then shouldn't the code this patch touches be
> looping over remote_head_points_at to see if there is one with a peer_ref and
> use that?  The original is assuming that remote_head_points_at that is not
> NULL has a valid and usable entry at the beginning of the list, but if that
> assumption does not hold and we are getting multiple hits, wouldn't it be
> possible that a good entry is hidden behind a bad one in the list of refs?

In this path, guess_remote_head() is called without the
REMOTE_GUESS_HEAD_ALL flag, which makes it return only the most likely ref.

Kind regards,
Adrian