| Message ID | pull.1848.git.1736352005578.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d02c37c3e6baf1515e7d1372afa5941b9518ca5b |
| Headers | show |
| Series | t-reftable-basics: allow for `malloc` to be `#define`d | expand |
Am 08.01.25 um 17:00 schrieb Johannes Schindelin via GitGitGadget: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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 <johannes.schindelin@gmx.de> > --- > 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
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> 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.
René Scharfe <l.s.r@web.de> 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.
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); reftable_free(arr); }