{"thread":{"id":"21448","subject":"Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","startedAt":"2009-10-31T22:07:02Z","lastAt":"2009-11-02T19:21:46Z","messageCount":13,"participants":["Samium Gromoff","Junio C Hamano","Mike Hommey","Sverre Rabbelier","Daniel Barkalow","Jeff King","Clemens Buchacher"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"126540","messageId":"20091101.010702.527849118592864646._deepfire@feelingofgreen.ru","threadId":"21448","inReplyTo":null,"subject":"Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Samium Gromoff","fromEmail":"_deepfire@feelingofgreen.ru","sentAt":"2009-10-31T22:07:02Z","receivedAt":"2009-10-31T22:07:02Z","isPatch":false,"sender":{"key":"_deepfire@feelingofgreen.ru","avatar":"https://gravatar.com/avatar/d68b7e638272cedce7a897a0fdbe7296ebb1db2cbecaff5b1763c27c94c72940?d=mp&s=160"},"body":"Good day folks,\n\nAttached is the SEGV bugreport I sent to debian.\n\nI tried to convince ld to use /usr/lib/debug, via LD_LIBRARY_PATH,\nand run the thing under gdb 7.0, but it won't use debug libraries\nfor some reason.\n\n\nregards,\n Samium Gromoff\n--\n _deepfire-at-feelingofgreen.ru\nO< ascii ribbon campaign - stop html mail - www.asciiribbon.org\n\n\n\nPackage: git-core\nVersion: 1:1.6.5-1\nSeverity: important\n\n\nBasically,\n\ndeepfire@auriga:/$ git peek-remote http://common-lisp.net/project/qitab/git/poiu.git \nSegmentation fault\ndeepfire@auriga:/$ git peek-remote http://www.lichteblau.com/git/atdoc.git \nSegmentation fault\n\nStrace shows following:\n[snip]\nopen(\"/dev/urandom\", O_RDONLY) = 3\nread(3, \"\\27\\n\\7\\210!\\373\\210\\265\", 8) = 8\nclose(3) = 0\nmprotect(0x7fe055d69000, 16384, PROT_READ) = 0\nmprotect(0x7fe055f89000, 4096, PROT_READ) = 0\nmprotect(0x7fe0563c2000, 4096, PROT_READ) = 0\nmunmap(0x7fe0563ae000, 67595) = 0\nset_tid_address(0x7fe0563ac7c0) = 4047\nset_robust_list(0x7fe0563ac7d0, 0x18) = 0\nfutex(0x7fff5e3c057c, FUTEX_WAKE_PRIVATE, 1) = 0\nfutex(0x7fff5e3c057c, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, 1, NULL, 7fe0563ac6f0) = -1 ENOSYS (Function not implemented)\nrt_sigaction(SIGRTMIN, {0x7fe055d78750, [], SA_RESTORER|SA_SIGINFO, 0x7fe055d819a0}, NULL, 8) = 0\nrt_sigaction(SIGRT_1, {0x7fe055d787e0, [], SA_RESTORER|SA_RESTART|SA_SIGINFO, 0x7fe055d819a0}, NULL, 8) = 0\nrt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0\ngetrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0\nbrk(0) = 0x26d6000\nbrk(0x26f7000) = 0x26f7000\ngetcwd(\"/\", 4096) = 2\nstat(\".git\", 0x7fff5e3c02d0) = -1 ENOENT (No such file or directory)\naccess(\".git/objects\", X_OK) = -1 ENOENT (No such file or directory)\naccess(\"./objects\", X_OK) = -1 ENOENT (No such file or directory)\nchdir(\"..\") = 0\nstat(\".git\", 0x7fff5e3c02d0) = -1 ENOENT (No such file or directory)\naccess(\".git/objects\", X_OK) = -1 ENOENT (No such file or directory)\naccess(\"./objects\", X_OK) = -1 ENOENT (No such file or directory)\nchdir(\"/\") = 0\n--- SIGSEGV (Segmentation fault) @ 0 (0) ---\n+++ killed by SIGSEGV +++\nSegmentation fault\n\n\n-- System Information:\nDebian Release: squeeze/sid\n APT prefers unstable\n APT policy: (500, 'unstable'), (1, 'experimental')\nArchitecture: amd64 (x86_64)\n\nKernel: Linux 2.6.26-2-amd64 (SMP w/2 CPU cores)\nLocale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968)\nShell: /bin/sh linked to /bin/dash\n\nVersions of packages git-core depends on:\nii libc6 2.10.1-3 GNU C Library: Shared libraries\nii libcurl3-gnutls 7.19.5-1.1 Multi-protocol file transfer libra\nii libdigest-sha1-perl 2.12-1 NIST SHA-1 message digest algorith\nii liberror-perl 0.17-1 Perl module for error/exception ha\nii libexpat1 2.0.1-4 XML parsing C library - runtime li\nii perl-modules 5.10.1-6 Core Perl modules\nii zlib1g 1:1.2.3.3.dfsg-15 compression library - runtime\n\nVersions of packages git-core recommends:\nii less 436-1 pager program similar to more\nii openssh-client [ssh-client] 1:5.1p1-8 secure shell client, an rlogin/rsh\nii patch 2.5.9-5 Apply a diff file to an original\nii rsync 3.0.6-1 fast remote file copy program (lik\n\nVersions of packages git-core suggests:\npn git-arch (no description available)\nii git-cvs 1:1.6.5-1 fast, scalable, distributed revisi\nii git-daemon-run 1:1.6.5-1 fast, scalable, distributed revisi\npn git-doc (no description available)\npn git-email (no description available)\nii git-gui 1:1.6.5-1 fast, scalable, distributed revisi\nii git-svn 1:1.6.5-1 fast, scalable, distributed revisi\nii gitk 1:1.6.5-1 fast, scalable, distributed revisi\npn gitweb (no description available)\n\n-- no debconf information\n"},{"id":"126543","messageId":"7v8weq50pm.fsf@alter.siamese.dyndns.org","threadId":"21448","inReplyTo":"20091101.010702.527849118592864646._deepfire@feelingofgreen.ru","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-01T04:27:17Z","receivedAt":"2009-11-01T04:27:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Samium Gromoff <_deepfire@feelingofgreen.ru> writes:\n\n> Attached is the SEGV bugreport I sent to debian.\n\nThanks for a report.\n\nThere are two issues.\n\n * transport-helper.c::get_helper() assumes that the transport structure\n always has its remote member filled and it can get name out of it.\n This is the segv in the report.\n\n * Even if we work around the above issue, the helper subprocess (in this\n case, remote-curl helper) insists on being inside a git repository, To\n satisfy ls-remote request, you do not have to be in one.\n\nAttached is a minimum fix/work around, but this is done without being very\nfamiliar with the current assumptions in the codepaths involved.\n\nIssues I want area experts to consider before coming up with the final fix\nare:\n\n - Should we fix get_helper() in transport-helper.c, instead of touching\n ls-remote.c like this patch does?\n\n This issue really boils down to this question: is it valid for a\n transport to have NULL in its remote field, and should all the code\n that touch transport be prepared to deal with such a transport\n structure?\n\n - When helping to handle ls-remote request, there is no need for the\n helper to know anything about the local state. We probably shouldn't\n even run setup_git_directory_gently() at all in this case. But when\n helping other kinds of request, the helper does need to know where our\n repository is.\n\n In general, what should the initial environment for helpers be? Should\n they assume that they have to figure out where the git repository is\n themselves (in other words, should they assume they cannot rely on\n anything the caller does before they are called? Would the caller\n generally have done the usual repo discovery (including chdir() to the\n toplevel), and there are some set of assumptions they can make? If so\n what are they?\n\n\n builtin-ls-remote.c | 2 +-\n remote-curl.c | 3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-ls-remote.c b/builtin-ls-remote.c\nindex 78a88f7..a8d5613 100644\n--- a/builtin-ls-remote.c\n+++ b/builtin-ls-remote.c\n@@ -86,7 +86,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\t\tpattern[j - i] = p;\n \t\t}\n \t}\n-\tremote = nongit ? NULL : remote_get(dest);\n+\tremote = remote_get(dest);\n \tif (remote && !remote->url_nr)\n \t\tdie(\"remote %s has no configured URL\", dest);\n \ttransport = transport_get(remote, remote ? remote->url[0] : dest);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex ad6a163..7c83f77 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -81,8 +81,9 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *url;\n \tstruct walker *walker = NULL;\n+\tint nongit = 0;\n \n-\tsetup_git_directory();\n+\tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\n \t\tfprintf(stderr, \"Remote needed\\n\");\n \t\treturn 1;\n"},{"id":"126546","messageId":"20091101081904.GC28095@glandium.org","threadId":"21448","inReplyTo":"20091101.010702.527849118592864646._deepfire@feelingofgreen.ru","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2009-11-01T08:19:04Z","receivedAt":"2009-11-01T08:19:04Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sun, Nov 01, 2009 at 01:07:02AM +0300, Samium Gromoff wrote:\n> Good day folks,\n> \n> Attached is the SEGV bugreport I sent to debian.\n> \n> I tried to convince ld to use /usr/lib/debug, via LD_LIBRARY_PATH,\n> and run the thing under gdb 7.0, but it won't use debug libraries\n> for some reason.\n\nYou don't need to point to /usr/lib/debug, for two reasons:\n- The files there are empty nutshells as far as binaries are concerned.\n They only contain the debugging symbols.\n- gdb is going to load files from there automatically for each file it\n has loaded and doesn't contain debugging symbols.\n\nMike\n"},{"id":"126549","messageId":"fabb9a1e0911010646v2043bdb7l9215f1114e9e8385@mail.gmail.com","threadId":"21448","inReplyTo":"7v8weq50pm.fsf@alter.siamese.dyndns.org","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-11-01T14:46:40Z","receivedAt":"2009-11-01T14:46:40Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 1, 2009 at 05:27, Junio C Hamano wrote:\n>  - Should we fix get_helper() in transport-helper.c, instead of touching\n>   ls-remote.c like this patch does?\n\nProbably, yes.\n\n>   This issue really boils down to this question: is it valid for a\n>   transport to have NULL in its remote field, and should all the code\n>   that touch transport be prepared to deal with such a transport\n>   structure?\n\nI think the code in transport-helper should be prepared to deal with\nsuch a field appropriately, since it knows beforehand that only a few\noperations will be performed on such a remote (I'm guessing just the\n'list' command).\n\n>   In general, what should the initial environment for helpers be?  Should\n>   they assume that they have to figure out where the git repository is\n>   themselves (in other words, should they assume they cannot rely on\n>   anything the caller does before they are called?\n\nLet's not duplicate that logic, if git can figure out where we are, it\nshould do so, if it can't, then the helper can't either.\n\n>   Would the caller\n>   generally have done the usual repo discovery (including chdir() to the\n>   toplevel), and there are some set of assumptions they can make?  If so\n>   what are they?\n\nProbably the above, if there is going to be a git repository, we'll\nhave found it, if there isn't one, we're in 'bare' mode.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"126553","messageId":"alpine.LNX.2.00.0911011348190.14365@iabervon.org","threadId":"21448","inReplyTo":"7v8weq50pm.fsf@alter.siamese.dyndns.org","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-11-01T19:43:34Z","receivedAt":"2009-11-01T19:43:34Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 31 Oct 2009, Junio C Hamano wrote:\n\n> Samium Gromoff <_deepfire@feelingofgreen.ru> writes:\n> \n> > Attached is the SEGV bugreport I sent to debian.\n> \n> Thanks for a report.\n> \n> There are two issues.\n> \n> * transport-helper.c::get_helper() assumes that the transport structure\n> always has its remote member filled and it can get name out of it.\n> This is the segv in the report.\n> \n> * Even if we work around the above issue, the helper subprocess (in this\n> case, remote-curl helper) insists on being inside a git repository, To\n> satisfy ls-remote request, you do not have to be in one.\n> \n> Attached is a minimum fix/work around, but this is done without being very\n> familiar with the current assumptions in the codepaths involved.\n> \n> Issues I want area experts to consider before coming up with the final fix\n> are:\n> \n> - Should we fix get_helper() in transport-helper.c, instead of touching\n> ls-remote.c like this patch does?\n> \n> This issue really boils down to this question: is it valid for a\n> transport to have NULL in its remote field, and should all the code\n> that touch transport be prepared to deal with such a transport\n> structure?\n\nI think there's no benefit to allowing NULL for the remote; I think you \ncan always get a struct remote for what you want to access. So it's \nprobably just as well to require it, particularly because, as in the case \nof cmd_ls_remote() below, you'd need a special case to not get a struct \nremote.\n\nIs there any way in which the intended semantics of \"transport_get(NULL, \nurl)\" is not the same as \"transport_get(remote_get(url), url)\"?\n\n(And, in the extended series, I make \"transport_get(remote_get(url), \nNULL)\" also mean the same thing as above, while \"transport_get(NULL, \nNULL)\" is obviously underspecified.)\n\n> - When helping to handle ls-remote request, there is no need for the\n> helper to know anything about the local state. We probably shouldn't\n> even run setup_git_directory_gently() at all in this case. But when\n> helping other kinds of request, the helper does need to know where our\n> repository is.\n> \n> In general, what should the initial environment for helpers be? Should\n> they assume that they have to figure out where the git repository is\n> themselves (in other words, should they assume they cannot rely on\n> anything the caller does before they are called? Would the caller\n> generally have done the usual repo discovery (including chdir() to the\n> toplevel), and there are some set of assumptions they can make? If so\n> what are they?\n\nProbably, the helper should be run with a predicable initial environment, \nsimply because operations that use remote repositories are most often run \nfrom the toplevel of a repo, so people will fail to notice their bugs \nwhich trigger on running from subdirectories. If it's possible for the \ncore git code to enforce regularity here, these oversights aren't bugs. \nThat is, I think people working on helpers will assume that they always \nrun from the environment that they run from the first time they try, \nwhether or not they're supposed to assume this, and we can save a lot of \ntrouble by making it okay.\n\nPerhaps we should actively tell the helper if there is no git repository \n(or, if any git repository we happen to be in is merely coincidental and \nshouldn't affect the helper)? Helpers involving importing will probably \nwant to know they don't have a private refs namespace, private state \ndirectory, etc. even for implementing \"list\" for ls-remote, and it would \nprobably be best to require helper authors to report that they've \nconsidered this possibility before trying to use it.\n\n> builtin-ls-remote.c | 2 +-\n> remote-curl.c | 3 ++-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c\n> index 78a88f7..a8d5613 100644\n> --- a/builtin-ls-remote.c\n> +++ b/builtin-ls-remote.c\n> @@ -86,7 +86,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> \t\t\tpattern[j - i] = p;\n> \t\t}\n> \t}\n> -\tremote = nongit ? NULL : remote_get(dest);\n> +\tremote = remote_get(dest);\n> \tif (remote && !remote->url_nr)\n> \t\tdie(\"remote %s has no configured URL\", dest);\n> \ttransport = transport_get(remote, remote ? remote->url[0] : dest);\n\nYou can also drop the two checks for remote being non-NULL here, since \nit's now always non-NULL...\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index ad6a163..7c83f77 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -81,8 +81,9 @@ int main(int argc, const char **argv)\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tconst char *url;\n> \tstruct walker *walker = NULL;\n> +\tint nongit = 0;\n> \n> -\tsetup_git_directory();\n> +\tsetup_git_directory_gently(&nongit);\n> \tif (argc < 2) {\n> \t\tfprintf(stderr, \"Remote needed\\n\");\n> \t\treturn 1;\n\nDo things like git_path() fail cleanly if there was no git directory? If \nnot, there should probably be tests of nongit on paths that actually need \na git directory, rather than relying on the caller not to do anything that \nneeds a git directory. Even if the core is wise enough not to use \"fetch\" \nif there's nowhere to put the result, this code is likely to be an example \nfor helpers that won't be able to rely on the same analysis.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"126555","messageId":"7vljiqxapw.fsf@alter.siamese.dyndns.org","threadId":"21448","inReplyTo":"alpine.LNX.2.00.0911011348190.14365@iabervon.org","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-01T20:15:55Z","receivedAt":"2009-11-01T20:15:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> On Sat, 31 Oct 2009, Junio C Hamano wrote:\n> ...\n>> Attached is a minimum fix/work around, but this is done without being very\n>> familiar with the current assumptions in the codepaths involved.\n>> \n>> Issues I want area experts to consider before coming up with the final fix\n>> are:\n>> ... \n> I think there's no benefit to allowing NULL for the remote; I think you \n> can always get a struct remote for what you want to access. So it's \n> probably just as well to require it, particularly because, as in the case \n> of cmd_ls_remote() below, you'd need a special case to not get a struct \n> remote.\n>\n> Is there any way in which the intended semantics of \"transport_get(NULL, \n> url)\" is not the same as \"transport_get(remote_get(url), url)\"?\n> (And, in the extended series, I make \"transport_get(remote_get(url), \n> NULL)\" also mean the same thing as above, while \"transport_get(NULL, \n> NULL)\" is obviously underspecified.)\n\nThat was really my question to people who are involved in the transport\nlayer code. I didn't know how transport->url and transport->remote->url\nare intended to relate to each other, for example, and that was why you\nwere on Cc list. In other words, you are the area expert, you tell me ;-)\n\nSverre seemed to think slightly differently; perhaps having worked on the\nforeign vcs interface he has some other input.\n\n>> - When helping to handle ls-remote request, there is no need for the\n>> helper to know anything about the local state. We probably shouldn't\n>> even run setup_git_directory_gently() at all in this case. But when\n>> helping other kinds of request, the helper does need to know where our\n>> repository is.\n>> \n>> In general, what should the initial environment for helpers be? Should\n>> they assume that they have to figure out where the git repository is\n>> themselves (in other words, should they assume they cannot rely on\n>> anything the caller does before they are called? Would the caller\n>> generally have done the usual repo discovery (including chdir() to the\n>> toplevel), and there are some set of assumptions they can make? If so\n>> what are they?\n>\n> Probably, the helper should be run with a predicable initial environment, \n> simply because operations that use remote repositories are most often run \n> from the toplevel of a repo, so people will fail to notice their bugs \n> which trigger on running from subdirectories....\n> Perhaps we should actively tell the helper if there is no git repository \n> (or, if any git repository we happen to be in is merely coincidental and \n> shouldn't affect the helper)? Helpers involving importing will probably \n> want to know they don't have a private refs namespace, private state \n> directory, etc. even for implementing \"list\" for ls-remote, and it would \n> probably be best to require helper authors to report that they've \n> considered this possibility before trying to use it.\n\nI think that is a sane approach.\n\n>> diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c\n>> index 78a88f7..a8d5613 100644\n>> --- a/builtin-ls-remote.c\n>> +++ b/builtin-ls-remote.c\n>> @@ -86,7 +86,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n>> \t\t\tpattern[j - i] = p;\n>> \t\t}\n>> \t}\n>> -\tremote = nongit ? NULL : remote_get(dest);\n>> +\tremote = remote_get(dest);\n>> \tif (remote && !remote->url_nr)\n>> \t\tdie(\"remote %s has no configured URL\", dest);\n>> \ttransport = transport_get(remote, remote ? remote->url[0] : dest);\n>\n> You can also drop the two checks for remote being non-NULL here, since \n> it's now always non-NULL...\n\nYou are probably right; I didn't even look when I did the above.\n\n>> diff --git a/remote-curl.c b/remote-curl.c\n>> index ad6a163..7c83f77 100644\n>> --- a/remote-curl.c\n>> +++ b/remote-curl.c\n>> @@ -81,8 +81,9 @@ int main(int argc, const char **argv)\n>> \tstruct strbuf buf = STRBUF_INIT;\n>> \tconst char *url;\n>> \tstruct walker *walker = NULL;\n>> +\tint nongit = 0;\n>> \n>> -\tsetup_git_directory();\n>> +\tsetup_git_directory_gently(&nongit);\n>> \tif (argc < 2) {\n>> \t\tfprintf(stderr, \"Remote needed\\n\");\n>> \t\treturn 1;\n>\n> Do things like git_path() fail cleanly if there was no git directory? If\n> not, there should probably be tests of nongit on paths that actually need \n> a git directory,...\n\nI don't know. Again, you tell me ;-)\n\nIt probably makes sesne as you outlined in the earlier part of your\nresponse for the caller to give a bit more clue to the helper to help\nmaking such a decision.\n"},{"id":"126556","messageId":"alpine.LNX.2.00.0911011504460.14365@iabervon.org","threadId":"21448","inReplyTo":"fabb9a1e0911010646v2043bdb7l9215f1114e9e8385@mail.gmail.com","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-11-01T20:16:20Z","receivedAt":"2009-11-01T20:16:20Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 1 Nov 2009, Sverre Rabbelier wrote:\n\n> Heya,\n> \n> On Sun, Nov 1, 2009 at 05:27, Junio C Hamano wrote:\n> >  - Should we fix get_helper() in transport-helper.c, instead of touching\n> >   ls-remote.c like this patch does?\n> \n> Probably, yes.\n\nIf we change the ls-remote.c case, it becomes impossible for a struct \ntransport to ever have a NULL remote field. And the change to ls-remote \nremoves a special case. I'd go so far as to say that ls-remote.c should \nprovide a struct remote, and transport_get should enforce that there's a \nstruct remote.\n\n\t-Daniel\n*This .sig left intentionally blank*"},{"id":"126557","messageId":"fabb9a1e0911011254j316920e6y63c4f129f7df186d@mail.gmail.com","threadId":"21448","inReplyTo":"alpine.LNX.2.00.0911011504460.14365@iabervon.org","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-11-01T20:54:24Z","receivedAt":"2009-11-01T20:54:24Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 1, 2009 at 21:16, Daniel Barkalow wrote:\n> If we change the ls-remote.c case, it becomes impossible for a struct\n> transport to ever have a NULL remote field. And the change to ls-remote\n> removes a special case. I'd go so far as to say that ls-remote.c should\n> provide a struct remote, and transport_get should enforce that there's a\n> struct remote.\n\nIf that is the case (that we can eliminate the only special case), I\nagree that we should fix it there, where it will be the least effort.\nI got the impression from Junio's original post that there are\nmultiple places that would have to be fixed, and I figured that we\nshould fix it where it will be the least amount of effort :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"126560","messageId":"alpine.LNX.2.00.0911011518590.14365@iabervon.org","threadId":"21448","inReplyTo":"7vljiqxapw.fsf@alter.siamese.dyndns.org","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-11-01T21:19:34Z","receivedAt":"2009-11-01T21:19:34Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 1 Nov 2009, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> > On Sat, 31 Oct 2009, Junio C Hamano wrote:\n> > ...\n> >> Attached is a minimum fix/work around, but this is done without being very\n> >> familiar with the current assumptions in the codepaths involved.\n> >> \n> >> Issues I want area experts to consider before coming up with the final fix\n> >> are:\n> >> ... \n> > I think there's no benefit to allowing NULL for the remote; I think you \n> > can always get a struct remote for what you want to access. So it's \n> > probably just as well to require it, particularly because, as in the case \n> > of cmd_ls_remote() below, you'd need a special case to not get a struct \n> > remote.\n> >\n> > Is there any way in which the intended semantics of \"transport_get(NULL, \n> > url)\" is not the same as \"transport_get(remote_get(url), url)\"?\n> > (And, in the extended series, I make \"transport_get(remote_get(url), \n> > NULL)\" also mean the same thing as above, while \"transport_get(NULL, \n> > NULL)\" is obviously underspecified.)\n> \n> That was really my question to people who are involved in the transport\n> layer code. I didn't know how transport->url and transport->remote->url\n> are intended to relate to each other, for example, and that was why you\n> were on Cc list. In other words, you are the area expert, you tell me ;-)\n\ntransport->remote->url[...] has all of the URLs for a remote; \ntransport->url has the particular one this struct transport is supposed to \ncontact. At least, that was my intent, but I think some of the code that \nuses struct transports didn't realize that remote_get() will work in any \ncontext where transport_get() will work. (And this might have been broken \nat the time).\n\n> Sverre seemed to think slightly differently; perhaps having worked on the\n> foreign vcs interface he has some other input.\n\nI think we agree that transport-helper.c ought to be able to deal with \nanything transport.c will put together. But I'd also like to tighten what \ntransport.c will put together and I don't think he considered that \npossibility.\n\n> > Do things like git_path() fail cleanly if there was no git directory? If\n> > not, there should probably be tests of nongit on paths that actually need \n> > a git directory,...\n> \n> I don't know. Again, you tell me ;-)\n\nI'm not an expert on that part. But it looks like it misbehaves, returning \n\".git/foo\" even when that path doesn't make sense.\n\n> It probably makes sesne as you outlined in the earlier part of your\n> response for the caller to give a bit more clue to the helper to help\n> making such a decision.\n\nYeah, it's probably worth getting that in at this point. I'll write up \nsome patches.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"126561","messageId":"20091101230442.GA20264@coredump.intra.peff.net","threadId":"21448","inReplyTo":"alpine.LNX.2.00.0911011518590.14365@iabervon.org","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-01T23:04:42Z","receivedAt":"2009-11-01T23:04:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 01, 2009 at 04:19:34PM -0500, Daniel Barkalow wrote:\n\n> > > Do things like git_path() fail cleanly if there was no git directory? If\n> > > not, there should probably be tests of nongit on paths that actually need \n> > > a git directory,...\n> > \n> > I don't know. Again, you tell me ;-)\n> \n> I'm not an expert on that part. But it looks like it misbehaves, returning \n> \".git/foo\" even when that path doesn't make sense.\n\nI will not admit to being an expert in that area, but yes, that is what\nI observed before while looking into some of our weird startup problems.\nWe really have two systems for setting up the environment:\n\n - setup_git_directory, which tries to do everything at the outset (but\n which we don't necessarily run for all commands, and where \"outset\"\n is defined as when the git wrapper handles an actual command, which\n means we sometimes do quite a bit of stuff beforehand)\n\n - some lazy magic in environment.c, mostly setup_git_env. If\n setup_git_directory has been run, this does the right thing because\n it reads GIT_DIR from the environment as set previously. But if not,\n then it can quite often do the wrong thing (as you noticed).\n\nI tried simply ditching the lazy magic, but that doesn't work: there are\nmany cases where setup_git_directory hasn't been run. Moving it to the\nvery beginning doesn't quite work, either. I don't remember the details,\nsadly. It may be that the lazy setup_git_env magic should, rather than\ndoing anything itself, call setup_git_directory if it has not been\ninitialized. But at that point, you might as well setup_git_directory in\nevery program, since just about every one is going to want to look at\ngit_path at some point.\n\nSorry, I know that is vague. Refactoring this area has been on my plate\nfor so long that I have forgotten all the details, and it is such a\nmessy area that I am continually procrastinating on diving back into it.\n;)\n\n-Peff\n"},{"id":"126568","messageId":"alpine.LNX.2.00.0911011829130.14365@iabervon.org","threadId":"21448","inReplyTo":"20091101230442.GA20264@coredump.intra.peff.net","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-11-02T00:59:45Z","receivedAt":"2009-11-02T00:59:45Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 1 Nov 2009, Jeff King wrote:\n\n> On Sun, Nov 01, 2009 at 04:19:34PM -0500, Daniel Barkalow wrote:\n> \n> > > > Do things like git_path() fail cleanly if there was no git directory? If\n> > > > not, there should probably be tests of nongit on paths that actually need \n> > > > a git directory,...\n> > > \n> > > I don't know. Again, you tell me ;-)\n> > \n> > I'm not an expert on that part. But it looks like it misbehaves, returning \n> > \".git/foo\" even when that path doesn't make sense.\n> \n> I will not admit to being an expert in that area, but yes, that is what\n> I observed before while looking into some of our weird startup problems.\n> We really have two systems for setting up the environment:\n> \n> - setup_git_directory, which tries to do everything at the outset (but\n> which we don't necessarily run for all commands, and where \"outset\"\n> is defined as when the git wrapper handles an actual command, which\n> means we sometimes do quite a bit of stuff beforehand)\n> \n> - some lazy magic in environment.c, mostly setup_git_env. If\n> setup_git_directory has been run, this does the right thing because\n> it reads GIT_DIR from the environment as set previously. But if not,\n> then it can quite often do the wrong thing (as you noticed).\n> \n> I tried simply ditching the lazy magic, but that doesn't work: there are\n> many cases where setup_git_directory hasn't been run. Moving it to the\n> very beginning doesn't quite work, either. I don't remember the details,\n> sadly. It may be that the lazy setup_git_env magic should, rather than\n> doing anything itself, call setup_git_directory if it has not been\n> initialized. But at that point, you might as well setup_git_directory in\n> every program, since just about every one is going to want to look at\n> git_path at some point.\n> \n> Sorry, I know that is vague. Refactoring this area has been on my plate\n> for so long that I have forgotten all the details, and it is such a\n> messy area that I am continually procrastinating on diving back into it.\n> ;)\n\nI've been looking at it, just now, and I might try to clean stuff up. The \nproblem I'm running into is that, in some cases, you have to call \nsetup_git_directory_gently(), and it might determine that there is no git \nrepo, but then the various environment functions don't distinguish between \nthe situation where you haven't called it at all and the situation where \nyou called it and determined there to be no answer. Furthermore, a lot of \nfunctions seem to be getting git_path(something), ignoring the fact that \nthere is no repo, and acting like there is a repo that has simply not got \nthe file it is looking for.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"126574","messageId":"7vocnlworb.fsf@alter.siamese.dyndns.org","threadId":"21448","inReplyTo":"fabb9a1e0911011254j316920e6y63c4f129f7df186d@mail.gmail.com","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-02T04:10:16Z","receivedAt":"2009-11-02T04:10:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Sun, Nov 1, 2009 at 21:16, Daniel Barkalow wrote:\n>> If we change the ls-remote.c case, it becomes impossible for a struct\n>> transport to ever have a NULL remote field. And the change to ls-remote\n>> removes a special case. I'd go so far as to say that ls-remote.c should\n>> provide a struct remote, and transport_get should enforce that there's a\n>> struct remote.\n>\n> If that is the case (that we can eliminate the only special case), I\n> agree that we should fix it there, where it will be the least effort.\n> I got the impression from Junio's original post that there are\n> multiple places that would have to be fixed, and I figured that we\n> should fix it where it will be the least amount of effort :).\n\nNo, I genuinely didn't know what Daniel's intention was when a transport\nhas NULL in its remote field. If it is much easier and cleaner not to\nallow such a transport, then let's declare that and fix ls-remote that\nshould be the sole existing caller that used to use such a transport.\n\nThanks for looking into this, both of you.\n"},{"id":"126611","messageId":"20091102192145.GA12718@localhost","threadId":"21448","inReplyTo":"alpine.LNX.2.00.0911011829130.14365@iabervon.org","subject":"Re: Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-11-02T19:21:46Z","receivedAt":"2009-11-02T19:21:46Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Nov 01, 2009 at 07:59:45PM -0500, Daniel Barkalow wrote:\n\n> I've been looking at it, just now, and I might try to clean stuff up. The \n> problem I'm running into is that, in some cases, you have to call \n> setup_git_directory_gently(), and it might determine that there is no git \n> repo, but then the various environment functions don't distinguish between \n> the situation where you haven't called it at all and the situation where \n> you called it and determined there to be no answer. Furthermore, a lot of \n> functions seem to be getting git_path(something), ignoring the fact that \n> there is no repo, and acting like there is a repo that has simply not got \n> the file it is looking for.\n\nIndeed. In particular, you can cause most git commands which take a path\nargument to segfault in GIT_DIR:\n\n$ git grep -- /\nSegmentation fault\n\n$ git ls-files /\nSegmentation fault\n\n$ git show HEAD /\nSegmentation fault\n\n$ git diff HEAD /\nSegmentation fault\n\nYou get the idea. It looks like at some point each of these calls\nget_git_work_tree(), which returns NULL. I was trying to fix it this weekend\nbut got confused by the setup api (to be documented in\nDocumentation/technical/api-setup.txt). I don't think fixing all the callers\nof get_git_work_tree() is the right thing to do. Instead, we should die() if\nthere is no work tree and a different function should be used to check\nwhether or not a work tree exists.\n\nI will try to understand this convoluted codepath when I have gathered the\npatience to do it. But while you're looking at it, maybe you can also take\nthis issue into consideration.\n\nThanks,\nClemens\n"}]}