t-reftable-basics: allow for malloc to be #defined

[dscho]

This is a fix for one of the many issues that force me to delay Git for Windows v2.48.0-rc2 until I can increase my confidence via thorough testing.

The patch is based on rs/reftable-realloc-errors. Sadly, the patch fails the PR build, but then the base branch fails in the same way.

Cc: René Scharfe [email protected]

[dscho]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1848/dscho/reftable-tests-should-allow-malloc-to-be-defined-v1

To fetch this version to local tag pr-1848/dscho/reftable-tests-should-allow-malloc-to-be-defined-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1848/dscho/reftable-tests-should-allow-malloc-to-be-defined-v1

[gitgitgadget]

On the Git mailing list, René Scharfe wrote (reply to this):

Am 08.01.25 um 17:00 schrieb Johannes Schindelin via GitGitGadget:
> From: Johannes Schindelin <[email protected]>
>
> As indicated by the `#undef malloc` line in `reftable/basics.h`, it is
> quite common to use allocators other than the default one by defining
> `malloc` constants and friends.
>
> This pattern is used e.g. in Git for Windows, which uses the powerful
> and performant `mimalloc` allocator.
>
> Furthermore, in `reftable/basics.c` this `#undef malloc` is
> _specifically_ disabled by virtue of defining the
> `REFTABLE_ALLOW_BANNED_ALLOCATORS` constant before including
> `reftable/basic.h`, to ensure that such a custom allocator is also used
> in the reftable code.
>
> However, in 8db127d43f5b (reftable: avoid leaks on realloc error,
> 2024-12-28) and in 2cca185e8517 (reftable: fix allocation count on
> realloc error, 2024-12-28), `reftable_set_alloc()` function calls were
> introduced that pass `malloc`, `realloc` and `free` function pointers as
> parameters _after_ `reftable/basics.h` ensured that they were no longer
> `#define`d. This would override the custom allocator and re-set it to
> the default allocator provided by, say, libc or MSVCRT.
>
> This causes problems because those calls happen after the initial
> allocator has already been used to initialize an array, which is
> subsequently resized using the overridden default `realloc()` allocator.
>
> You cannot mix and match allocators like that, which leads to a
> `STATUS_HEAP_CORRUPTION` (C0000374) on Windows, and when running this
> unit test through shell and/or `prove` (which only support 7-bit status
> codes), it surfaces as exit code 127.
>
> It is actually unnecessary to use those function pointers to
> `malloc`/`realloc`/`free`, though: The `reftable` code goes out of its
> way to fall back to the initial allocator when passing `NULL` parameters
> instead. So let's do that instead of causing heap corruptions.

Ugh.  That makes a lot of sense.  Sorry for the trouble! :-/

>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>     t-reftable-basics: allow for malloc to be #defined
>
>     This is a fix for one of the many issues that force me to delay Git for
>     Windows v2.48.0-rc2 until I can increase my confidence via thorough
>     testing.
>
>     The patch is based on rs/reftable-realloc-errors. Sadly, the patch fails
>     the PR build
>     [https://github.com/gitgitgadget/git/actions/runs/12672507500/job/35316720255],
>     but then the base branch fails in the same way
>     [https://github.com/gitgitgadget/git/actions/runs/12533205564/job/34952668803].
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1848%2Fdscho%2Freftable-tests-should-allow-malloc-to-be-defined-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1848/dscho/reftable-tests-should-allow-malloc-to-be-defined-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1848
>
>  t/unit-tests/t-reftable-basics.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c
> index 990dc1a2445..1d640b280f9 100644
> --- a/t/unit-tests/t-reftable-basics.c
> +++ b/t/unit-tests/t-reftable-basics.c
> @@ -157,13 +157,13 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
>
>  		old_alloc = alloc;
>  		old_arr = arr;
> -		reftable_set_alloc(malloc, realloc_stub, free);
> +		reftable_set_alloc(NULL, realloc_stub, NULL);
>  		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);
> +		reftable_set_alloc(NULL, NULL, NULL);
>  		check(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
>  		check(arr != NULL);
>  		check_uint(alloc, >, old_alloc);
> @@ -188,11 +188,11 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
>  		arr[alloc - 1] = 42;
>
>  		old_alloc = alloc;
> -		reftable_set_alloc(malloc, realloc_stub, free);
> +		reftable_set_alloc(NULL, realloc_stub, NULL);
>  		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
>  		check(arr == NULL);
>  		check_uint(alloc, ==, 0);
> -		reftable_set_alloc(malloc, realloc, free);
> +		reftable_set_alloc(NULL, NULL, NULL);

Using NULL also captures the intent to set the default allocator better.

>
>  		reftable_free(arr);
>  	}
>
> base-commit: 1e781209284eb5952e153339f45bf0c1555e78bb

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> -		reftable_set_alloc(malloc, realloc_stub, free);
> +		reftable_set_alloc(NULL, realloc_stub, NULL);

Nice.  By setting it to NULL, we force the use of whichever "malloc"
is in effect, and thanks to the way reftable_malloc() is written, we
do not even have to be able to take the address of "malloc" ;-)

Will fast-track down to 'master'.

Thanks.

[gitgitgadget]

This patch series was integrated into seen via git@6f1b764.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

René Scharfe <[email protected]> writes:

>> It is actually unnecessary to use those function pointers to
>> `malloc`/`realloc`/`free`, though: The `reftable` code goes out of its
>> way to fall back to the initial allocator when passing `NULL` parameters
>> instead. So let's do that instead of causing heap corruptions.
>
> Ugh.  That makes a lot of sense.  Sorry for the trouble! :-/

Thanks for a quick Ack.

[gitgitgadget]

This patch series was integrated into seen via git@532dd8c.

[gitgitgadget]

This patch series was integrated into next via git@5efe7e2.

[gitgitgadget]

This patch series was integrated into seen via git@a60673e.

[gitgitgadget]

This patch series was integrated into master via git@a60673e.

[gitgitgadget]

This patch series was integrated into next via git@3f8e2ee.

[gitgitgadget]

Closed via a60673e.