Re: [PATCH v3 1/2] refspec: safely parse refspecs outside a repository

[Junio C Hamano <gitster@pobox.com>]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> When git-remote-http is invoked outside of a repository (for example,
> by running `git ls-remote` in a non-git directory with a globally
> configured fetch refspec), `the_hash_algo` is left as NULL by
> setup_git_directory_gently().
>
> parse_refspec() checks whether the LHS of a refspec is an exact OID by
> evaluating `llen == the_hash_algo->hexsz`. With `the_hash_algo` being
> NULL, this results in a segmentation fault. The same NULL dereference
> exists in the negative refspec path.
>
> Note that builtin/ls-remote already works around a related issue by
> setting a fallback hash algorithm before calling into the transport
> layer (see 9e89dcb66a). However, since remote-curl runs as a separate
> process, that fix does not help here.
>
> Guard both dereferences with a NULL check on `the_hash_algo`. When
> operating outside a repository, fetching and pushing are impossible
> anyway, so skipping the exact OID check is safe: the exact_sha1 flag
> only influences ref prefixes sent to a remote v2 upload-pack during
> fetch, and we will never reach that point without a local repository.
>
> Reported-by: Jo Liss <joliss@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  refspec.c                   | 4 ++--
>  t/t5551-http-fetch-smart.sh | 7 +++++++
>  2 files changed, 9 insertions(+), 2 deletions(-)

Looking good.  Shall we declare victory and mark the topic for
'next' by now?

Thanks.

> diff --git a/refspec.c b/refspec.c
> index 0775358d96..a864a0bac2 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -84,7 +84,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
>  		 */
>  		if (!*item->src)
>  			return 0; /* negative refspecs must not be empty */
> -		else if (llen == the_hash_algo->hexsz && !get_oid_hex(item->src, &unused))
> +		else if (the_hash_algo && llen == the_hash_algo->hexsz && !get_oid_hex(item->src, &unused))
>  			return 0; /* negative refpsecs cannot be exact sha1 */
>  		else if (!check_refname_format(item->src, flags))
>  			; /* valid looking ref is ok */
> @@ -101,7 +101,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet
>  		/* LHS */
>  		if (!*item->src)
>  			; /* empty is ok; it means "HEAD" */
> -		else if (llen == the_hash_algo->hexsz && !get_oid_hex(item->src, &unused))
> +		else if (the_hash_algo && llen == the_hash_algo->hexsz && !get_oid_hex(item->src, &unused))
>  			item->exact_sha1 = 1; /* ok */
>  		else if (!check_refname_format(item->src, flags))
>  			; /* valid looking ref is ok */
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 73cf531580..a26b6c2844 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -782,4 +782,11 @@ test_expect_success 'tag following always works over v0 http' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'ls-remote outside repo does not segfault with fetch refspec' '
> +	nongit git \
> +		-c remote.origin.url="$HTTPD_URL/smart/repo.git" \
> +		-c remote.origin.fetch=anything \
> +		ls-remote origin
> +'
> +
>  test_done