Skip to content

Commit 942b834

Browse files
Jarred-SumnerJarred-Sumner
authored
Merge pull request #30 from oven-sh/jarred/free-null-before-init
page-map: free(NULL) before init must not fault (glibc 2.44 startup segfault)
2 parents fd265aa + 7ac561a commit 942b834

3 files changed

Lines changed: 64 additions & 4 deletions

File tree

‎CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -877,7 +877,7 @@ if (MI_BUILD_TESTS)
877877
enable_testing()
878878

879879
# static link tests
880-
set(mi_static_tests api api-fill stress-heaps stress-subprocs stress heap-mt heap-teardown heap-delete-race heap-churn heap-aba fork-user-heap snapshot prof prof-adversarial purge-zero park-handoff)
880+
set(mi_static_tests api api-fill stress-heaps stress-subprocs stress heap-mt heap-teardown heap-delete-race heap-churn heap-aba fork-user-heap snapshot prof prof-adversarial purge-zero park-handoff free-before-init)
881881
if(NOT (MI_DEBUG_TSAN OR MI_TRACK_ASAN OR MI_DEBUG_UBSAN))
882882
list(APPEND mi_static_tests thp-optout) # counts madvise calls by interposing it, which a sanitizer runtime does first
883883
endif()

‎src/page-map.c‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,13 +214,17 @@ mi_decl_nodiscard mi_decl_export bool mi_is_in_heap_region(const void* p) mi_att
214214
// A 2-level page map
215215
#define MI_PAGE_MAP_SUB_SIZE (MI_PAGE_MAP_SUB_COUNT * sizeof(mi_page_t*))
216216

217-
// Use an initial empty page map so `free(NULL)` works even if mimalloc is not yet initialized (issue #1341)
218-
static mi_page_map_t mi_page_map_empty = {
217+
// Use an initial empty page map so `free(NULL)` works even if mimalloc is not yet initialized (issue #1341).
218+
// The first submap must be a real (all-NULL) table: `_mi_unchecked_ptr_page` indexes `submaps[0][0]` for
219+
// `p==NULL` without checking the submap, so a NULL entry here makes `free(NULL)` before initialization
220+
// fault at address 0 (glibc 2.44's `__newlocale` does exactly that from the loader, before any constructor).
221+
static mi_page_t* mi_submap_empty[MI_PAGE_MAP_SUB_COUNT]; // zero-initialized: every lookup yields no page
222+
static mi_page_map_t mi_page_map_empty = {
219223
MI_ATOMIC_VAR_INIT(1),
220224
sizeof(mi_page_map_t),
221225
MI_MEMID_STATIC,
222226
MI_LOCK_INITIALIZER,
223-
{ MI_ATOMIC_VAR_INIT(NULL) }
227+
{ MI_ATOMIC_VAR_INIT(mi_submap_empty) }
224228
};
225229

226230
mi_decl_hidden mi_decl_cache_align _Atomic(mi_page_map_t*) __mi_page_map = MI_ATOMIC_VAR_INIT(&mi_page_map_empty);

‎test/test-free-before-init.c‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/* ----------------------------------------------------------------------------
2+
Copyright (c) Microsoft Research, Daan Leijen
3+
This is free software; you can redistribute it and/or modify it under the
4+
terms of the MIT license.
5+
-----------------------------------------------------------------------------*/
6+
7+
/* `free(NULL)` must work before mimalloc has initialized (upstream issue #1341).
8+
9+
glibc 2.44's `__newlocale` calls `free(NULL)` from the dynamic loader, before any constructor
10+
of the executable has run. With `malloc` overridden that lands in `mi_free`, which looks `p`
11+
up in the page map without a NULL check: `_mi_unchecked_ptr_page` reads `submaps[0][0]`. The
12+
initial (static) page map has to carry a real all-NULL submap at index 0; with a NULL submap
13+
the lookup faults at address 0 and the process dies before `main()`.
14+
15+
On ELF the call is made from `.preinit_array`, which the loader runs before every
16+
`.init_array` entry of the executable and its libraries -- the same point in process startup
17+
as the glibc call. Elsewhere a plain constructor is the closest available approximation (its
18+
order relative to mimalloc's own constructor is not guaranteed). */
19+
20+
#include <stdio.h>
21+
#include <stdlib.h>
22+
#include <mimalloc.h>
23+
24+
static int calls_before_init = 0;
25+
26+
static void free_null_before_init(void) {
27+
free(NULL); // reaches mi_free only when malloc is overridden
28+
mi_free(NULL); // always reaches the page-map lookup
29+
calls_before_init++;
30+
}
31+
32+
#if defined(__ELF__)
33+
__attribute__((section(".preinit_array"), used))
34+
static void (*mi_test_preinit)(void) = &free_null_before_init;
35+
#elif defined(__GNUC__) || defined(__clang__)
36+
__attribute__((constructor))
37+
static void free_null_before_init_ctor(void) { free_null_before_init(); }
38+
#endif
39+
40+
int main(void) {
41+
if (calls_before_init != 1) {
42+
printf("test-free-before-init: FAILED, the pre-init hook ran %d times\n", calls_before_init);
43+
return 1;
44+
}
45+
// the real page map replaced the static one: allocation and free still work
46+
void* p = mi_malloc(64);
47+
if (p == NULL) {
48+
printf("test-free-before-init: FAILED, mi_malloc returned NULL after the pre-init free\n");
49+
return 1;
50+
}
51+
mi_free(p);
52+
free(NULL);
53+
mi_free(NULL);
54+
printf("test-free-before-init: ok\n");
55+
return 0;
56+
}

0 commit comments

Comments
 (0)