git-log segfault on 00 graft

git-log segfault on 00 graft

[Jan Engelhardt]

Hi,


I was playing a bit with grafts, and actually did this:

  echo '839affa3313011da783b5b8074a5c9805ee8503a 
0000000000000000000000000000000000000000' >.git/info/grafts

running `git log --topo-order` causes a segfault. Yes, I probably 
"should not be doing that", but I think it at least should not
segfault.

Re: git-log segfault on 00 graft

[Johannes Schindelin]

Hi,

On Tue, 4 Mar 2008, Jan Engelhardt wrote:

> I was playing a bit with grafts, and actually did this:
> 
>   echo '839affa3313011da783b5b8074a5c9805ee8503a 
> 0000000000000000000000000000000000000000' >.git/info/grafts
> 
> running `git log --topo-order` causes a segfault. Yes, I probably 
> "should not be doing that", but I think it at least should not
> segfault.

Well, I agree with the first, but not the latter.  grafts are a really 
core and plumbing thing, and if you set it to something nonsensical, I 
think you should expect something like a segmentation fault.

Ciao,
Dscho

Re: git-log segfault on 00 graft

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Tue, 4 Mar 2008, Jan Engelhardt wrote:
> 
> > I was playing a bit with grafts, and actually did this:
> > 
> >   echo '839affa3313011da783b5b8074a5c9805ee8503a 
> > 0000000000000000000000000000000000000000' >.git/info/grafts
> > 
> > running `git log --topo-order` causes a segfault. Yes, I probably 
> > "should not be doing that", but I think it at least should not
> > segfault.
> 
> Well, I agree with the first, but not the latter.  grafts are a really 
> core and plumbing thing, and if you set it to something nonsensical, I 
> think you should expect something like a segmentation fault.

I'm sorry, I don't know where you learned to program Dscho, but
my mentors always taught me that user input should be handled
with care, and SIGSEGV / SIGBUS / SIGILL is not handling with
care!

We tell users to popuate the .git/info/grafts file.  By hand.
Its user input.  We shouldn't segfault over a malformed entry.

-- 
Shawn.

Re: git-log segfault on 00 graft

[Björn Steinbrink]

On 2008.03.04 19:57:42 +0100, Jan Engelhardt wrote:
> Hi,
> 
> 
> I was playing a bit with grafts, and actually did this:
> 
>   echo '839affa3313011da783b5b8074a5c9805ee8503a 
> 0000000000000000000000000000000000000000' >.git/info/grafts
> 
> running `git log --topo-order` causes a segfault. Yes, I probably 
> "should not be doing that", but I think it at least should not
> segfault.

error: Could not read 0000000000000000000000000000000000000000
fatal: revision walk setup failed

That's what I get with a somewhat up-to-date version from master.

Björn

Re: git-log segfault on 00 graft

[Johannes Schindelin]

Hi,

On Wed, 5 Mar 2008, Shawn O. Pearce wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Tue, 4 Mar 2008, Jan Engelhardt wrote:
> > 
> > > I was playing a bit with grafts, and actually did this:
> > > 
> > >   echo '839affa3313011da783b5b8074a5c9805ee8503a 
> > > 0000000000000000000000000000000000000000' >.git/info/grafts
> > > 
> > > running `git log --topo-order` causes a segfault. Yes, I probably 
> > > "should not be doing that", but I think it at least should not 
> > > segfault.
> > 
> > Well, I agree with the first, but not the latter.  grafts are a really 
> > core and plumbing thing, and if you set it to something nonsensical, I 
> > think you should expect something like a segmentation fault.
> 
> I'm sorry, I don't know where you learned to program Dscho, but my 
> mentors always taught me that user input should be handled with care, 
> and SIGSEGV / SIGBUS / SIGILL is not handling with care!

I agree.

> We tell users to popuate the .git/info/grafts file.  By hand.
> Its user input.  We shouldn't segfault over a malformed entry.

Well, I disagree about the user input.  .git/info/grafts can break tons of 
things, just by _existing_.  So you definitely need to know what you are 
doing.

Just inserting random strings into the grafts file is not an option. It is 
not something that we should take pains to catch... just like a Unix 
system does not prevent "rm -rf /" as root.

So again, I do think that a segmentation fault is not good.  But I 
disagree that you have to go to great lengths to prevent a segmentation 
fault when a user is fiddling with internals without even knowing what 
could happen.

IOW in this case, the _user input_ would have better been crafted with 
care, and it was clearly not.

But as has been pointed out, the segfault has been already fixed (most 
likely by one of Martin's patches), so the discussion about this 
particular problem is moot.

Ciao,
Dscho