Fw: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.

Good day folks,

Attached is the SEGV bugreport I sent to debian.

I tried to convince ld to use /usr/lib/debug, via LD_LIBRARY_PATH, and run the thing under gdb 7.0, but it won't use debug libraries for some reason.

regards,
  Samium Gromoff
--
                                 _deepfire-at-feelingofgreen.ru
O< ascii ribbon campaign - stop html mail - www.asciiribbon.org

Package: git-core
Version: 1:1.6.5-1
Severity: important

Basically,

deepfire@auriga:/$ git peek-remote http://common-lisp.net/project/qitab/git/poiu.git Segmentation fault deepfire@auriga:/$ git peek-remote http://www.lichteblau.com/git/atdoc.git Segmentation fault

Strace shows following: [snip] open("/dev/urandom", O_RDONLY) = 3 read(3, "\27\n\7\210!\373\210\265", 8) = 8 close(3) = 0 mprotect(0x7fe055d69000, 16384, PROT_READ) = 0 mprotect(0x7fe055f89000, 4096, PROT_READ) = 0 mprotect(0x7fe0563c2000, 4096, PROT_READ) = 0 munmap(0x7fe0563ae000, 67595) = 0 set_tid_address(0x7fe0563ac7c0) = 4047 set_robust_list(0x7fe0563ac7d0, 0x18) = 0 futex(0x7fff5e3c057c, FUTEX_WAKE_PRIVATE, 1) = 0 futex(0x7fff5e3c057c, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, 1, NULL, 7fe0563ac6f0) = -1 ENOSYS (Function not implemented) rt_sigaction(SIGRTMIN, {0x7fe055d78750, [], SA_RESTORER|SA_SIGINFO, 0x7fe055d819a0}, NULL, 8) = 0 rt_sigaction(SIGRT_1, {0x7fe055d787e0, [], SA_RESTORER|SA_RESTART|SA_SIGINFO, 0x7fe055d819a0}, NULL, 8) = 0 rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0 getrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0 brk(0) = 0x26d6000 brk(0x26f7000) = 0x26f7000 getcwd("/", 4096) = 2 stat(".git", 0x7fff5e3c02d0) = -1 ENOENT (No such file or directory) access(".git/objects", X_OK) = -1 ENOENT (No such file or directory) access("./objects", X_OK) = -1 ENOENT (No such file or directory) chdir("..") = 0 stat(".git", 0x7fff5e3c02d0) = -1 ENOENT (No such file or directory) access(".git/objects", X_OK) = -1 ENOENT (No such file or directory) access("./objects", X_OK) = -1 ENOENT (No such file or directory) chdir("/") = 0 --- SIGSEGV (Segmentation fault) @ 0 (0) --- +++ killed by SIGSEGV +++ Segmentation fault

-- System Information:
Debian Release: squeeze/sid
  APT prefers unstable
  APT policy: (500, 'unstable'), (1, 'experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 2.6.26-2-amd64 (SMP w/2 CPU cores)
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968)
Shell: /bin/sh linked to /bin/dash

Versions of packages git-core depends on: ii libc6 2.10.1-3 GNU C Library: Shared libraries ii libcurl3-gnutls 7.19.5-1.1 Multi-protocol file transfer libra ii libdigest-sha1-perl 2.12-1 NIST SHA-1 message digest algorith ii liberror-perl 0.17-1 Perl module for error/exception ha ii libexpat1 2.0.1-4 XML parsing C library - runtime li ii perl-modules 5.10.1-6 Core Perl modules ii zlib1g 1:1.2.3.3.dfsg-15 compression library - runtime

Versions of packages git-core recommends: ii less 436-1 pager program similar to more ii openssh-client [ssh-client] 1:5.1p1-8 secure shell client, an rlogin/rsh ii patch 2.5.9-5 Apply a diff file to an original ii rsync 3.0.6-1 fast remote file copy program (lik

Versions of packages git-core suggests: pn git-arch <none> (no description available) ii git-cvs 1:1.6.5-1 fast, scalable, distributed revisi ii git-daemon-run 1:1.6.5-1 fast, scalable, distributed revisi pn git-doc <none> (no description available) pn git-email <none> (no description available) ii git-gui 1:1.6.5-1 fast, scalable, distributed revisi ii git-svn 1:1.6.5-1 fast, scalable, distributed revisi ii gitk 1:1.6.5-1 fast, scalable, distributed revisi pn gitweb <none> (no description available)

-- no debconf information

Samium Gromoff <_deepfire@feelingofgreen.ru> writes:

Thanks for a report.

There are two issues.

 * transport-helper.c::get_helper() assumes that the transport structure
   always has its remote member filled and it can get name out of it.
   This is the segv in the report.

 * Even if we work around the above issue, the helper subprocess (in this
   case, remote-curl helper) insists on being inside a git repository,  To
   satisfy ls-remote request, you do not have to be in one.

Attached is a minimum fix/work around, but this is done without being very familiar with the current assumptions in the codepaths involved.

Issues I want area experts to consider before coming up with the final fix are:

 - Should we fix get_helper() in transport-helper.c, instead of touching
   ls-remote.c like this patch does?

   This issue really boils down to this question: is it valid for a
   transport to have NULL in its remote field, and should all the code
   that touch transport be prepared to deal with such a transport
   structure?

 - When helping to handle ls-remote request, there is no need for the
   helper to know anything about the local state.  We probably shouldn't
   even run setup_git_directory_gently() at all in this case.  But when
   helping other kinds of request, the helper does need to know where our
   repository is.

   In general, what should the initial environment for helpers be?  Should
   they assume that they have to figure out where the git repository is
   themselves (in other words, should they assume they cannot rely on
   anything the caller does before they are called?  Would the caller
   generally have done the usual repo discovery (including chdir() to the
   toplevel), and there are some set of assumptions they can make?  If so
   what are they?

 builtin-ls-remote.c |    2 +-
 remote-curl.c       |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

Heya,

On Sun, Nov 1, 2009 at 05:27, Junio C Hamano <gitster@pobox.com> wrote:

Probably, yes.

I think the code in transport-helper should be prepared to deal with such a field appropriately, since it knows beforehand that only a few operations will be performed on such a remote (I'm guessing just the 'list' command).

Let's not duplicate that logic, if git can figure out where we are, it should do so, if it can't, then the helper can't either.

Probably the above, if there is going to be a git repository, we'll have found it, if there isn't one, we're in 'bare' mode.

On Sun, 1 Nov 2009, Sverre Rabbelier wrote:

If we change the ls-remote.c case, it becomes impossible for a struct transport to ever have a NULL remote field. And the change to ls-remote removes a special case. I'd go so far as to say that ls-remote.c should provide a struct remote, and transport_get should enforce that there's a struct remote.

	-Daniel
*This .sig left intentionally blank*

Heya,

On Sun, Nov 1, 2009 at 21:16, Daniel Barkalow <barkalow@iabervon.org> wrote:

If that is the case (that we can eliminate the only special case), I agree that we should fix it there, where it will be the least effort. I got the impression from Junio's original post that there are multiple places that would have to be fixed, and I figured that we should fix it where it will be the least amount of effort :).

Sverre Rabbelier <srabbelier@gmail.com> writes:

No, I genuinely didn't know what Daniel's intention was when a transport has NULL in its remote field. If it is much easier and cleaner not to allow such a transport, then let's declare that and fix ls-remote that should be the sole existing caller that used to use such a transport.

Thanks for looking into this, both of you.

On Sat, 31 Oct 2009, Junio C Hamano wrote:

I think there's no benefit to allowing NULL for the remote; I think you can always get a struct remote for what you want to access. So it's probably just as well to require it, particularly because, as in the case of cmd_ls_remote() below, you'd need a special case to not get a struct remote.

Is there any way in which the intended semantics of "transport_get(NULL, url)" is not the same as "transport_get(remote_get(url), url)"?

(And, in the extended series, I make "transport_get(remote_get(url), NULL)" also mean the same thing as above, while "transport_get(NULL, NULL)" is obviously underspecified.)

Probably, the helper should be run with a predicable initial environment, simply because operations that use remote repositories are most often run from the toplevel of a repo, so people will fail to notice their bugs which trigger on running from subdirectories. If it's possible for the core git code to enforce regularity here, these oversights aren't bugs. That is, I think people working on helpers will assume that they always run from the environment that they run from the first time they try, whether or not they're supposed to assume this, and we can save a lot of trouble by making it okay.

Perhaps we should actively tell the helper if there is no git repository (or, if any git repository we happen to be in is merely coincidental and shouldn't affect the helper)? Helpers involving importing will probably want to know they don't have a private refs namespace, private state directory, etc. even for implementing "list" for ls-remote, and it would probably be best to require helper authors to report that they've considered this possibility before trying to use it.

You can also drop the two checks for remote being non-NULL here, since it's now always non-NULL...

Do things like git_path() fail cleanly if there was no git directory? If not, there should probably be tests of nongit on paths that actually need a git directory, rather than relying on the caller not to do anything that needs a git directory. Even if the core is wise enough not to use "fetch" if there's nowhere to put the result, this code is likely to be an example for helpers that won't be able to rely on the same analysis.

	-Daniel
*This .sig left intentionally blank*

Daniel Barkalow <barkalow@iabervon.org> writes:

That was really my question to people who are involved in the transport layer code. I didn't know how transport->url and transport->remote->url are intended to relate to each other, for example, and that was why you were on Cc list. In other words, you are the area expert, you tell me ;-)

Sverre seemed to think slightly differently; perhaps having worked on the foreign vcs interface he has some other input.

I think that is a sane approach.

You are probably right; I didn't even look when I did the above.

I don't know.  Again, you tell me ;-)

It probably makes sesne as you outlined in the earlier part of your response for the caller to give a bit more clue to the helper to help making such a decision.

On Sun, 1 Nov 2009, Junio C Hamano wrote:

transport->remote->url[...] has all of the URLs for a remote; transport->url has the particular one this struct transport is supposed to contact. At least, that was my intent, but I think some of the code that uses struct transports didn't realize that remote_get() will work in any context where transport_get() will work. (And this might have been broken at the time).

I think we agree that transport-helper.c ought to be able to deal with anything transport.c will put together. But I'd also like to tighten what transport.c will put together and I don't think he considered that possibility.

I'm not an expert on that part. But it looks like it misbehaves, returning ".git/foo" even when that path doesn't make sense.

Yeah, it's probably worth getting that in at this point. I'll write up some patches.

	-Daniel
*This .sig left intentionally blank*

On Sun, Nov 01, 2009 at 04:19:34PM -0500, Daniel Barkalow wrote:

I will not admit to being an expert in that area, but yes, that is what I observed before while looking into some of our weird startup problems. We really have two systems for setting up the environment:

  - setup_git_directory, which tries to do everything at the outset (but
    which we don't necessarily run for all commands, and where "outset"
    is defined as when the git wrapper handles an actual command, which
    means we sometimes do quite a bit of stuff beforehand)

  - some lazy magic in environment.c, mostly setup_git_env. If
    setup_git_directory has been run, this does the right thing because
    it reads GIT_DIR from the environment as set previously. But if not,
    then it can quite often do the wrong thing (as you noticed).

I tried simply ditching the lazy magic, but that doesn't work: there are many cases where setup_git_directory hasn't been run. Moving it to the very beginning doesn't quite work, either. I don't remember the details, sadly. It may be that the lazy setup_git_env magic should, rather than doing anything itself, call setup_git_directory if it has not been initialized. But at that point, you might as well setup_git_directory in every program, since just about every one is going to want to look at git_path at some point.

Sorry, I know that is vague. Refactoring this area has been on my plate for so long that I have forgotten all the details, and it is such a messy area that I am continually procrastinating on diving back into it. ;)

-Peff

On Sun, 1 Nov 2009, Jeff King wrote:

I've been looking at it, just now, and I might try to clean stuff up. The problem I'm running into is that, in some cases, you have to call setup_git_directory_gently(), and it might determine that there is no git repo, but then the various environment functions don't distinguish between the situation where you haven't called it at all and the situation where you called it and determined there to be no answer. Furthermore, a lot of functions seem to be getting git_path(something), ignoring the fact that there is no repo, and acting like there is a repo that has simply not got the file it is looking for.

	-Daniel
*This .sig left intentionally blank*

On Sun, Nov 01, 2009 at 07:59:45PM -0500, Daniel Barkalow wrote:

Indeed. In particular, you can cause most git commands which take a path argument to segfault in GIT_DIR:

$ git grep -- / Segmentation fault

$ git ls-files / Segmentation fault

$ git show HEAD / Segmentation fault

$ git diff HEAD / Segmentation fault

You get the idea. It looks like at some point each of these calls get_git_work_tree(), which returns NULL. I was trying to fix it this weekend but got confused by the setup api (to be documented in Documentation/technical/api-setup.txt). I don't think fixing all the callers of get_git_work_tree() is the right thing to do. Instead, we should die() if there is no work tree and a different function should be used to check whether or not a work tree exists.

I will try to understand this convoluted codepath when I have gathered the patience to do it. But while you're looking at it, maybe you can also take this issue into consideration.

Thanks, Clemens

On Sun, Nov 01, 2009 at 01:07:02AM +0300, Samium Gromoff wrote:

You don't need to point to /usr/lib/debug, for two reasons:
- The files there are empty nutshells as far as binaries are concerned.
  They only contain the debugging symbols.
- gdb is going to load files from there automatically for each file it
  has loaded and doesn't contain debugging symbols.

Mike