Segfault when cloning http:// without libcurl

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

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

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

On Tue, 27 May 2008, Mike Hommey wrote:

On Tue, May 27, 2008 at 10:28:43AM -0400, Jeff King wrote:

quoted

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*

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