windows: Enable heap terminate-on-corruption #17916

This PR is currently two separate changes:

Enable heap terminate-on-corruption

This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. This is also done by projects like tor, chromium etc.

Enables the terminate-on-corruption feature. If the heap manager detects an error in any heap used by the process, it calls the Windows Error Reporting service and terminates the process. After a process enables this feature, it cannot be disabled.

More info here.

Remove call to SetProcessDEPPolicy()

DEP is always enabled on 64-bit Windows processes, and SetProcessDEPPolicy() only works when called from a 32-bit process. I've tested that our current usage always fails (as expected) with ERROR_NOT_SUPPORTED.

Please don't add a "Needs gitian build" tag here yet.

Concept ACK

Setting HeapEnableTerminationOnCorruption seems like a sensible idea (trusting that it doesn't generate false positives like some of the debug memory monitoring tools).

But why remove the SetProcessDEPPolicy(PROCESS_DEP_ENABLE); call? It still has an effect on x86 builds. Or is this in anticipation of deprecating x86 support?

Concept ACK: enabling mitigations is good, but it seems PROCESS_DEP_ENABLE should not be removed unless defined(_WIN64)?

Perhaps Chromium's ApplyProcessMitigationsToCurrentProcess(MitigationFlags flags) in win/src/process_mitigations.cc can serve as inspiration for our Windows mitigation strategies.

But why remove the SetProcessDEPPolicy(PROCESS_DEP_ENABLE); call? It still has an effect on x86 builds. Or is this in anticipation of deprecating x86 support?

We stopped shipping 32-bit windows binaries as part of the 0.18.0 release, and have been removing windows 32-bit support from our build system (see #15939, #17756 etc).

can serve as inspiration for our Windows mitigation strategies.

Yep, as mentioned in the PR body, I have been looking at what other projects like tor, chromium etc are doing.

ACK 3d5d7aad269c7afe7e36677d3e76c6579e1b8aba.

If anyone would like to test/verify these changes, here are a couple things you can look at.

Process Explorer

Using Process Explorer, you can inspect Bitcoin Core while it's running and check that DEP is enabled. In the "Image File" tab for the process, you are looking for DEP Status: Enabled (Permanent) along with ASLR: High Entropy, Bottom Up etc. Below is the output for the 0.17.1 32-bit, 0.19.0.1 (release binaries) and 0.19.99.0 (this PR, built using WSL).

dumpbin

You can use dumpbin to dump the header info for the Bitcoin Core executables. You are looking for the DLL Characteristics and NX compatible. For example

dumpbin /headers bitcoind.exe (0.17.1 32-bit)
...
             140 DLL characteristics
                   Dynamic base
                   NX compatible

dumpbin /headers bitcoind.exe (0.19.99.0 this PR)
...
             160 DLL characteristics
                   High Entropy Virtual Addresses
                   Dynamic base
                   NX compatible

Verify ERROR_NOT_SUPPORTED

You could also test that our current call to SetProcessDEPPolicy() fails with ERROR_NOT_SUPPORTED.

diff --git a/src/init.cpp b/src/init.cpp
index dc0f2ce05..cf7d6e89f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -879,6 +879,8 @@ bool AppInitBasicSetup()
 #ifdef WIN32
     // Enable Data Execution Prevention (DEP)
     SetProcessDEPPolicy(PROCESS_DEP_ENABLE);
+    std::cout << "Failed to SetProcessDEPPolicy with: " << GetLastError() << "\n\n";
+    std::abort();
 #endif

ACK 3d5d7aad269c7afe7e36677d3e76c6579e1b8aba