diff mbox series

[2/4] reftable: fix allocation count on realloc error

Message ID 33bbacc7-1727-4efd-9cbf-3c9abfa94d8c@web.de (mailing list archive)
State Superseded
Headers show
Series reftable: fix realloc error handling | expand

Commit Message

René Scharfe Dec. 25, 2024, 6:38 p.m. UTC 
When realloc(3) fails, it returns NULL and keeps the original allocation
intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and
the allocation count variable in that case, simultaneously leaking the
original allocation and misrepresenting the number of storable items.

parse_names() avoids the leak by keeping the original pointer if
reallocation fails, but still increase the allocation count in such a
case as if it succeeded.  That's OK, because the error handling code
just frees everything and doesn't look at names_cap anymore.

reftable_buf_add() does the same, but here it is a problem as it leaves
the reftable_buf in a broken state, with ->alloc being roughly twice as
big as the actually allocated memory, allowing out-of-bounds writes in
subsequent calls.

Reimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts
in sync and still signal failures to callers while avoiding code
duplication in callers.  Make it an expression that evaluates to 0 if no
reallocation is needed or it succeeded and 1 on failure while keeping
the original pointer and allocation counter values.

Adjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for
REFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables
for now.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 reftable/basics.c | 11 +++--------
 reftable/basics.h | 39 ++++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 21 deletions(-)

--
2.47.1

Comments

Patrick Steinhardt Dec. 27, 2024, 10:33 a.m. UTC | #1 
On Wed, Dec 25, 2024 at 07:38:35PM +0100, René Scharfe wrote:
> When realloc(3) fails, it returns NULL and keeps the original allocation
> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and
> the allocation count variable in that case, simultaneously leaking the
> original allocation and misrepresenting the number of storable items.
> 
> parse_names() avoids the leak by keeping the original pointer if
> reallocation fails, but still increase the allocation count in such a
> case as if it succeeded.  That's OK, because the error handling code
> just frees everything and doesn't look at names_cap anymore.
> 
> reftable_buf_add() does the same, but here it is a problem as it leaves
> the reftable_buf in a broken state, with ->alloc being roughly twice as
> big as the actually allocated memory, allowing out-of-bounds writes in
> subsequent calls.
> 
> Reimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts
> in sync and still signal failures to callers while avoiding code
> duplication in callers.  Make it an expression that evaluates to 0 if no
> reallocation is needed or it succeeded and 1 on failure while keeping
> the original pointer and allocation counter values.
> 
> Adjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for
> REFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables
> for now.

Okay, this change goes into the direction I was wondering about with the
preceding commit, good.

> diff --git a/reftable/basics.h b/reftable/basics.h
> index 259f4c274c..fa5d75868b 100644
> --- a/reftable/basics.h
> +++ b/reftable/basics.h
> @@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);
>  #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
>  #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
>  #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
> -#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
> -	do { \
> -		if ((nr) > alloc) { \
> -			alloc = 2 * (alloc) + 1; \
> -			if (alloc < (nr)) \
> -				alloc = (nr); \
> -			REFTABLE_REALLOC_ARRAY(x, alloc); \
> -		} \
> -	} while (0)
> +
> +static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
> +					size_t *allocp)
> +{
> +	void *new_p;
> +	size_t alloc = *allocp * 2 + 1;
> +	if (alloc < nelem)
> +		alloc = nelem;
> +	new_p = reftable_realloc(p, st_mult(elsize, alloc));
> +	if (!new_p)
> +		return p;
> +	*allocp = alloc;
> +	return new_p;
> +}
> +
> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \
> +	(nr) > (alloc) && ( \
> +		(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
> +		(nr) > (alloc) \
> +	) \
> +)

Do we even need this macro? I don't really think it serves much of a
purpose anymore.

Patrick
René Scharfe Dec. 27, 2024, 8:16 p.m. UTC | #2 
Am 25.12.24 um 19:38 schrieb René Scharfe:
> When realloc(3) fails, it returns NULL and keeps the original allocation
> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and
> the allocation count variable in that case, simultaneously leaking the
> original allocation and misrepresenting the number of storable items.
>
> parse_names() avoids the leak by keeping the original pointer if
> reallocation fails, but still increase the allocation count in such a
> case as if it succeeded.  That's OK, because the error handling code
> just frees everything and doesn't look at names_cap anymore.
>
> reftable_buf_add() does the same, but here it is a problem as it leaves
> the reftable_buf in a broken state, with ->alloc being roughly twice as
> big as the actually allocated memory, allowing out-of-bounds writes in
> subsequent calls.
>
> Reimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts
> in sync and still signal failures to callers while avoiding code
> duplication in callers.  Make it an expression that evaluates to 0 if no
> reallocation is needed or it succeeded and 1 on failure while keeping
> the original pointer and allocation counter values.
>
> Adjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for
> REFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables
> for now.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  reftable/basics.c | 11 +++--------
>  reftable/basics.h | 39 ++++++++++++++++++++++++++-------------
>  2 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/reftable/basics.c b/reftable/basics.c
> index 70b1091d14..cd6b39dbe9 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -124,11 +124,8 @@ int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len)
>  	size_t newlen = buf->len + len;
>
>  	if (newlen + 1 > buf->alloc) {
> -		char *reallocated = buf->buf;
> -		REFTABLE_ALLOC_GROW(reallocated, newlen + 1, buf->alloc);
> -		if (!reallocated)
> +		if (REFTABLE_ALLOC_GROW(buf->buf, newlen + 1, buf->alloc))
>  			return REFTABLE_OUT_OF_MEMORY_ERROR;
> -		buf->buf = reallocated;
>  	}
>
>  	memcpy(buf->buf + buf->len, data, len);
> @@ -233,11 +230,9 @@ char **parse_names(char *buf, int size)
>  			next = end;
>  		}
>  		if (p < next) {
> -			char **names_grown = names;
> -			REFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);
> -			if (!names_grown)
> +			if (REFTABLE_ALLOC_GROW(names, names_len + 1,
> +						names_cap))
>  				goto err;
> -			names = names_grown;
>
>  			names[names_len] = reftable_strdup(p);
>  			if (!names[names_len++])
> diff --git a/reftable/basics.h b/reftable/basics.h
> index 259f4c274c..fa5d75868b 100644
> --- a/reftable/basics.h
> +++ b/reftable/basics.h
> @@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);
>  #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
>  #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
>  #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
> -#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
> -	do { \
> -		if ((nr) > alloc) { \
> -			alloc = 2 * (alloc) + 1; \
> -			if (alloc < (nr)) \
> -				alloc = (nr); \
> -			REFTABLE_REALLOC_ARRAY(x, alloc); \
> -		} \
> -	} while (0)
> +
> +static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
> +					size_t *allocp)
> +{
> +	void *new_p;
> +	size_t alloc = *allocp * 2 + 1;
> +	if (alloc < nelem)
> +		alloc = nelem;
> +	new_p = reftable_realloc(p, st_mult(elsize, alloc));
> +	if (!new_p)
> +		return p;
> +	*allocp = alloc;
> +	return new_p;
> +}
> +
> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \
> +	(nr) > (alloc) && ( \
> +		(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
> +		(nr) > (alloc) \
> +	) \
> +)
>
>  #define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \
> -	void *reftable_alloc_grow_or_null_orig_ptr = (x); \
> -	REFTABLE_ALLOC_GROW((x), (nr), (alloc)); \
> -	if (!(x)) { \
> -		reftable_free(reftable_alloc_grow_or_null_orig_ptr); \
> +	size_t reftable_alloc_grow_or_null_alloc = alloc; \
> +	if (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \
> +		reftable_free(x); \
>  		alloc = 0; \

This forgets to set x = NULL, which is bad since that's what callers check. :-O
It was not necessary without this patch, because realloc(3) did it.

> +	} else { \
> +		alloc = reftable_alloc_grow_or_null_alloc; \
>  	} \
>  } while (0)
>
> --
> 2.47.1
>
René Scharfe Dec. 27, 2024, 8:16 p.m. UTC | #3 
Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:
> On Wed, Dec 25, 2024 at 07:38:35PM +0100, René Scharfe wrote:
>
>> diff --git a/reftable/basics.h b/reftable/basics.h
>> index 259f4c274c..fa5d75868b 100644
>> --- a/reftable/basics.h
>> +++ b/reftable/basics.h
>> @@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);
>>  #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
>>  #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
>>  #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))

>> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \
>> +	(nr) > (alloc) && ( \
>> +		(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
>> +		(nr) > (alloc) \
>> +	) \
>> +)
>
> Do we even need this macro? I don't really think it serves much of a
> purpose anymore.

It provides the same value as the *ALLOC_ARRAY macros above: automatic
sizeof handling.  Plus it returns whether the allocation succeeded,
avoiding repetition of its arguments in callers.

René
diff mbox series

Patch

diff --git a/reftable/basics.c b/reftable/basics.c
index 70b1091d14..cd6b39dbe9 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -124,11 +124,8 @@  int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len)
 	size_t newlen = buf->len + len;

 	if (newlen + 1 > buf->alloc) {
-		char *reallocated = buf->buf;
-		REFTABLE_ALLOC_GROW(reallocated, newlen + 1, buf->alloc);
-		if (!reallocated)
+		if (REFTABLE_ALLOC_GROW(buf->buf, newlen + 1, buf->alloc))
 			return REFTABLE_OUT_OF_MEMORY_ERROR;
-		buf->buf = reallocated;
 	}

 	memcpy(buf->buf + buf->len, data, len);
@@ -233,11 +230,9 @@  char **parse_names(char *buf, int size)
 			next = end;
 		}
 		if (p < next) {
-			char **names_grown = names;
-			REFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);
-			if (!names_grown)
+			if (REFTABLE_ALLOC_GROW(names, names_len + 1,
+						names_cap))
 				goto err;
-			names = names_grown;

 			names[names_len] = reftable_strdup(p);
 			if (!names[names_len++])
diff --git a/reftable/basics.h b/reftable/basics.h
index 259f4c274c..fa5d75868b 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -120,22 +120,35 @@  char *reftable_strdup(const char *str);
 #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
 #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
 #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
-#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
-	do { \
-		if ((nr) > alloc) { \
-			alloc = 2 * (alloc) + 1; \
-			if (alloc < (nr)) \
-				alloc = (nr); \
-			REFTABLE_REALLOC_ARRAY(x, alloc); \
-		} \
-	} while (0)
+
+static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
+					size_t *allocp)
+{
+	void *new_p;
+	size_t alloc = *allocp * 2 + 1;
+	if (alloc < nelem)
+		alloc = nelem;
+	new_p = reftable_realloc(p, st_mult(elsize, alloc));
+	if (!new_p)
+		return p;
+	*allocp = alloc;
+	return new_p;
+}
+
+#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \
+	(nr) > (alloc) && ( \
+		(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
+		(nr) > (alloc) \
+	) \
+)

 #define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \
-	void *reftable_alloc_grow_or_null_orig_ptr = (x); \
-	REFTABLE_ALLOC_GROW((x), (nr), (alloc)); \
-	if (!(x)) { \
-		reftable_free(reftable_alloc_grow_or_null_orig_ptr); \
+	size_t reftable_alloc_grow_or_null_alloc = alloc; \
+	if (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \
+		reftable_free(x); \
 		alloc = 0; \
+	} else { \
+		alloc = reftable_alloc_grow_or_null_alloc; \
 	} \
 } while (0)