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

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

[Adrian Friedli]

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

> 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 for your review.

While implementing the test I discovered it is protocol version 0 which
triggers that behavior. I updated the log message and implemented a test.

Kind regards,
Adrian

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

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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 for your review.
>
> While implementing the test I discovered it is protocol version 0 which
> triggers that behavior. I updated the log message and implemented a test.

Oh, the proposed log message reads wonderfully.  Thanks for digging
down to the root cause of the issue.

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

[Adrian Friedli]

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 <adrian.friedli@mt.com>
---
 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 <adrian.friedli@mt.com>
     
      ## 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 <adrian.friedli@mt.com>
     
    @@ 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