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

[Allan McRae]

On 02/02/14 20:39, Allan McRae wrote:
> 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.
> 

Reading through the rest of your patches and seeing you are refactoring
some of the other realloc calls into this,  I'd be happy if this was
just a wrapper to realloc that did the memset.   I.e.  You just realloc
to the required value.  Then do the memset if current < required.

Allan