[pacman-dev] [PATCH v3 2/5] util: Add _alpm_realloc()

[Allan McRae]

On 31/01/14 02:24, Florian Pritz wrote:
> Signed-off-by: Florian Pritz <bluewind at xinu.at>
> ---
> 
> v3: Implement suggestions from Andrew
> 
> RFC from v2 still holds: is using char for *newdata fine
> or is there any good reason to use uint8_t like systemd does?
> 
>  lib/libalpm/util.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/libalpm/util.h |  1 +
>  2 files changed, 50 insertions(+)
> 
> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> index 150b85e..28b8646 100644
> --- a/lib/libalpm/util.c
> +++ b/lib/libalpm/util.c
> @@ -1287,6 +1287,55 @@ int _alpm_fnmatch(const void *pattern, const void *string)
>  	return fnmatch(pattern, string, 0);
>  }
>  
> +/**
> + * Think of this as realloc with error handling and
> + * automatic growing based on current/required.
> + *
> + * data will point to a memory space with at least required
> + * bytes, likely more. You may want to free unused memory.
> + * If required < current data is returned and nothing happens.
> + *
> + * @param data source memory space
> + * @param current size of the space pointed to be data
> + * @param required size you want
> + * @return new memory if grown; old memory otherwise; NULL on error
> + */
> +void *_alpm_realloc(void **data, size_t *current, const size_t required)
> +{
> +	char *newdata;
> +	size_t newsize = 0;
> +
> +	if(*current >= required) {
> +		return data;
> +	}
> +
> +	if(*current == 0) {
> +		newsize = ALPM_BUFFER_SIZE;
> +	} else if (required > (*current) * 2) {
> +		newsize = required * 2;
> +	} else {
> +		newsize = *current * 2;
> +	}

What is this?   It takes a "required" parameter and then returns
whatever it feels like and ...

> +	/* check for overflows */
> +	if (newsize < required) {
> +		return NULL;
> +	}

... doing that can cause an overflow.

I'd say the correct place for an overflow check should be when
calculating required before calling this function.

> +	newdata = realloc(*data, newsize);
> +	if(!newdata) {
> +		_alpm_alloc_fail(newsize);
> +		return NULL;
> +	}
> +
> +	/* ensure all new memory is zeroed out, in both the initial
> +	 * allocation and later reallocs */
> +	memset(newdata + *current, 0, newsize - *current);

Do we really need that memset?


Anyway, I see no need for this function at all.  Just use plain realloc
and check the return.  You still need to check what this returns anyway.
 It is more work for zero gain, particularly if you just use
ALPM_BUFFER_SIZE at the start and double the size each realloc.  It is
exactly what your formula currently does because "required > (*current)
* 2" can never happen in your filelist reading from mtree function in
patch #5.


As an aside, using packages with mtree files on my system:

47.7% of packages will not need a realloc
15.5% will need 1 realloc
14.3% will need 2
8.6% will need 3
6.6% will need 4
...
I found 1 package on my system needing 8.

That looks fine (maybe decays a bit slow, but it will do).

Allan