From c1352b79ca910d551d38062882f05adad763ba39 Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Tue, 25 Mar 2025 14:35:49 +0100 Subject: [PATCH] copy __FILE__ when allocating memory When allocating memory under -m trace|record, the __FILE__ pointer is stored, so it can be printed out later in order to figure out in which file an allocation leaked. (among others, like the line number). However named crashes when called with -m record and using a plugin leaking memory. The reason is that plugins are unloaded earlier than when the leaked allocations are dumped (obviously, as it's done as late as possible). In such circumstances, __FILE__ is dangling because the dynamically loaded library (the plugin) is not in memory anymore. Fix the crash by systematically copying the __FILE__ string instead of copying the pointer. Of course, this make each allocation to consume a bit more memory (and longer, as it needs to calculate the length of __FILE__) but this occurs only under -m trace|record debugging flags. In term of unit test, because grepping in C is not fun, and because the whole "syntax" of the dump output is tested in other tests, this simply search for a substring in the whole buffer to make sure the expected allocations are found. (cherry picked from commit 4eb2cd364ac866c924e761c0596abae5e6428146) --- lib/isc/mem.c | 19 +++++++++++++++---- tests/isc/mem_test.c | 33 ++++++++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/lib/isc/mem.c b/lib/isc/mem.c index 8a23b0f46a..592d945b44 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -84,11 +84,12 @@ volatile void *isc__mem_malloc = mallocx; #if ISC_MEM_TRACKLINES typedef struct debuglink debuglink_t; struct debuglink { + size_t dlsize; ISC_LINK(debuglink_t) link; const void *ptr; size_t size; - const char *file; unsigned int line; + const char file[]; }; typedef ISC_LIST(debuglink_t) debuglist_t; @@ -200,6 +201,15 @@ add_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size FLARG) { uint32_t hash; uint32_t idx; + /* + * "file" needs to be copied because it can be part of a dynamically + * loaded plugin which would be unloaded at the time the trace is + * dumped. Storing "file" pointer then leads to a dangling pointer + * dereference and a crash. + */ + size_t filelen = strlen(file) + 1; + size_t dlsize = STRUCT_FLEX_SIZE(dl, file, filelen); + MCTXLOCK(mctx); if ((mctx->debugging & ISC_MEM_DEBUGTRACE) != 0) { @@ -222,14 +232,15 @@ add_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size FLARG) { #endif idx = hash % DEBUG_TABLE_COUNT; - dl = mallocx(sizeof(debuglink_t), mctx->jemalloc_flags); + dl = mallocx(dlsize, mctx->jemalloc_flags); INSIST(dl != NULL); ISC_LINK_INIT(dl, link); dl->ptr = ptr; dl->size = size; - dl->file = file; dl->line = line; + dl->dlsize = dlsize; + strlcpy((char *)dl->file, file, filelen); ISC_LIST_PREPEND(mctx->debuglist[idx], dl, link); mctx->debuglistcnt++; @@ -270,7 +281,7 @@ delete_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size, while (dl != NULL) { if (dl->ptr == ptr) { ISC_LIST_UNLINK(mctx->debuglist[idx], dl, link); - sdallocx(dl, sizeof(*dl), mctx->jemalloc_flags); + sdallocx(dl, dl->dlsize, mctx->jemalloc_flags); goto unlock; } dl = ISC_LIST_NEXT(dl, link); diff --git a/tests/isc/mem_test.c b/tests/isc/mem_test.c index cdf2546656..c8269024ba 100644 --- a/tests/isc/mem_test.c +++ b/tests/isc/mem_test.c @@ -369,14 +369,30 @@ ISC_RUN_TEST_IMPL(isc_mem_recordflag) { char buf[4096], *p; FILE *f; void *ptr; + void *ptr2; + char dummyfilename[2] = "a"; result = isc_stdio_open("mem.output", "w", &f); assert_int_equal(result, ISC_R_SUCCESS); isc_mem_create(&mctx2); + ptr = isc_mem_get(mctx2, 2048); assert_non_null(ptr); + + /* + * This strange allocation verifies that the file name (dummyfilename, + * instead of __FILE__) actually gets copied instead of simply putting + * its pointers in the debuglink struct. This avoids named to crash on + * shutdown if a plugin leaked memory, because the plugin would be + * unloaded, and __FILE__ pointer passed at this time would be dangling. + */ + ptr2 = isc__mem_get(mctx2, 1024, 0, dummyfilename, __LINE__); + assert_non_null(ptr2); + dummyfilename[0] = 'b'; + isc__mem_printactive(mctx2, f); + isc_mem_put(mctx2, ptr2, 1024); isc_mem_put(mctx2, ptr, 2048); isc_mem_destroy(&mctx2); isc_stdio_close(f); @@ -391,13 +407,20 @@ ISC_RUN_TEST_IMPL(isc_mem_recordflag) { buf[sizeof(buf) - 1] = 0; - p = strchr(buf, '\n'); + /* + * Find the allocation of ptr2 and make sure it contains + * "[...] 1024 file a line [...]" and _not_ "[...] 1024 file b [...]", + * which prove the copy + */ + p = strstr(buf, "1024 file a line"); assert_non_null(p); - assert_in_range(p, 0, buf + sizeof(buf) - 3); - assert_memory_equal(p + 2, "ptr ", 4); - p = strchr(p + 1, '\n'); + + /* + * Find the allocation of ptr and make sure it contains "[...] 2048 file + * mem_test.c line [...]" + */ + p = strstr(buf, "2048 file mem_test.c line"); assert_non_null(p); - assert_int_equal(strlen(p), 1); } /* test mem with trace flag */