fix(runtime): free a DLL copy's allocation state at FreeLibrary

CHANGELOG.md

@@ -86,6 +86,13 @@ pull request.
   crash handler now: it reports the crashes in an instrumented DLL's code too, and a DLL's copy
   leaves the process's filter alone. An instrumented DLL that a program without the runtime loads
   reports no crash any more (README, "Crash handler on Windows") (#236).
+- **Memory kept by a DLL that allocates, after it unloaded, on Windows.** The copy of the runtime in
+  a DLL that links it never destroyed its allocation table, nor freed what its block index grew
+  into, its quarantine's queue or the blocks that its quarantine held. A DLL that allocated and
+  freed through its copy left them allocated at each load and unload: about 42 KB per cycle for 64
+  blocks of 64 bytes freed, and 957 KB for 5000 blocks of 16 bytes, in a probe of #227. The copy now
+  frees them when `FreeLibrary` unloads the DLL, after the DLL's exit-time code, and keeps them when
+  the process exits, for the leak report (#227).
 
 ### Tests and CI
 

README.md

@@ -239,7 +239,9 @@ reports the DLL's leaks when the process exits, after the DLL's `atexit` handler
 of its global objects and its destructor functions: from a terminator of the DLL, or, when the DLL
 itself ends the process with `exit()`, when the loader detaches it. That comes before the exit-time
 code of the modules that detach after the DLL. It reports nothing when `FreeLibrary` unloads the
-DLL (#232).
+DLL (#232). It frees its table then, after that exit-time code, with the blocks that its quarantine
+held, which the DLL had released, and what its block index grew into; the blocks that the DLL still
+holds stay allocated (#227).
 
 ## DbgHelp on Windows
 

src/runtime/ct_runtime_quarantine.cpp

@@ -170,6 +170,17 @@ CT_NOINSTR void ct_quarantine_set_limit(size_t bytes)
     ct_release_over_limit();
 }
 
+CT_NOINSTR void ct_quarantine_release_all(void)
+{
+    ct_quarantine_set_limit(0);
+    ct_lock_acquire();
+    std::free(ct_queue);
+    ct_queue = nullptr;
+    ct_queue_capacity = 0;
+    ct_queue_head = 0;
+    ct_lock_release();
+}
+
 CT_NODISCARD CT_NOINSTR size_t ct_quarantine_bytes(void)
 {
     ct_lock_acquire();

src/runtime/ct_runtime_quarantine.h

@@ -32,10 +32,19 @@ struct ct_quarantine_item
 CT_NOINSTR void ct_quarantine_push(const ct_quarantine_item& item);
 
 // The byte limit, read from CT_QUARANTINE_MB until set. Lowering it releases the oldest
-// blocks down to the new limit; 0 releases every block at once. ct_alloc_lock not held.
+// blocks down to the new limit; 0 releases every block at once. Besides tests, only
+// ct_quarantine_release_all sets it, to 0 for good. ct_alloc_lock not held.
 CT_NOINSTR void ct_quarantine_set_limit(size_t bytes);
 CT_NODISCARD CT_NOINSTR size_t ct_quarantine_bytes(void);
 
+// Releases every block, as a limit of 0 does, then frees the queue: for a copy of the runtime
+// whose module unloads (#227). The limit stays 0: CT_QUARANTINE_MB is read only while no limit is
+// set, and nothing in the runtime sets a limit but this function, only tests do. From then on,
+// every block released, whatever the thread, goes to the allocator at once without its freed
+// record: the queue is empty when this function frees it and is not allocated again.
+// ct_alloc_lock not held.
+CT_NOINSTR void ct_quarantine_release_all(void);
+
 // Takes the oldest block out of the quarantine, its freed record dropped, when the allocation
 // table needs the room. False when the quarantine is empty. The caller releases the block, with
 // ct_quarantine_release, once it no longer holds the lock: the block is in no queue and has no

src/runtime/windows/ct_runtime_alloc.cpp

@@ -57,46 +57,53 @@ namespace
     // The table and its lock live until the process ends: .CRT$XT terminators, which the
     // CRT runs after the destructors of every static object, may still release blocks, and
     // the leak report runs among them (#159). The lock needs no constructor nor destructor;
-    // the table is built on first use, in storage nothing destroys.
+    // the table is built on first use, in storage that only the terminator of a DLL's copy
+    // destroys, when FreeLibrary unloads the DLL, and built again for the code of the DLL that
+    // uses it after that terminator (#227).
     SRWLOCK ct_alloc_lock = SRWLOCK_INIT;
 
     using CtAllocTable = std::unordered_map<void*, CtAllocEntry>;
 
-    // Set once the table is built. A copy of the runtime that never built it has nothing to
+    // Set while the table is built. A copy of the runtime that never built it has nothing to
     // report, and building it at process exit would allocate with the loader lock held (#232).
     std::atomic<bool> ct_alloc_table_built{false};
 
+    // ct_alloc_lock held, as for every use of the table. Building the table allocates, which may
+    // reach the runtime's entry points (#197): it is the runtime's own work, whichever use builds
+    // it, a bounds check for one, which holds the lock outside a runtime scope.
     CT_NOINSTR CtAllocTable& ct_alloc_table()
     {
         alignas(CtAllocTable) static unsigned char storage[sizeof(CtAllocTable)];
-        static CtAllocTable* const table = []
+        if (!ct_alloc_table_built.load(std::memory_order_relaxed))
         {
-            CtAllocTable* built = new (storage) CtAllocTable();
+            CtRuntimeScope scope;
+            new (storage) CtAllocTable();
             ct_alloc_table_built.store(true, std::memory_order_release);
-            return built;
-        }();
-        return *table;
+        }
+        return *std::launder(reinterpret_cast<CtAllocTable*>(storage));
     }
 
     constexpr size_t kIndexInitialCapacity = size_t{1} << 12;
 
+    // The index below, and the storage it starts with, in the module's image.
+    alignas(uintptr_t) unsigned char ct_index_storage[ct_block_index_storage_size(
+        kIndexInitialCapacity)];
+    ct_block_index ct_index;
+    bool ct_index_ready = false;
+
 } // namespace
 
 // The index of the blocks the table records, live or freed, by address, for
 // ct_table_lookup_containing (#185). It holds a block while its record is one the lookup sees,
 // and changes with the record, under ct_alloc_lock.
 CT_NODISCARD CT_NOINSTR ct_block_index* ct_alloc_index()
 {
-    alignas(
-        uintptr_t) static unsigned char storage[ct_block_index_storage_size(kIndexInitialCapacity)];
-    static ct_block_index index;
-    static bool ready = false;
-    if (!ready)
+    if (!ct_index_ready)
     {
-        ct_block_index_init(&index, storage, kIndexInitialCapacity);
-        ready = true;
+        ct_block_index_init(&ct_index, ct_index_storage, kIndexInitialCapacity);
+        ct_index_ready = true;
     }
-    return &index;
+    return &ct_index;
 }
 
 namespace
@@ -1361,9 +1368,42 @@ namespace
         }
     }
 
+    // Frees what this copy of the runtime holds for the blocks of its DLL when FreeLibrary unloads
+    // the DLL, from the terminator below: the blocks in its quarantine, which the program had
+    // released, the quarantine's queue, what its block index grew into, and its allocation table.
+    // The DLL's atexit handlers, the destructors of its global objects, its destructor functions
+    // and the terminators of its code, in .CRT$XTU, have run by then. The terminators of later
+    // sections, which a library that the DLL links may add, run after it, then the C runtime's
+    // teardown, which calls no function of the runtime. Code there that goes through the copy
+    // finds a new table (ct_alloc_table), and what that code leaves stays allocated once the DLL
+    // is gone: the new table, a record for each block it allocated and still holds, buckets for
+    // the most blocks it held at once and, when it held more than the 4096 that the block index
+    // holds in the module's image, the storage the index grew into. A block that code releases
+    // leaves no record: the quarantine's limit stays 0, so the block goes to the allocator at once
+    // (ct_quarantine_release_all). The blocks that the DLL still holds stay allocated, without
+    // their records (#227).
+    CT_NOINSTR void ct_release_alloc_state()
+    {
+        // The table and the quarantine release blocks with ct_alloc_lock held, which may reach the
+        // runtime's entry points (#197).
+        CtRuntimeScope scope;
+        if (!ct_alloc_table_built.load(std::memory_order_acquire))
+        {
+            return;
+        }
+        ct_quarantine_release_all();
+        ct_lock_acquire();
+        ct_block_index_destroy(&ct_index);
+        ct_block_index_init(&ct_index, ct_index_storage, kIndexInitialCapacity);
+        ct_alloc_table().~CtAllocTable();
+        ct_alloc_table_built.store(false, std::memory_order_release);
+        ct_lock_release();
+    }
+
     // A DLL's C runtime runs its .CRT$XTY terminators after its atexit handlers, the destructors
     // of its global objects and its destructor functions, when FreeLibrary unloads it and when the
-    // process exits.
+    // process exits. When FreeLibrary unloads it, the copy then frees its allocation state; it
+    // keeps it when the process exits, for the leak report, and while the DLL's own exit() runs.
     CT_NOINSTR void ct_report_dll_leaks_at_exit()
     {
         if (!ct_dll_detaching.load(std::memory_order_acquire))
@@ -1372,6 +1412,10 @@ namespace
             return;
         }
         ct_report_dll_leaks_once();
+        if (ct_dll_unloading_dynamically())
+        {
+            ct_release_alloc_state();
+        }
     }
 } // namespace
 

test/unit/CMakeLists.txt

@@ -169,6 +169,21 @@ if(TARGET ct_instrument_runtime)
     target_compile_definitions(runtime_unit_tests PRIVATE
       CT_RUNTIME_LIBRARY="$<TARGET_FILE:ct_runtime_library>"
       CT_DLLMAIN_LIBRARY="$<TARGET_FILE:ct_dllmain_library>")
+    # A DLL that allocates through its copy of the runtime, which records the blocks and keeps
+    # those it frees in its quarantine (#227); and the same DLL, whose operators new and delete
+    # go through its copy, as the copies of the library's inline functions that an instrumented
+    # DLL compiles may (#197).
+    add_library(ct_allocating_library MODULE reload/ct_allocating_library.cpp)
+    add_library(ct_new_delete_through_runtime_library MODULE reload/ct_allocating_library.cpp
+      reload/ct_new_delete_through_runtime.cpp)
+    foreach(library ct_allocating_library ct_new_delete_through_runtime_library)
+      target_link_libraries(${library} PRIVATE ct_instrument_runtime coretrace_logger)
+      coretrace_force_msvc_runtime(${library})
+      add_dependencies(runtime_unit_tests ${library})
+    endforeach()
+    target_compile_definitions(runtime_unit_tests PRIVATE
+      CT_ALLOCATING_LIBRARY="$<TARGET_FILE:ct_allocating_library>"
+      CT_NEW_DELETE_THROUGH_RUNTIME_LIBRARY="$<TARGET_FILE:ct_new_delete_through_runtime_library>")
     # A DLL's copy of the runtime reports its leaks when the process exits, after the DLL's
     # exit-time code, and says nothing when FreeLibrary unloads the DLL (#232). The program links
     # no runtime, so the report it prints is the DLL's copy's.

test/unit/reload/ct_allocating_library.cpp

@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: Apache-2.0
+//
+// A DLL that links the runtime and allocates through its copy, as an instrumented DLL does: the
+// copy records the blocks in its allocation table and its block index, and keeps the blocks it
+// frees in its quarantine. runtime_vtable_replace_test.cpp loads it, has it allocate, and unloads
+// it, to check what its copy keeps once FreeLibrary has unloaded it (#227). Its allocation trace
+// is off, so that many blocks print nothing.
+#include <cstddef>
+
+extern "C" void* __ct_malloc(size_t size, const char* site);
+extern "C" void __ct_free(void* ptr, const char* site);
+extern "C" void __ct_check_bounds(const void* base, const void* ptr, size_t access_size,
+                                  const char* site, int is_write);
+
+extern "C" int __ct_config_disable_alloc_trace = 1;
+
+// Allocates `count` blocks of `size` bytes through the copy of the runtime, then frees them all.
+extern "C" __declspec(dllexport) void ct_allocating_library_churn(int count, size_t size)
+{
+    auto** blocks = static_cast<void**>(__ct_malloc(sizeof(void*) * count, "allocating:array"));
+    for (int index = 0; index < count; ++index)
+    {
+        blocks[index] = __ct_malloc(size, "allocating:block");
+    }
+    for (int index = 0; index < count; ++index)
+    {
+        __ct_free(blocks[index], "allocating:block");
+    }
+    __ct_free(blocks, "allocating:array");
+}
+
+// A block of `size` bytes allocated through the copy of the runtime, which the DLL never frees.
+extern "C" __declspec(dllexport) void* ct_allocating_library_keep(size_t size)
+{
+    return __ct_malloc(size, "allocating:kept");
+}
+
+namespace
+{
+    int churn_at_unload_count = 0;
+    size_t churn_at_unload_size = 0;
+    bool check_at_unload = false;
+    void (*unload_returned)() = nullptr;
+
+    void atUnload()
+    {
+        if (check_at_unload)
+        {
+            __ct_check_bounds(&check_at_unload, &check_at_unload, sizeof(check_at_unload),
+                              "allocating:check", 0);
+        }
+        if (churn_at_unload_count > 0)
+        {
+            ct_allocating_library_churn(churn_at_unload_count, churn_at_unload_size);
+        }
+        if (unload_returned)
+        {
+            unload_returned();
+        }
+    }
+} // namespace
+
+// Has the DLL allocate `count` blocks of `size` bytes through the copy of the runtime and free them
+// when it unloads, after the terminator of the copy: as code that runs after it would.
+extern "C" __declspec(dllexport) void ct_allocating_library_churn_at_unload(int count, size_t size)
+{
+    churn_at_unload_count = count;
+    churn_at_unload_size = size;
+}
+
+// Has the DLL check an access through the copy of the runtime when it unloads, after the terminator
+// of the copy and before the blocks it allocates then: the first use of the copy's allocation table
+// once the terminator has freed it.
+extern "C" __declspec(dllexport) void ct_allocating_library_check_at_unload()
+{
+    check_at_unload = true;
+}
+
+// Has the DLL call `returned`, a function of the program, at the end of its late terminator. The
+// loader swallows an exception raised while it unloads a DLL: without the call, FreeLibrary returns
+// as if that terminator, or the copy's before it, had not faulted.
+extern "C" __declspec(dllexport) void ct_allocating_library_report_unload(void (*returned)())
+{
+    unload_returned = returned;
+}
+
+// .CRT$XTYZ comes after .CRT$XTY, where the copy places its terminator, and before .CRT$XTZ, which
+// ends the terminators.
+#pragma comment(linker, "/INCLUDE:ct_allocating_library_late_terminator")
+#pragma section(".CRT$XTYZ", read)
+extern "C" __declspec(allocate(".CRT$XTYZ"))
+__attribute__((used)) void (*const ct_allocating_library_late_terminator)() = atUnload;

test/unit/reload/ct_new_delete_through_runtime.cpp

@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: Apache-2.0
+//
+// Linked into a DLL with ct_allocating_library.cpp: the DLL's operators new and delete allocate
+// and free through its copy of the runtime, as the copies of the library's inline functions that
+// an instrumented DLL compiles may, which the linker can keep for the runtime too (#197). The
+// blocks that the runtime allocates and releases for itself, the nodes of its allocation table
+// among them, then reach its entry points.
+#include <cstddef>
+#include <new>
+
+extern "C" void* __ct_malloc(size_t size, const char* site);
+extern "C" void __ct_free(void* ptr, const char* site);
+
+void* operator new(std::size_t size)
+{
+    if (void* ptr = __ct_malloc(size, "new-through-runtime"))
+    {
+        return ptr;
+    }
+    throw std::bad_alloc();
+}
+
+void operator delete(void* ptr) noexcept
+{
+    if (ptr)
+    {
+        __ct_free(ptr, "delete-through-runtime");
+    }
+}
+
+void operator delete(void* ptr, std::size_t) noexcept
+{
+    operator delete(ptr);
+}

test/unit/runtime_quarantine_test.cpp

@@ -132,4 +132,36 @@ namespace
         EXPECT_EQ(__ct_realloc(released, 4 * kBlockSize, "test:realloc"), nullptr);
         EXPECT_EQ(recordedState(released), CT_ENTRY_FREED);
     }
+
+    // Releasing everything empties the quarantine, the freed records of its blocks going with
+    // them, as a copy of the runtime does when its module unloads (#227). A block released
+    // afterwards goes to the allocator at once.
+    TEST_F(QuarantineTest, ReleaseAllEmptiesTheQuarantine)
+    {
+        std::vector<void*> released;
+        for (int i = 0; i < 4; ++i)
+        {
+            void* tracked = __ct_malloc(kBlockSize, "test:tracked");
+            ASSERT_NE(tracked, nullptr);
+            released.push_back(tracked);
+        }
+        for (void* tracked : released)
+        {
+            __ct_free(tracked, "test:free");
+        }
+        ASSERT_EQ(recordedState(released.back()), CT_ENTRY_FREED);
+
+        ct_quarantine_release_all();
+        EXPECT_EQ(ct_quarantine_bytes(), 0u);
+        for (void* tracked : released)
+        {
+            EXPECT_EQ(recordedState(tracked), 0);
+        }
+
+        void* later = __ct_malloc(kBlockSize, "test:tracked");
+        ASSERT_NE(later, nullptr);
+        __ct_free(later, "test:free");
+        EXPECT_EQ(recordedState(later), 0);
+        EXPECT_EQ(ct_quarantine_bytes(), 0u);
+    }
 } // namespace