[v2,0/4] reftable: fix realloc error handling

Message

René Scharfe Dec. 28, 2024, 9:43 a.m. UTC

Changes since v1:
- added unit tests
- explicitly set pointer to NULL on REFTABLE_ALLOC_GROW_OR_NULL failure
  in patch 2; omission found by unit test

  reftable: avoid leaks on realloc error
  reftable: fix allocation count on realloc error
  reftable: handle realloc error in parse_names()
  t-reftable-merged: handle realloc errors

 reftable/basics.c                | 14 +++-----
 reftable/basics.h                | 41 ++++++++++++++++++-----
 reftable/block.c                 | 10 +++---
 reftable/pq.c                    |  2 +-
 reftable/record.c                | 12 +++----
 reftable/stack.c                 |  8 +++--
 reftable/writer.c                |  5 +--
 t/unit-tests/t-reftable-basics.c | 56 ++++++++++++++++++++++++++++++++
 t/unit-tests/t-reftable-merged.c |  4 +--
 9 files changed, 116 insertions(+), 36 deletions(-)

Range-diff against v1:
1:  b41547720d ! 1:  b3cad92038 reftable: avoid leaks on realloc error
    @@ reftable/writer.c: static int writer_flush_nonempty_block(struct reftable_writer
      	if (!w->index)
      		return REFTABLE_OUT_OF_MEMORY_ERROR;

    +
    + ## t/unit-tests/t-reftable-basics.c ##
    +@@ t/unit-tests/t-reftable-basics.c: static int integer_needle_lesseq(size_t i, void *_args)
    + 	return args->needle <= args->haystack[i];
    + }
    +
    ++static void *realloc_stub(void *p UNUSED, size_t size UNUSED)
    ++{
    ++	return NULL;
    ++}
    ++
    + int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
    + {
    + 	if_test ("binary search with binsearch works") {
    +@@ t/unit-tests/t-reftable-basics.c: int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
    + 		check_int(in, ==, out);
    + 	}
    +
    ++	if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
    ++		int *arr = NULL;
    ++		size_t alloc = 0, old_alloc;
    ++
    ++		REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
    ++		check(arr != NULL);
    ++		check_uint(alloc, >=, 1);
    ++		arr[0] = 42;
    ++
    ++		old_alloc = alloc;
    ++		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
    ++		check(arr != NULL);
    ++		check_uint(alloc, >, old_alloc);
    ++		arr[alloc - 1] = 42;
    ++
    ++		old_alloc = alloc;
    ++		reftable_set_alloc(malloc, realloc_stub, free);
    ++		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
    ++		check(arr == NULL);
    ++		check_uint(alloc, ==, 0);
    ++		reftable_set_alloc(malloc, realloc, free);
    ++
    ++		reftable_free(arr);
    ++	}
    ++
    + 	return test_done();
    + }
2:  bde2f0e4a5 ! 2:  62a1042825 reftable: fix allocation count on realloc error
    @@ reftable/basics.h: char *reftable_strdup(const char *str);
     -		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); \
    ++		REFTABLE_FREE_AND_NULL(x); \
      		alloc = 0; \
     +	} else { \
     +		alloc = reftable_alloc_grow_or_null_alloc; \
      	} \
      } while (0)

    +
    + ## t/unit-tests/t-reftable-basics.c ##
    +@@ t/unit-tests/t-reftable-basics.c: int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
    + 		check_int(in, ==, out);
    + 	}
    +
    ++	if_test ("REFTABLE_ALLOC_GROW works") {
    ++		int *arr = NULL, *old_arr;
    ++		size_t alloc = 0, old_alloc;
    ++
    ++		check(!REFTABLE_ALLOC_GROW(arr, 1, alloc));
    ++		check(arr != NULL);
    ++		check_uint(alloc, >=, 1);
    ++		arr[0] = 42;
    ++
    ++		old_alloc = alloc;
    ++		old_arr = arr;
    ++		reftable_set_alloc(malloc, realloc_stub, free);
    ++		check(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
    ++		check(arr == old_arr);
    ++		check_uint(alloc, ==, old_alloc);
    ++
    ++		old_alloc = alloc;
    ++		reftable_set_alloc(malloc, realloc, free);
    ++		check(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
    ++		check(arr != NULL);
    ++		check_uint(alloc, >, old_alloc);
    ++		arr[alloc - 1] = 42;
    ++
    ++		reftable_free(arr);
    ++	}
    ++
    + 	if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
    + 		int *arr = NULL;
    + 		size_t alloc = 0, old_alloc;
3:  7c9f044813 = 3:  ed1a292622 reftable: handle realloc error in parse_names()
4:  3d9493f48a = 4:  916032657e t-reftable-merged: handle realloc errors
--
2.47.1

Comments

Patrick Steinhardt Dec. 30, 2024, 7:25 a.m. UTC | #1

On Sat, Dec 28, 2024 at 10:43:41AM +0100, René Scharfe wrote:
> Changes since v1:
> - added unit tests
> - explicitly set pointer to NULL on REFTABLE_ALLOC_GROW_OR_NULL failure
>   in patch 2; omission found by unit test
> 
>   reftable: avoid leaks on realloc error
>   reftable: fix allocation count on realloc error
>   reftable: handle realloc error in parse_names()
>   t-reftable-merged: handle realloc errors

I think this version is good enough for now. I'm not particularly happy
about the split we have with the reftable reallocators now, but don't
think that the series really is to blame for that as it simply fixes a
preexisting issue. We can iterate on this in the future.

Thanks!

Patrick