From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 27 Feb 2014 22:44:07 GMT
Subject: Re: [PATCH] Use ALLOC_GROW() instead of inline code
Message-ID: <xmqqr46ow4zs.fsf@gitster.dls.corp.google.com>
In-Reply-To: <530FA3E7.8020200@yandex.ru>

"Dmitry S. Dolzhenko" <dmitrys.dolzhenko@yandex.ru> writes:

> diff --git a/dir.c b/dir.c
> index b35b633..72f6e2a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1329,13 +1329,10 @@ static struct path_simplify *create_simplify(const char **pathspec)
>  
>  	for (nr = 0 ; ; nr++) {
>  		const char *match;
> -		if (nr >= alloc) {
> -			alloc = alloc_nr(alloc);
> -			simplify = xrealloc(simplify, alloc * sizeof(*simplify));
> -		}
>  		match = *pathspec++;
>  		if (!match)
>  			break;
> +		ALLOC_GROW(simplify, nr + 1, alloc);
>  		simplify[nr].path = match;
>  		simplify[nr].len = simple_length(match);
>  	}

What follows the post-context of this hunk is a NULL termination of
the array:

	simplify[nr].path = NULL;
        simplify[nr].len = 0;

If the first element in pathspec[] were NULL, we set nr to 0, break
the loop without calling ALLOC_GROW() even once, and try to NULL
terminate simplify[] array after the loop.

Don't we try to store to an unallocated piece of memory with this
change?

> diff --git a/read-cache.c b/read-cache.c
> index 33dd676..e585541 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1466,8 +1462,7 @@ int read_index_from(struct index_state *istate, const char *path)
>  
>  	istate->version = ntohl(hdr->hdr_version);
>  	istate->cache_nr = ntohl(hdr->hdr_entries);
> -	istate->cache_alloc = alloc_nr(istate->cache_nr);
> -	istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));
> +	ALLOC_GROW(istate->cache, istate->cache_nr, istate->cache_alloc);

This being the initial allocation, not growing reallocation, use of
ALLOC_GROW() looks somewhat strange.  I know that an realloc from
NULL ends up being the same as calloc(), but still.