On 02/02/14 20:39, Allan McRae wrote: > On 31/01/14 02:24, Florian Pritz wrote: >> Signed-off-by: Florian Pritz <[email protected]> >> --- >> >> 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
