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