Re: regression in 92392b4

From: Shawn O. Pearce <spearce@spearce.org>
Date: Jul 23, 2008, 00:41 UTC
Message-ID: <20080723004108.GB14668@spearce.org>
In-Reply-To: <alpine.DEB.1.00.0807230033000.8986@racer>

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 15 quoted lines

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

...

Show 12 quoted lines

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

-- 
Shawn.