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.

diff --git a/lib/isc/mem.c b/lib/isc/mem.c
index 1b569f042d46a1e98c463455b4198d1c93b88809..22ad423f7e05543a59afabf0b025d05094aa8780 100644 (file)
--- a/lib/isc/mem.c
+++ b/lib/isc/mem.c
@@ -83,12 +83,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 *func;
-       const char *file;
        unsigned int line;
+       const char file[];
 };
 
 typedef ISC_LIST(debuglink_t) debuglist_t;
@@ -198,6 +198,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) {
@@ -221,15 +230,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->func = func;
-       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 +279,7 @@ delete_trace_entry(isc_mem_t *mctx, const void *ptr, size_t size FLARG) {
        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 aaf94c0d7757b76862fa588bd9779306947d42f5..31a6364834103b5570cbea42ffa7e81f07a0d0f0 100644 (file)
--- a/tests/isc/mem_test.c
+++ b/tests/isc/mem_test.c
@@ -370,14 +370,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, __func__, 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_detach(&mctx2);
        isc_stdio_close(f);
@@ -392,13 +408,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 */