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

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

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

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

Mike

On Tue, 27 May 2008, Mike Hommey wrote:

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:

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>