Skip to content

Windows: release VirtualVector reservations only on FreeLibrary, not at process exit - #886

Merged
Matthew Parkinson (mjp41) merged 3 commits into
mainfrom
copilot/issue-885-fix-teardown-order
Oct 1, 2026
Merged

Matthew Parkinson (mjp41) merged 3 commits into
mainfrom
copilot/issue-885-fix-teardown-order

Conversation

Copilot AI commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Since #773, ~VirtualVector releases all pagemap reservations during the C runtime's teardown. For an exe that is too early: fiber-local storage (FLS) callbacks run afterwards, inside ExitProcess (LdrShutdownProcess). That includes Rust's thread-local destructors, which then touch released memory and crash. This PR keeps the #773 behaviour (release when a DLL is unloaded while the process keeps running) and stops releasing at process exit, in a single build with no config flag.

Exit order on Windows

Module C runtime statics destroyed FLS callbacks / other DLLs' detach RtlDllShutdownInProgress() in ~VirtualVector
exe exit() → atexit list later, in ExitProcess FALSE
DLL, process exit its DLL_PROCESS_DETACH around the same time TRUE
DLL, FreeLibrary its DLL_PROCESS_DETACH not affected FALSE

The flag alone can't tell an exe apart from a DLL being unloaded by FreeLibrary, so the module type is checked as well.

Changes

  • pal_windows.h
    • PALWindows::current_module_is_dll() reads IMAGE_FILE_DLL from the __ImageBase headers.
    • RtlDllShutdownInProgress is declared directly. MSDN documents it as exported by ntdll and not declared in SDK headers, so the caller must declare it.
    • ~VirtualVector returns early unless the module is a DLL and shutdown is not in progress:
      if (
        !snmalloc::PALWindows::current_module_is_dll() ||
        RtlDllShutdownInProgress())
        return;
    • Comments on init_seg and reservations are updated to describe this.
  • Linking ntdll
    • MSVC/clang-cl: #pragma comment(lib, "ntdll.lib").
    • MinGW (which ignores the pragma): target_link_libraries(snmalloc INTERFACE ntdll).
    • snmalloc-sys/build.rs: rustc-link-lib=ntdll for both the MSVC and windows-gnu targets.
  • Tests (Windows only, in TESTLIB_ONLY_TESTS)
    • func/fls_teardown reproduces the Rust pattern without Rust. It registers an FLS callback in an exe that touches and frees snmalloc memory at exit.
    • func/dll_teardown uses two helper DLLs:
      • alloc allocates and is unloaded with FreeLibrary; the test checks that the memory was released.
      • touch checks during DLL_PROCESS_DETACH at process exit that the allocation is still readable. If not, it calls TerminateProcess(1), because the loader swallows faults raised in DllMain.
    • Both tests fail without the early return.

For reviewers

  • SDK import library: confirm that the Windows SDK ntdll.lib exports RtlDllShutdownInProgress for both x86 and x64.
  • MinGW pins the DLL: MinGW keeps DLLs that use thread_local destructors loaded. The FreeLibrary release check in dll_teardown is skipped when the DLL is still loaded; confirm that under MSVC it actually runs.
  • Behaviour change: executables no longer return their reservations explicitly; the OS reclaims them at process termination.

Copilot AI and others added 3 commits September 29, 2026 21:13
Co-authored-by: mjp41 <270363+mjp41@users.noreply.github.com>
Co-authored-by: mjp41 <270363+mjp41@users.noreply.github.com>
Co-authored-by: mjp41 <270363+mjp41@users.noreply.github.com>
@mjp41

Copy link
Copy Markdown
Member

Damoy (@DamoyY) would you be able to confirm that this PR fixes the issue for you.

@DamoyY

Copy link
Copy Markdown

Matthew Parkinson (@mjp41) Yes I can confirm that this PR fixes the bug I reported
Thanks for the fix

@mjp41
Matthew Parkinson (mjp41) marked this pull request as ready for review October 1, 2026 06:33
@mjp41
Matthew Parkinson (mjp41) merged commit 1d8dce7 into main Oct 1, 2026
198 checks passed
@mjp41
Matthew Parkinson (mjp41) deleted the copilot/issue-885-fix-teardown-order branch October 1, 2026 06:35
@mjp41

Copy link
Copy Markdown
Member

Neil Monday (@NeilMonday) Sorry, I should have got you to comment before I merged. Can you confirm your use case is not affected. Could you confirm it is okay please. I would like to make a release with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants