Re: regression in 92392b4

From: Pierre Habouzit <madcoder@debian.org>
Date: Jul 23, 2008, 01:09 UTC
Message-ID: <20080723010928.GG11831@artemis.madism.org>
In-Reply-To: <20080723004108.GB14668@spearce.org>

On Wed, Jul 23, 2008 at 12:41:08AM +0000, Shawn O. Pearce wrote:

Show 63 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Wed, 23 Jul 2008, Pierre Habouzit wrote:
> > 
> > >   Hi, here is a manual painful down-secting (opposed to a bisect ;P) I
> > > did, since git in next cannot fetch on a regular basis for me. The
> > > culprit seems to be commit  92392b4:
> > > 
> > >     ┌─(1:11)──<~/dev/scm/git 92392b4....>──
> > >     └[artemis] git fetch
> > >     remote: Counting objects: 461, done.
> > >     remote: Compressing objects: 100% (141/141), done.
> > >     remote: Total 263 (delta 227), reused 155 (delta 121)
> > >     Receiving objects: 100% (263/263), 95.55 KiB, done.
> > >     fatal: Out of memory, malloc failed
> > >     fatal: index-pack failed
> > >     [2]    16674 abort (core dumped)  git fetch
> ....
> > 
> > Just a guess:
> ....
> > diff --git a/index-pack.c b/index-pack.c
> > index ac20a46..19c39e5 100644
> > --- a/index-pack.c
> > +++ b/index-pack.c
> > @@ -257,6 +257,7 @@ static void unlink_base_data(struct base_data *c)
> >  		base_cache = NULL;
> >  	if (c->data) {
> >  		free(c->data);
> > +		c->data = NULL;
> >  		base_cache_used -= c->size;
> >  	}
> >  }
> 
> Oh.  This is a pointless assignment.  If you look at any call sites
> for unlink_base_data() you will find that the struct passed in as
> "c" here is going out of scope after unlink_base_data() returns.  In
> no such case does the value of c->data get tested once this free is
> complete.
> 
> We need the if (c->data) guard because we only want to decrement
> base_cache_used if the memory is still allocated.  It may have been
> released earlier, in which case base_cache_used has already been
> decreased and we don't want to double-decrement it.
> 
> This patch makes the code more obvious, so Ack I guess, but it is
> not a solution to Pierre's woes.  Something else is wrong.
> 
> Reading above shows we got a "fatal: Out of memory, malloc failed"
> right before the segfault.  What's odd is we segfaulted after we
> ran out of memory and should have die'd.
> 
> There's at least two bugs in the above output:
> 
> a) index-pack ran out of memory on a small pull (95 KiB).
> b) fetch segfaulted when index-pack failed.
> 
> And this patch will unfortunately address neither of them.  :-|
> 
> I've had a long past couple of days, and another one tomorrow.
> I'm not going to be able to debug this myself until perhaps Thursday
> or Friday.  Sorry.  If nobody beats me to it, I will put this on
> the top of the pile and try to fix it once I get back online at my
> new home.

  Like I said, I had a core that I stupidly lost, but I remember that
the broken malloc was:

    static void *get_data_from_pack(struct object_entry *obj)
    {
	    off_t from = obj[0].idx.offset + obj[0].hdr_size;
	    unsigned long len = obj[1].idx.offset - from;
	    unsigned long rdy = 0;
	    unsigned char *src, *data;
	    z_stream stream;
	    int st;

	    src = xmalloc(len);
            ^^^^^^^^^^^^^^^^^^

  len was horribly big, and outputing obj[1].idx showed that `sha1` had
text in it. I mean something like "could not\r\n     han" IIRC.

  I don't remember the rest of the backtrace, and have stupidly not kept
any ways of reproducing it.