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

[PATCH resend] 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.

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(-)

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");
-- 
2.55.0.379.g54b6532b97

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

[Junio C Hamano]

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

> 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

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

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

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.


>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 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");

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

[Junio C Hamano]

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

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

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.