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

From: Junio C Hamano <junkio@cox.net>
Date: Dec 26, 2005, 19:10 UTC
Message-ID: <7vzmmnisix.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0512261916440.8435@wbgn013.biozentrum.uni-wuerzburg.de>

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

> This is the result of a relatively quick audit of the source code. There
> might still be a few odd places lurking out there, but I am quite certain
> I caught most if not all.

Thanks.

Show 14 quoted lines

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

Show 5 quoted lines

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

Show 10 quoted lines

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

Show 13 quoted lines

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

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

 - 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;
        }