fix(runtime): drop an unmapped page's record as the runtime's own work

CHANGELOG.md

@@ -93,6 +93,13 @@ pull request.
   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).
+- **Unmapping a recorded page, on Windows.** The runtime's `munmap` dropped the record of a
+  mapping that it had recorded with its allocation table's lock held, outside the runtime's own
+  work. When the release of that record came back to the runtime's entry points, as through the
+  copies of the library's inline functions that an instrumented module compiles (#197), it waited
+  for that lock, which its own thread held, and the unmap never returned: a test DLL whose
+  operators new and delete go through the runtime hung in each of the four Windows jobs. `munmap`
+  now drops the record as the runtime's own work (#243).
 
 ### Tests and CI
 

src/runtime/windows/ct_runtime_alloc.cpp

@@ -1202,6 +1202,9 @@ extern "C"
     {
         ct_init_env_once();
         (void)len;
+        // Dropping the page's record frees a node of the table with ct_alloc_lock held, which
+        // may reach the runtime's entry points (#197): it is the runtime's own work (#243).
+        CtRuntimeScope scope;
 
         size_t size = 0;
         const char* alloc_site = nullptr;

test/unit/reload/ct_allocating_library.cpp

@@ -3,14 +3,18 @@
 // 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.
+// it, to check what its copy keeps once FreeLibrary has unloaded it (#227), and has it map and
+// unmap a page through its copy (#243). 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" void* __ct_mmap(void* addr, size_t len, int prot, int flags, int fd, size_t offset,
+                           const char* site);
+extern "C" int __ct_munmap(void* addr, size_t len, const char* site);
 
 extern "C" int __ct_config_disable_alloc_trace = 1;
 
@@ -29,6 +33,20 @@ extern "C" __declspec(dllexport) void ct_allocating_library_churn(int count, siz
     __ct_free(blocks, "allocating:array");
 }
 
+// Maps a page through the copy of the runtime, which records it, and unmaps it, which drops the
+// page's record. 0 once unmapped, 1 when the mapping failed, -1 when the unmapping did.
+extern "C" __declspec(dllexport) int ct_allocating_library_map_and_unmap()
+{
+    constexpr size_t kPageSize = 4096;
+    constexpr int kReadWrite = 0x1 | 0x2; // PROT_READ | PROT_WRITE, which Windows does not define.
+    void* page = __ct_mmap(nullptr, kPageSize, kReadWrite, 0, -1, 0, "allocating:page");
+    if (page == reinterpret_cast<void*>(-1))
+    {
+        return 1;
+    }
+    return __ct_munmap(page, kPageSize, "allocating:page");
+}
+
 // 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)
 {

test/unit/runtime_vtable_replace_test.cpp

@@ -775,6 +775,45 @@ namespace
                     testing::ExitedWithCode(0), "");
     }
 
+    // Loads `library` and has it map a page through its copy of the runtime and unmap it. Exits
+    // with 0 once unmapped, 2 when the DLL, its function or the watchdog are missing, 3 when the
+    // mapping or the unmapping failed, and 7 when they have not returned within 20 seconds, as when
+    // the copy waits on a lock that its own thread holds.
+    [[noreturn]] void mapAndUnmap(const char* library)
+    {
+        startWatchdog();
+        HMODULE module = LoadLibraryA(library);
+        if (!module)
+        {
+            std::exit(2);
+        }
+        auto map_and_unmap = reinterpret_cast<int (*)()>(
+            GetProcAddress(module, "ct_allocating_library_map_and_unmap"));
+        if (!map_and_unmap)
+        {
+            std::exit(2);
+        }
+        std::exit(map_and_unmap() == 0 ? 0 : 3);
+    }
+
+    // The copy of a DLL that maps and unmaps a page through it records the page, then drops its
+    // record.
+    TEST(DllAllocStateDeathTest, ADllThatMapsAPageThroughItsCopyUnmapsIt)
+    {
+        GTEST_FLAG_SET(death_test_style, "threadsafe");
+        EXPECT_EXIT(mapAndUnmap(CT_ALLOCATING_LIBRARY), testing::ExitedWithCode(0), "");
+    }
+
+    // Dropping the page's record frees a node of the copy's table with the table's lock held. The
+    // node comes back to the copy's entry points through the DLL's operator delete: the copy drops
+    // the record as its own work, so that the release does not wait on that lock (#197, #243).
+    TEST(DllAllocStateDeathTest, ADllWhoseOperatorDeleteFreesThroughItsCopyUnmapsAPage)
+    {
+        GTEST_FLAG_SET(death_test_style, "threadsafe");
+        EXPECT_EXIT(mapAndUnmap(CT_NEW_DELETE_THROUGH_RUNTIME_LIBRARY), testing::ExitedWithCode(0),
+                    "");
+    }
+
     // The line `label` of the first box of `site`, empty without one.
     std::string lineOf(const std::string& boxes, const char* site, const char* label)
     {