Enable use-after-return checking in ASAN - #89204
Conversation
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
We're trying to start using the official ASAN headers in dotnet/runtime#89204 and we're hitting issues about where to include them from. This PR updates them to be placed next to clang like they are in a standard clang installation and not in the rootfs like we were doing
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-sanitized |
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… where we need to figure out a good solution.
…umented helper methods.
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-sanitized |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| endif(MSVC) | ||
|
|
||
| if (CLR_CMAKE_ENABLE_SANITIZERS) | ||
| enable_language(C) |
There was a problem hiding this comment.
Is it necessary? IOW, are there cases where C not enabled and control reaches this line?
# Conflicts: # eng/native/configurecompiler.cmake # src/coreclr/pal/src/include/pal/palinternal.h
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "b124ad00077545adbba36ae37eae36516d91c684",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f2d5480e5f1e0ade3e73cdd176151f625d6db278",
"last_reviewed_commit": "b124ad00077545adbba36ae37eae36516d91c684",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f2d5480e5f1e0ade3e73cdd176151f625d6db278",
"last_recorded_worker_run_id": "29672700853",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "b124ad00077545adbba36ae37eae36516d91c684",
"review_id": 4729978160
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: AddressSanitizer's use-after-return detection was globally disabled for CoreCLR native code (-fsanitize-address-use-after-return=never) because the runtime relies heavily on the assumption that C++ locals live on the real machine stack. ASAN's use-after-return instrumentation moves locals onto a heap-allocated "fake stack", which breaks (a) the many frame-chain/stack-pointer ordering assertions that compare Frame* and SP addresses directly, and (b) the exception/activation dispatch code that stashes a CONTEXT in a local and later recovers it from a fixed frame-offset during a stackwalk. This PR (fixes #89133) re-enables use-after-return checking on Debug/Checked builds by teaching the runtime to translate fake-stack addresses back to their real-stack counterparts and by isolating the frame-offset-sensitive functions from ASAN.
Approach: Three coordinated changes: (1) Build: enable use-after-return always on Debug/Checked and keep it never on Release/RelWithDebInfo (size), plus add the clang resource-dir include workaround for the -nostdinc/CMake #19227 interaction. (2) Address translation: add Thread::m_fakeStack (populated in SetStackLimits, present on all DEBUG builds for DAC layout stability), and helpers GetRealStackPointer/IsStackPointerBefore/IsAddressInCurrentStack built on __asan_addr_is_in_fake_stack. Nearly all raw Frame*/SP </>= comparisons in exceptionhandling/stackwalk/frames/threads are rerouted through these helpers so they compare real-stack addresses. (3) Frame-offset isolation: mark the context-holding dispatch functions DISABLE_ASAN and factor HoldContextAndInvokeActivationHandler/CallSEHProcessException into their own noinline frames, and fix PAL_VirtualUnwind to dereference the stored pointer (*(CONTEXT**)) rather than treating the slot as an inline CONTEXT.
Summary: The design is sound and the mechanical rerouting of pointer comparisons through IsStackPointerBefore/GetRealStackPointer is consistent and well-targeted; the fake-stack machinery is correctly guarded to DEBUG && HAS_ADDRESS_SANITIZER while m_fakeStack itself is DEBUG-only to preserve DAC layout parity. I found one likely functional bug in the OSX/MacCatalyst sanitizer-runtime lookup in configurecompiler.cmake (flagged inline) where a renamed variable is captured but the old variable is still returned, which would break locating the dynamic ASAN runtime on Apple targets. The PAL_VirtualUnwind pointer-dereference fix is correct and matches the new HoldContextAndInvokeActivationHandler storage convention. Because this only affects native build/test infrastructure and Debug/Checked diagnostic paths, functional risk to shipping (Release) behavior is low, but the cmake issue should be resolved before merge and validated by the ASAN CI legs. See the inline comment and Detailed Findings below.
Detailed Findings
-
[Likely bug]
configurecompiler.cmakeOSX sanitizer runtime dir (inline on line 197):getSanitizerRuntimeDirectorynow computescompilerRuntimeDirfrom-print-runtime-dirbut still returns${compilerResourceDir}. The runtime-dir result is discarded and the returned path is wrong forfind_library(clang_rt.asan_osx_dynamic). See the inline comment for details and the suggested fix. -
[Observation, non-blocking]
IsAddressInStackreadsm_fakeStackbefore contracts: Inthreads.h, the new#if defined(DEBUG) && defined(HAS_ADDRESS_SANITIZER)block dereferencesm_fakeStackand returns early before the existingLIMITED_METHOD_DAC_CONTRACT/_ASSERTE(m_CacheStackBase != NULL)lines. This is harmless for correctness (the fast-path return is only taken for genuine fake-stack addresses), but placing the contract macro first would keep the method's contract annotation semantics consistent with the rest of the codebase. No change required. -
[Observation] DAC layout parity: Adding
m_fakeStackunder#ifdef DEBUG(rather thanDEBUG && HAS_ADDRESS_SANITIZER) is the right call to keep the runtime and DACThreadlayouts identical, and the comment explains this well.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 85 AIC · ⌖ 11.2 AIC · ⊞ 10K
| OUTPUT_VARIABLE compilerRuntimeDir | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE) | ||
| set(${output} "${compilerResourceDir}/lib/darwin/" PARENT_SCOPE) | ||
| set(${output} "${compilerResourceDir}" PARENT_SCOPE) |
There was a problem hiding this comment.
This looks like a copy/paste bug. The execute_process now runs -print-runtime-dir and captures the result into compilerRuntimeDir, but the set(${output} ...) still uses the old compilerResourceDir variable. As written, getSanitizerRuntimeDirectory returns the resource dir root rather than the runtime dir, and the freshly-computed compilerRuntimeDir is discarded. Previously the code returned ${compilerResourceDir}/lib/darwin/; now it returns the bare resource dir, so find_library(ASAN_RUNTIME clang_rt.asan_osx_dynamic ...) will likely fail to locate the dynamic ASAN runtime on OSX/MacCatalyst. This should almost certainly be set(${output} "${compilerRuntimeDir}" PARENT_SCOPE).
|
I'll come back to this another time |
Fixes #89133