diff --git a/CHANGELOG.md b/CHANGELOG.md index 6002e7f..3d1860e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/runtime/windows/ct_runtime_alloc.cpp b/src/runtime/windows/ct_runtime_alloc.cpp index cb743fb..70374a7 100644 --- a/src/runtime/windows/ct_runtime_alloc.cpp +++ b/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; diff --git a/test/unit/reload/ct_allocating_library.cpp b/test/unit/reload/ct_allocating_library.cpp index d61767b..03b9cd6 100644 --- a/test/unit/reload/ct_allocating_library.cpp +++ b/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 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(-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) { diff --git a/test/unit/runtime_vtable_replace_test.cpp b/test/unit/runtime_vtable_replace_test.cpp index ca8da3c..b4708ee 100644 --- a/test/unit/runtime_vtable_replace_test.cpp +++ b/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( + 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) {