{"thread":{"id":"13686","subject":"Segfault when cloning http:// without libcurl","startedAt":"2008-05-27T09:24:36Z","lastAt":"2008-05-27T18:05:37Z","messageCount":5,"participants":["Thomas Rast","Jeff King","Mike Hommey","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"77840","messageId":"200805271124.50172.trast@student.ethz.ch","threadId":"13686","inReplyTo":null,"subject":"Segfault when cloning http:// without libcurl","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2008-05-27T09:24:36Z","receivedAt":"2008-05-27T09:24:36Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hi all\n\n1.5.6.rc0.15.gd513 segfaults when attempting to clone from a http://\nrepository if compiled without libcurl:\n\n(gdb) run clone http://repo.or.cz/r/git-homepage.git/\n[..]\nerror: git was compiled without libcurl support.\n\nProgram received signal SIGSEGV, Segmentation fault.\n0x00000000 in ?? ()\n(gdb) bt\n#0  0x00000000 in ?? ()\n#1  0x080dd18d in transport_get_remote_refs (transport=0x814dbf0) at transport.c:795\n#2  0x0805d24b in cmd_clone (argc=1, argv=0xbfda0ae8, prefix=0x0) at builtin-clone.c:461\n#3  0x0804adbf in handle_internal_command (argc=2, argv=0xbfda0ae8) at git.c:249\n#4  0x0804afa9 in main (argc=2, argv=0xbfda0ae8) at git.c:444\n(gdb) up\n#1  0x080dd18d in transport_get_remote_refs (transport=0x814dbf0) at transport.c:795\n795                     transport->remote_refs = transport->get_refs_list(transport);\n\nThe underlying problem seems to be that at builtin-clone.c:160, no\nerror checking is done on the output of transport_get():\n\n\ttransport = transport_get(remote, ref_git_copy);\n\tfor (extra = transport_get_remote_refs(transport); extra;\n\t     extra = extra->next)\n\t\tadd_extra_ref(extra->name, extra->old_sha1, 0);\n\nBut transport_get() never sets the ->get_refs_list() member if libcurl\nwasn't enabled at compile time, cf. transport.c:738:\n\n#ifdef NO_CURL\n\t\terror(\"git was compiled without libcurl support.\");\n#else\n\t\tret->get_refs_list = get_refs_via_curl;\n\t\tret->fetch = fetch_objs_via_curl;\n\t\tret->push = curl_transport_push;\n#endif\n\nSome digging shows that at the time the above #ifdef was inserted\n(ccfc02a3), there was no builtin-clone.c, so the error checking\nprobably got lost in the translation.\n\nI'd attempt to write a patch, but it looks like I would have to read\ninto a lot of code for a fairly trivial issue, so I hope someone can\nhelp me out with this...\n\n- Thomas\n\n-- \nThomas Rast\ntrast@student.ethz.ch\n\n\n"},{"id":"77862","messageId":"20080527142842.GA7651@sigill.intra.peff.net","threadId":"13686","inReplyTo":"200805271124.50172.trast@student.ethz.ch","subject":"[PATCH] clone: make sure we support the transport type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-05-27T14:28:43Z","receivedAt":"2008-05-27T14:28:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we use an unsupported transport (e.g., http when curl\nsupport is not compiled in), transport_get reports an error\nto the user, but we still get a transport object. We need to\nmanually check and abort the clone process at that point, or\nwe end up with a segfault.\n\nNoticed by Thomas Rast.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThere are a few other calls to transport_get in builtin-clone, for\nsetting up references and doing local cloning. I didn't check, but\nassumed it was impossible for http:// remotes to make it to that code\npath.\n\n builtin-clone.c |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 4740b13..f4accbe 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -449,6 +449,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tstruct remote *remote = remote_get(argv[0]);\n \t\tstruct transport *transport = transport_get(remote, argv[0]);\n \n+\t\tif (!transport->get_refs_list || !transport->fetch)\n+\t\t\tdie(\"Don't know how to clone %s\", transport->url);\n+\n \t\ttransport_set_option(transport, TRANS_OPT_KEEP, \"yes\");\n \n \t\tif (option_depth)\n-- \n1.5.6.rc0.128.g5fd3b9.dirty\n"},{"id":"77872","messageId":"20080527163027.GB11865@glandium.org","threadId":"13686","inReplyTo":"20080527142842.GA7651@sigill.intra.peff.net","subject":"Re: [PATCH] clone: make sure we support the transport type","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2008-05-27T16:30:27Z","receivedAt":"2008-05-27T16:30:27Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, May 27, 2008 at 10:28:43AM -0400, Jeff King wrote:\n> If we use an unsupported transport (e.g., http when curl\n> support is not compiled in), transport_get reports an error\n> to the user, but we still get a transport object. We need to\n> manually check and abort the clone process at that point, or\n> we end up with a segfault.\n\nShouldn't transport_get return NULL in such a situation, instead ?\n\nMike\n"},{"id":"77873","messageId":"alpine.LNX.1.00.0805271334490.19665@iabervon.org","threadId":"13686","inReplyTo":"20080527163027.GB11865@glandium.org","subject":"Re: [PATCH] clone: make sure we support the transport type","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-05-27T17:49:56Z","receivedAt":"2008-05-27T17:49:56Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 27 May 2008, Mike Hommey wrote:\n\n> On Tue, May 27, 2008 at 10:28:43AM -0400, Jeff King wrote:\n> > If we use an unsupported transport (e.g., http when curl\n> > support is not compiled in), transport_get reports an error\n> > to the user, but we still get a transport object. We need to\n> > manually check and abort the clone process at that point, or\n> > we end up with a segfault.\n> \n> Shouldn't transport_get return NULL in such a situation, instead ?\n\nPerhaps, but we would still want to account for the possibility of a \ntransport that only supports pushing or something like that, and therefore \nis available for the URL but isn't suitable for clone.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"77876","messageId":"alpine.LNX.1.00.0805271352070.19665@iabervon.org","threadId":"13686","inReplyTo":"20080527142842.GA7651@sigill.intra.peff.net","subject":"Re: [PATCH] clone: make sure we support the transport type","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-05-27T18:05:37Z","receivedAt":"2008-05-27T18:05:37Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 27 May 2008, Jeff King wrote:\n\n> If we use an unsupported transport (e.g., http when curl\n> support is not compiled in), transport_get reports an error\n> to the user, but we still get a transport object. We need to\n> manually check and abort the clone process at that point, or\n> we end up with a segfault.\n> \n> Noticed by Thomas Rast.\n\nGood catch. I think it might be better to have the transport functions \nreport failure when the method requested is NULL, but it's also worthwhile \nto notice this in advance and give the user a comprehensive message in \nadvance.\n\nAcked-by: Daniel Barkalow <barkalow@iabervon.org>\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> There are a few other calls to transport_get in builtin-clone, for\n> setting up references and doing local cloning. I didn't check, but\n> assumed it was impossible for http:// remotes to make it to that code\n> path.\n> \n>  builtin-clone.c |    3 +++\n>  1 files changed, 3 insertions(+), 0 deletions(-)\n> \n> diff --git a/builtin-clone.c b/builtin-clone.c\n> index 4740b13..f4accbe 100644\n> --- a/builtin-clone.c\n> +++ b/builtin-clone.c\n> @@ -449,6 +449,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\tstruct remote *remote = remote_get(argv[0]);\n>  \t\tstruct transport *transport = transport_get(remote, argv[0]);\n>  \n> +\t\tif (!transport->get_refs_list || !transport->fetch)\n> +\t\t\tdie(\"Don't know how to clone %s\", transport->url);\n> +\n>  \t\ttransport_set_option(transport, TRANS_OPT_KEEP, \"yes\");\n>  \n>  \t\tif (option_depth)\n> -- \n> 1.5.6.rc0.128.g5fd3b9.dirty\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"}]}