Segfault when cloning http:// without libcurl

Segfault when cloning http:// without libcurl

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

Hi all

1.5.6.rc0.15.gd513 segfaults when attempting to clone from a http://
repository if compiled without libcurl:

(gdb) run clone http://repo.or.cz/r/git-homepage.git/
[..]
error: git was compiled without libcurl support.

Program received signal SIGSEGV, Segmentation fault.
0x00000000 in ?? ()
(gdb) bt
#0  0x00000000 in ?? ()
#1  0x080dd18d in transport_get_remote_refs (transport=0x814dbf0) at transport.c:795
#2  0x0805d24b in cmd_clone (argc=1, argv=0xbfda0ae8, prefix=0x0) at builtin-clone.c:461
#3  0x0804adbf in handle_internal_command (argc=2, argv=0xbfda0ae8) at git.c:249
#4  0x0804afa9 in main (argc=2, argv=0xbfda0ae8) at git.c:444
(gdb) up
#1  0x080dd18d in transport_get_remote_refs (transport=0x814dbf0) at transport.c:795
795                     transport->remote_refs = transport->get_refs_list(transport);

The underlying problem seems to be that at builtin-clone.c:160, no
error checking is done on the output of transport_get():

	transport = transport_get(remote, ref_git_copy);
	for (extra = transport_get_remote_refs(transport); extra;
	     extra = extra->next)
		add_extra_ref(extra->name, extra->old_sha1, 0);

But transport_get() never sets the ->get_refs_list() member if libcurl
wasn't enabled at compile time, cf. transport.c:738:

#ifdef NO_CURL
		error("git was compiled without libcurl support.");
#else
		ret->get_refs_list = get_refs_via_curl;
		ret->fetch = fetch_objs_via_curl;
		ret->push = curl_transport_push;
#endif

Some digging shows that at the time the above #ifdef was inserted
(ccfc02a3), there was no builtin-clone.c, so the error checking
probably got lost in the translation.

I'd attempt to write a patch, but it looks like I would have to read
into a lot of code for a fairly trivial issue, so I hope someone can
help me out with this...

- Thomas

-- 
Thomas Rast
trast@student.ethz.ch



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

[PATCH] clone: make sure we support the transport type

[Jeff King]

If we use an unsupported transport (e.g., http when curl
support is not compiled in), transport_get reports an error
to the user, but we still get a transport object. We need to
manually check and abort the clone process at that point, or
we end up with a segfault.

Noticed by Thomas Rast.

Signed-off-by: Jeff King <peff@peff.net>
---
There are a few other calls to transport_get in builtin-clone, for
setting up references and doing local cloning. I didn't check, but
assumed it was impossible for http:// remotes to make it to that code
path.

 builtin-clone.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 4740b13..f4accbe 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -449,6 +449,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		struct remote *remote = remote_get(argv[0]);
 		struct transport *transport = transport_get(remote, argv[0]);
 
+		if (!transport->get_refs_list || !transport->fetch)
+			die("Don't know how to clone %s", transport->url);
+
 		transport_set_option(transport, TRANS_OPT_KEEP, "yes");
 
 		if (option_depth)
-- 
1.5.6.rc0.128.g5fd3b9.dirty

Re: [PATCH] clone: make sure we support the transport type

[Mike Hommey]

On Tue, May 27, 2008 at 10:28:43AM -0400, Jeff King wrote:
> If we use an unsupported transport (e.g., http when curl
> support is not compiled in), transport_get reports an error
> to the user, but we still get a transport object. We need to
> manually check and abort the clone process at that point, or
> we end up with a segfault.

Shouldn't transport_get return NULL in such a situation, instead ?

Mike

Re: [PATCH] clone: make sure we support the transport type

[Daniel Barkalow]

On Tue, 27 May 2008, Mike Hommey wrote:

> On Tue, May 27, 2008 at 10:28:43AM -0400, Jeff King wrote:
> > If we use an unsupported transport (e.g., http when curl
> > support is not compiled in), transport_get reports an error
> > to the user, but we still get a transport object. We need to
> > manually check and abort the clone process at that point, or
> > we end up with a segfault.
> 
> Shouldn't transport_get return NULL in such a situation, instead ?

Perhaps, but we would still want to account for the possibility of a 
transport that only supports pushing or something like that, and therefore 
is available for the URL but isn't suitable for clone.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] clone: make sure we support the transport type

[Daniel Barkalow]

On Tue, 27 May 2008, Jeff King wrote:

> If we use an unsupported transport (e.g., http when curl
> support is not compiled in), transport_get reports an error
> to the user, but we still get a transport object. We need to
> manually check and abort the clone process at that point, or
> we end up with a segfault.
> 
> Noticed by Thomas Rast.

Good catch. I think it might be better to have the transport functions 
report failure when the method requested is NULL, but it's also worthwhile 
to notice this in advance and give the user a comprehensive message in 
advance.

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> There are a few other calls to transport_get in builtin-clone, for
> setting up references and doing local cloning. I didn't check, but
> assumed it was impossible for http:// remotes to make it to that code
> path.
> 
>  builtin-clone.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 4740b13..f4accbe 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -449,6 +449,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		struct remote *remote = remote_get(argv[0]);
>  		struct transport *transport = transport_get(remote, argv[0]);
>  
> +		if (!transport->get_refs_list || !transport->fetch)
> +			die("Don't know how to clone %s", transport->url);
> +
>  		transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>  
>  		if (option_depth)
> -- 
> 1.5.6.rc0.128.g5fd3b9.dirty
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>