{"thread":{"id":"62769","subject":"[PATCH] t-reftable-basics: allow for `malloc` to be `#define`d","startedAt":"2025-01-08T16:00:09Z","lastAt":"2025-01-08T17:41:34Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"510167","messageId":"pull.1848.git.1736352005578.gitgitgadget@gmail.com","threadId":"62769","inReplyTo":null,"subject":"[PATCH] t-reftable-basics: allow for `malloc` to be `#define`d","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-08T16:00:05Z","receivedAt":"2025-01-08T16:00:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAs indicated by the `#undef malloc` line in `reftable/basics.h`, it is\nquite common to use allocators other than the default one by defining\n`malloc` constants and friends.\n\nThis pattern is used e.g. in Git for Windows, which uses the powerful\nand performant `mimalloc` allocator.\n\nFurthermore, in `reftable/basics.c` this `#undef malloc` is\n_specifically_ disabled by virtue of defining the\n`REFTABLE_ALLOW_BANNED_ALLOCATORS` constant before including\n`reftable/basic.h`, to ensure that such a custom allocator is also used\nin the reftable code.\n\nHowever, in 8db127d43f5b (reftable: avoid leaks on realloc error,\n2024-12-28) and in 2cca185e8517 (reftable: fix allocation count on\nrealloc error, 2024-12-28), `reftable_set_alloc()` function calls were\nintroduced that pass `malloc`, `realloc` and `free` function pointers as\nparameters _after_ `reftable/basics.h` ensured that they were no longer\n`#define`d. This would override the custom allocator and re-set it to\nthe default allocator provided by, say, libc or MSVCRT.\n\nThis causes problems because those calls happen after the initial\nallocator has already been used to initialize an array, which is\nsubsequently resized using the overridden default `realloc()` allocator.\n\nYou cannot mix and match allocators like that, which leads to a\n`STATUS_HEAP_CORRUPTION` (C0000374) on Windows, and when running this\nunit test through shell and/or `prove` (which only support 7-bit status\ncodes), it surfaces as exit code 127.\n\nIt is actually unnecessary to use those function pointers to\n`malloc`/`realloc`/`free`, though: The `reftable` code goes out of its\nway to fall back to the initial allocator when passing `NULL` parameters\ninstead. So let's do that instead of causing heap corruptions.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    t-reftable-basics: allow for malloc to be #defined\n    \n    This is a fix for one of the many issues that force me to delay Git for\n    Windows v2.48.0-rc2 until I can increase my confidence via thorough\n    testing.\n    \n    The patch is based on rs/reftable-realloc-errors. Sadly, the patch fails\n    the PR build\n    [https://github.com/gitgitgadget/git/actions/runs/12672507500/job/35316720255],\n    but then the base branch fails in the same way\n    [https://github.com/gitgitgadget/git/actions/runs/12533205564/job/34952668803].\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1848%2Fdscho%2Freftable-tests-should-allow-malloc-to-be-defined-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1848/dscho/reftable-tests-should-allow-malloc-to-be-defined-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1848\n\n t/unit-tests/t-reftable-basics.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c\nindex 990dc1a2445..1d640b280f9 100644\n--- a/t/unit-tests/t-reftable-basics.c\n+++ b/t/unit-tests/t-reftable-basics.c\n@@ -157,13 +157,13 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \n \t\told_alloc = alloc;\n \t\told_arr = arr;\n-\t\treftable_set_alloc(malloc, realloc_stub, free);\n+\t\treftable_set_alloc(NULL, realloc_stub, NULL);\n \t\tcheck(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n \t\tcheck(arr == old_arr);\n \t\tcheck_uint(alloc, ==, old_alloc);\n \n \t\told_alloc = alloc;\n-\t\treftable_set_alloc(malloc, realloc, free);\n+\t\treftable_set_alloc(NULL, NULL, NULL);\n \t\tcheck(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n \t\tcheck(arr != NULL);\n \t\tcheck_uint(alloc, >, old_alloc);\n@@ -188,11 +188,11 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \t\tarr[alloc - 1] = 42;\n \n \t\told_alloc = alloc;\n-\t\treftable_set_alloc(malloc, realloc_stub, free);\n+\t\treftable_set_alloc(NULL, realloc_stub, NULL);\n \t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n \t\tcheck(arr == NULL);\n \t\tcheck_uint(alloc, ==, 0);\n-\t\treftable_set_alloc(malloc, realloc, free);\n+\t\treftable_set_alloc(NULL, NULL, NULL);\n \n \t\treftable_free(arr);\n \t}\n\nbase-commit: 1e781209284eb5952e153339f45bf0c1555e78bb\n-- \ngitgitgadget\n"},{"id":"510174","messageId":"e842ea8c-4ead-49d0-a48b-0d5f8c4e7c0c@web.de","threadId":"62769","inReplyTo":"pull.1848.git.1736352005578.gitgitgadget@gmail.com","subject":"Re: [PATCH] t-reftable-basics: allow for `malloc` to be `#define`d","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-08T16:36:34Z","receivedAt":"2025-01-08T16:36:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.01.25 um 17:00 schrieb Johannes Schindelin via GitGitGadget:\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> As indicated by the `#undef malloc` line in `reftable/basics.h`, it is\n> quite common to use allocators other than the default one by defining\n> `malloc` constants and friends.\n>\n> This pattern is used e.g. in Git for Windows, which uses the powerful\n> and performant `mimalloc` allocator.\n>\n> Furthermore, in `reftable/basics.c` this `#undef malloc` is\n> _specifically_ disabled by virtue of defining the\n> `REFTABLE_ALLOW_BANNED_ALLOCATORS` constant before including\n> `reftable/basic.h`, to ensure that such a custom allocator is also used\n> in the reftable code.\n>\n> However, in 8db127d43f5b (reftable: avoid leaks on realloc error,\n> 2024-12-28) and in 2cca185e8517 (reftable: fix allocation count on\n> realloc error, 2024-12-28), `reftable_set_alloc()` function calls were\n> introduced that pass `malloc`, `realloc` and `free` function pointers as\n> parameters _after_ `reftable/basics.h` ensured that they were no longer\n> `#define`d. This would override the custom allocator and re-set it to\n> the default allocator provided by, say, libc or MSVCRT.\n>\n> This causes problems because those calls happen after the initial\n> allocator has already been used to initialize an array, which is\n> subsequently resized using the overridden default `realloc()` allocator.\n>\n> You cannot mix and match allocators like that, which leads to a\n> `STATUS_HEAP_CORRUPTION` (C0000374) on Windows, and when running this\n> unit test through shell and/or `prove` (which only support 7-bit status\n> codes), it surfaces as exit code 127.\n>\n> It is actually unnecessary to use those function pointers to\n> `malloc`/`realloc`/`free`, though: The `reftable` code goes out of its\n> way to fall back to the initial allocator when passing `NULL` parameters\n> instead. So let's do that instead of causing heap corruptions.\n\nUgh.  That makes a lot of sense.  Sorry for the trouble! :-/\n\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>     t-reftable-basics: allow for malloc to be #defined\n>\n>     This is a fix for one of the many issues that force me to delay Git for\n>     Windows v2.48.0-rc2 until I can increase my confidence via thorough\n>     testing.\n>\n>     The patch is based on rs/reftable-realloc-errors. Sadly, the patch fails\n>     the PR build\n>     [https://github.com/gitgitgadget/git/actions/runs/12672507500/job/35316720255],\n>     but then the base branch fails in the same way\n>     [https://github.com/gitgitgadget/git/actions/runs/12533205564/job/34952668803].\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1848%2Fdscho%2Freftable-tests-should-allow-malloc-to-be-defined-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1848/dscho/reftable-tests-should-allow-malloc-to-be-defined-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1848\n>\n>  t/unit-tests/t-reftable-basics.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c\n> index 990dc1a2445..1d640b280f9 100644\n> --- a/t/unit-tests/t-reftable-basics.c\n> +++ b/t/unit-tests/t-reftable-basics.c\n> @@ -157,13 +157,13 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n>\n>  \t\told_alloc = alloc;\n>  \t\told_arr = arr;\n> -\t\treftable_set_alloc(malloc, realloc_stub, free);\n> +\t\treftable_set_alloc(NULL, realloc_stub, NULL);\n>  \t\tcheck(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n>  \t\tcheck(arr == old_arr);\n>  \t\tcheck_uint(alloc, ==, old_alloc);\n>\n>  \t\told_alloc = alloc;\n> -\t\treftable_set_alloc(malloc, realloc, free);\n> +\t\treftable_set_alloc(NULL, NULL, NULL);\n>  \t\tcheck(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));\n>  \t\tcheck(arr != NULL);\n>  \t\tcheck_uint(alloc, >, old_alloc);\n> @@ -188,11 +188,11 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n>  \t\tarr[alloc - 1] = 42;\n>\n>  \t\told_alloc = alloc;\n> -\t\treftable_set_alloc(malloc, realloc_stub, free);\n> +\t\treftable_set_alloc(NULL, realloc_stub, NULL);\n>  \t\tREFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);\n>  \t\tcheck(arr == NULL);\n>  \t\tcheck_uint(alloc, ==, 0);\n> -\t\treftable_set_alloc(malloc, realloc, free);\n> +\t\treftable_set_alloc(NULL, NULL, NULL);\n\nUsing NULL also captures the intent to set the default allocator better.\n\n>\n>  \t\treftable_free(arr);\n>  \t}\n>\n> base-commit: 1e781209284eb5952e153339f45bf0c1555e78bb\n\n"},{"id":"510176","messageId":"xmqqseptmdn1.fsf@gitster.g","threadId":"62769","inReplyTo":"pull.1848.git.1736352005578.gitgitgadget@gmail.com","subject":"Re: [PATCH] t-reftable-basics: allow for `malloc` to be `#define`d","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-08T17:14:26Z","receivedAt":"2025-01-08T17:14:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> -\t\treftable_set_alloc(malloc, realloc_stub, free);\n> +\t\treftable_set_alloc(NULL, realloc_stub, NULL);\n\nNice.  By setting it to NULL, we force the use of whichever \"malloc\"\nis in effect, and thanks to the way reftable_malloc() is written, we\ndo not even have to be able to take the address of \"malloc\" ;-)\n\nWill fast-track down to 'master'.\n\nThanks.\n"},{"id":"510178","messageId":"xmqqjzb5mcdv.fsf@gitster.g","threadId":"62769","inReplyTo":"e842ea8c-4ead-49d0-a48b-0d5f8c4e7c0c@web.de","subject":"Re: [PATCH] t-reftable-basics: allow for `malloc` to be `#define`d","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-08T17:41:32Z","receivedAt":"2025-01-08T17:41:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>> It is actually unnecessary to use those function pointers to\n>> `malloc`/`realloc`/`free`, though: The `reftable` code goes out of its\n>> way to fall back to the initial allocator when passing `NULL` parameters\n>> instead. So let's do that instead of causing heap corruptions.\n>\n> Ugh.  That makes a lot of sense.  Sorry for the trouble! :-/\n\nThanks for a quick Ack.\n"}]}