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

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

[Samium Gromoff]

[-- Attachment #1: Type: Text/Plain, Size: 377 bytes --]

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


[-- Attachment #2: Type: Message/Rfc822, Size: 4691 bytes --]

From: Samium Gromoff <deepfire@feelingofgreen.ru>
To: Debian Bug Tracking System <submit@bugs.debian.org>
Subject: git-core: SIGSEGV during {peek,ls}-remote on HTTP remotes.
Date: Sun, 01 Nov 2009 00:38:31 +0300
Message-ID: <20091031213831.3786.15715.reportbug@auriga.deep> (sfid-20091101_003832_561599_A7C90C6C)

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

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

[Junio C Hamano]

Samium Gromoff <_deepfire@feelingofgreen.ru> writes:

> Attached is the SEGV bugreport I sent to debian.

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

diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
index 78a88f7..a8d5613 100644
--- a/builtin-ls-remote.c
+++ b/builtin-ls-remote.c
@@ -86,7 +86,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			pattern[j - i] = p;
 		}
 	}
-	remote = nongit ? NULL : remote_get(dest);
+	remote = remote_get(dest);
 	if (remote && !remote->url_nr)
 		die("remote %s has no configured URL", dest);
 	transport = transport_get(remote, remote ? remote->url[0] : dest);
diff --git a/remote-curl.c b/remote-curl.c
index ad6a163..7c83f77 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -81,8 +81,9 @@ int main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	const char *url;
 	struct walker *walker = NULL;
+	int nongit = 0;
 
-	setup_git_directory();
+	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
 		fprintf(stderr, "Remote needed\n");
 		return 1;

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

[Mike Hommey]

On Sun, Nov 01, 2009 at 01:07:02AM +0300, Samium Gromoff wrote:
> 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.

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

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

[Sverre Rabbelier]

Heya,

On Sun, Nov 1, 2009 at 05:27, Junio C Hamano <gitster@pobox.com> wrote:
>  - Should we fix get_helper() in transport-helper.c, instead of touching
>   ls-remote.c like this patch does?

Probably, yes.

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

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

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

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.

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

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.

-- 
Cheers,

Sverre Rabbelier

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

[Daniel Barkalow]

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

> Samium Gromoff <_deepfire@feelingofgreen.ru> writes:
> 
> > Attached is the SEGV bugreport I sent to debian.
> 
> 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?

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

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

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.

>  builtin-ls-remote.c |    2 +-
>  remote-curl.c       |    3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
> index 78a88f7..a8d5613 100644
> --- a/builtin-ls-remote.c
> +++ b/builtin-ls-remote.c
> @@ -86,7 +86,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  			pattern[j - i] = p;
>  		}
>  	}
> -	remote = nongit ? NULL : remote_get(dest);
> +	remote = remote_get(dest);
>  	if (remote && !remote->url_nr)
>  		die("remote %s has no configured URL", dest);
>  	transport = transport_get(remote, remote ? remote->url[0] : dest);

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

> diff --git a/remote-curl.c b/remote-curl.c
> index ad6a163..7c83f77 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -81,8 +81,9 @@ int main(int argc, const char **argv)
>  	struct strbuf buf = STRBUF_INIT;
>  	const char *url;
>  	struct walker *walker = NULL;
> +	int nongit = 0;
>  
> -	setup_git_directory();
> +	setup_git_directory_gently(&nongit);
>  	if (argc < 2) {
>  		fprintf(stderr, "Remote needed\n");
>  		return 1;

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*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Sat, 31 Oct 2009, Junio C Hamano wrote:
> ...
>> 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:
>> ... 
> 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.)

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.

>>  - 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?
>
> 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....
> 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.

I think that is a sane approach.

>> diff --git a/builtin-ls-remote.c b/builtin-ls-remote.c
>> index 78a88f7..a8d5613 100644
>> --- a/builtin-ls-remote.c
>> +++ b/builtin-ls-remote.c
>> @@ -86,7 +86,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>  			pattern[j - i] = p;
>>  		}
>>  	}
>> -	remote = nongit ? NULL : remote_get(dest);
>> +	remote = remote_get(dest);
>>  	if (remote && !remote->url_nr)
>>  		die("remote %s has no configured URL", dest);
>>  	transport = transport_get(remote, remote ? remote->url[0] : dest);
>
> You can also drop the two checks for remote being non-NULL here, since 
> it's now always non-NULL...

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

>> diff --git a/remote-curl.c b/remote-curl.c
>> index ad6a163..7c83f77 100644
>> --- a/remote-curl.c
>> +++ b/remote-curl.c
>> @@ -81,8 +81,9 @@ int main(int argc, const char **argv)
>>  	struct strbuf buf = STRBUF_INIT;
>>  	const char *url;
>>  	struct walker *walker = NULL;
>> +	int nongit = 0;
>>  
>> -	setup_git_directory();
>> +	setup_git_directory_gently(&nongit);
>>  	if (argc < 2) {
>>  		fprintf(stderr, "Remote needed\n");
>>  		return 1;
>
> 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,...

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.

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

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 620 bytes --]

On Sun, 1 Nov 2009, Sverre Rabbelier wrote:

> Heya,
> 
> On Sun, Nov 1, 2009 at 05:27, Junio C Hamano <gitster@pobox.com> wrote:
> >  - Should we fix get_helper() in transport-helper.c, instead of touching
> >   ls-remote.c like this patch does?
> 
> Probably, yes.

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*

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

[Sverre Rabbelier]

Heya,

On Sun, Nov 1, 2009 at 21:16, Daniel Barkalow <barkalow@iabervon.org> 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.

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 :).

-- 
Cheers,

Sverre Rabbelier

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

[Daniel Barkalow]

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

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Sat, 31 Oct 2009, Junio C Hamano wrote:
> > ...
> >> 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:
> >> ... 
> > 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.)
> 
> 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 ;-)

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

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

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.

> > 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,...
> 
> I don't know.  Again, you tell me ;-)

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.

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

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

	-Daniel
*This .sig left intentionally blank*

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

[Jeff King]

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

> > > 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,...
> > 
> > I don't know.  Again, you tell me ;-)
> 
> 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.

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

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

[Daniel Barkalow]

On Sun, 1 Nov 2009, Jeff King wrote:

> On Sun, Nov 01, 2009 at 04:19:34PM -0500, Daniel Barkalow wrote:
> 
> > > > 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,...
> > > 
> > > I don't know.  Again, you tell me ;-)
> > 
> > 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.
> 
> 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.
> ;)

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*

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

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Sun, Nov 1, 2009 at 21:16, Daniel Barkalow <barkalow@iabervon.org> 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.
>
> 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 :).

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.

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

[Clemens Buchacher]

On Sun, Nov 01, 2009 at 07:59:45PM -0500, Daniel Barkalow 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.

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