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)

diff --git a/lib/isc/mem.c b/lib/isc/mem.c
index 8a23b0f46ad34ead79f9f126caccaed445268aa0..592d945b44fb295a89d67d67c5228fbc7c8e1d43 100644 (file)
--- 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 cdf2546656869ac962a916d30736f400fd571ad1..c8269024ba4aa864a6b3bf0e3319c233a415bb09 100644 (file)
--- 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 */