Re: [PATCH] Avoid allocating 0 bytes, was Re: [PATCH 4/4] git-compat-util.h: dietlibc-friendly x{malloc,realloc,calloc}

Hi,

On Mon, 26 Dec 2005, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/csum-file.c b/csum-file.c
> > index 5f9249a..2c0f097 100644
> > --- a/csum-file.c
> > +++ b/csum-file.c
> > @@ -121,6 +121,9 @@ int sha1write_compressed(struct sha1file
> >  	unsigned long maxsize;
> >  	void *out;
> >  
> > +	if (size == 0)
> > +		return 0;
> > +
> >  	memset(&stream, 0, sizeof(stream));
> >  	deflateInit(&stream, Z_DEFAULT_COMPRESSION);
> >  	maxsize = deflateBound(&stream, size);
> 
> I think this and the one in sha1_file.c::write_sha1_file() are
> wrong; 0-size input would not result in 0-size output.  Have you
> tested them by actually exercising the codepaths you touched?

No, I did not test them. At least not consciously (I did the whole work on 
Dec 24, when I was lying in bed, ill, trying to distract myself from being 
miserable).

The reason I did it this way: If zlib inflates a buffer of 0 bytes, it 
makes no sense to expect anything than 0 bytes to come out of it, right? 
Therefore, if zlib encounters a deflated buffer of 0 bytes, it should 
inflate it to 0 bytes. So it is a good idea in any case to write out 0 
bytes.

However, thinking of it again, it might break backwards compatibility. But 
I don't think so.

> > diff --git a/diffcore-pathspec.c b/diffcore-pathspec.c
> > index 68fe009..a12337a 100644
> > --- a/diffcore-pathspec.c
> > +++ b/diffcore-pathspec.c
> > @@ -48,10 +48,14 @@ void diffcore_pathspec(const char **path
> 
> diffcore-pathspec and diffcore-order can probably return without
> touching diff_queued_diff if there is no work to be done.

To be honest: I did not completely understand what that code does. 
It seemed obvious, but I got a track record of breaking things. So I was 
cautious. I will not try to modify it myself without a good test case.

> > @@ -353,7 +353,8 @@ static void write_index_file(const char 
> >  {
> >  	struct sha1file *f;
> >  	struct object_entry **sorted_by_sha =
> > -		xcalloc(nr_objects, sizeof(struct object_entry *));
> > +		xcalloc(nr_objects ? nr_objects : 1,
> > +				sizeof(struct object_entry *));
> >  	struct object_entry **list = sorted_by_sha;
> >  	struct object_entry **last = sorted_by_sha + nr_objects;
> >  	unsigned int array[256];
> 
> This can be simplified by sorted_by_sha = list = last = NULL
> when nr_objects == 0 and avoiding qsort; that is what you did in
> pack-objects, I think.

If nr_objects == 0, we could write out a fixed index.

We could alternatively exit(1), because what sense does it make to have an 
index if the pack contains 0 objects? However, this has consequences: 
Every user of index-pack (does "fetch-pack --keep" call it?) has to be 
aware that empty packs are illegal.

Again, I did this so that the tool does not break. I'll try to come up 
with something better soon.

> > @@ -448,8 +449,10 @@ int main(int argc, char **argv)
> >  
> >  	open_pack_file();
> >  	parse_pack_header();
> > -	objects = xcalloc(nr_objects, sizeof(struct object_entry));
> > -	deltas = xcalloc(nr_objects, sizeof(struct delta_entry));
> > +	objects = xcalloc(nr_objects ? nr_objects : 1,
> > +			sizeof(struct object_entry));
> > +	deltas = xcalloc(nr_objects ? nr_objects : 1,
> > +			sizeof(struct delta_entry));
> >  	parse_pack_objects();
> >  	free(deltas);
> >  	write_index_file(index_name, sha1);
> 
> Likewise I suspect.  After all the special case is only when
> reindexing an empty pack ;-).

Right.

> My inclination is to do things in these steps:
> 
>  - apply cleanups that actually simplify the logic, while
>    leaving the ones that you needed to do (size ?  size : 1)
>    unmodified (BTW, must next_in/next_out point at non NULL when
>    avail_in/avail_out are zero?).

I was cautious. Maybe at some point in future, zlib relies on that. But it 
is probably better to try not calling zlib at all in these cases.

>  - change x*alloc like this, once the above is done:
> 
>         static inline void *xmalloc(size_t size)
>         {
>                 void *ret = malloc(size);
>         #ifdef MALLOC_CAN_RETURN_NULL_ON_0SIZE
>                 if (!ret && !size)
>                         ret = malloc(size+1);
>         #endif
>                 if (!ret)
>                         die("Out of memory, malloc failed");
>                 return ret;
>         }

Sounds easiest.

Ciao,
Dscho

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html