[win/asan] Don't intercept memset etc. in ntdll #120397
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Hans Wennborg (zmodem) ChangesWhen ntdll was added to the list of of "interesting DLLs" list (in However, after 42cdfbc, This causes a problem when ntdll-internal functions like Since we never intended to intercept ntdll's memset etc., the simplest Fixes #114793 Full diff: https://github.com/llvm/llvm-project/pull/120397.diff 1 Files Affected:
diff --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp
index a5897274521e92..094953d3544fba 100644
--- a/compiler-rt/lib/interception/interception_win.cpp
+++ b/compiler-rt/lib/interception/interception_win.cpp
@@ -1177,8 +1177,7 @@ static void **InterestingDLLsAvailable() {
"libc++.dll", // libc++
"libunwind.dll", // libunwind
# endif
- // NTDLL should go last as it exports some functions that we should
- // override in the CRT [presumably only used internally].
+ // NTDLL must go last as it gets special treatment in OverrideFunction.
"ntdll.dll",
NULL
};
@@ -1281,9 +1280,22 @@ uptr InternalGetProcAddress(void *module, const char *func_name) {
bool OverrideFunction(
const char *func_name, uptr new_func, uptr *orig_old_func) {
+ static const char *kNtDllIgnore[] = {
+ "memcmp", "memcpy", "memmove", "memset"
+ };
+
bool hooked = false;
void **DLLs = InterestingDLLsAvailable();
for (size_t i = 0; DLLs[i]; ++i) {
+ if (DLLs[i + 1] == nullptr) {
+ // This is the last DLL, i.e. NTDLL. It exports some functions that
+ // we only want to override in the CRT.
+ for (const char *ignored : kNtDllIgnore) {
+ if (strcmp(func_name, ignored) == 0)
+ return hooked;
+ }
+ }
+
uptr func_addr = InternalGetProcAddress(DLLs[i], func_name);
if (func_addr &&
OverrideFunction(func_addr, new_func, orig_old_func)) {
|
You can test this locally with the following command:git-clang-format --diff 90968794e26709957d49dd660e4e453235d393e8 8959e640613d189bb0aad09905e8fe534f5fc3ba --extensions cpp -- compiler-rt/lib/interception/interception_win.cppView the diff from clang-format here.diff --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp
index bd85c50a08..ef19d0d3cf 100644
--- a/compiler-rt/lib/interception/interception_win.cpp
+++ b/compiler-rt/lib/interception/interception_win.cpp
@@ -217,8 +217,10 @@ static int _strcmp(const char *s1, const char *s2) {
while (true) {
unsigned c1 = *s1;
unsigned c2 = *s2;
- if (c1 != c2) return (c1 < c2) ? -1 : 1;
- if (c1 == 0) break;
+ if (c1 != c2)
+ return (c1 < c2) ? -1 : 1;
+ if (c1 == 0)
+ break;
s1++;
s2++;
}
@@ -1174,25 +1176,23 @@ bool OverrideFunction(
static void **InterestingDLLsAvailable() {
static const char *InterestingDLLs[] = {
- "kernel32.dll",
- "msvcr100d.dll", // VS2010
- "msvcr110d.dll", // VS2012
- "msvcr120d.dll", // VS2013
- "vcruntime140d.dll", // VS2015
- "ucrtbased.dll", // Universal CRT
- "msvcr100.dll", // VS2010
- "msvcr110.dll", // VS2012
- "msvcr120.dll", // VS2013
- "vcruntime140.dll", // VS2015
- "ucrtbase.dll", // Universal CRT
+ "kernel32.dll",
+ "msvcr100d.dll", // VS2010
+ "msvcr110d.dll", // VS2012
+ "msvcr120d.dll", // VS2013
+ "vcruntime140d.dll", // VS2015
+ "ucrtbased.dll", // Universal CRT
+ "msvcr100.dll", // VS2010
+ "msvcr110.dll", // VS2012
+ "msvcr120.dll", // VS2013
+ "vcruntime140.dll", // VS2015
+ "ucrtbase.dll", // Universal CRT
# if (defined(__MINGW32__) && defined(__i386__))
- "libc++.dll", // libc++
- "libunwind.dll", // libunwind
+ "libc++.dll", // libc++
+ "libunwind.dll", // libunwind
# endif
- // NTDLL must go last as it gets special treatment in OverrideFunction.
- "ntdll.dll",
- NULL
- };
+ // NTDLL must go last as it gets special treatment in OverrideFunction.
+ "ntdll.dll", NULL};
static void *result[ARRAY_SIZE(InterestingDLLs)] = { 0 };
if (!result[0]) {
for (size_t i = 0, j = 0; InterestingDLLs[i]; ++i) {
@@ -1292,9 +1292,7 @@ uptr InternalGetProcAddress(void *module, const char *func_name) {
bool OverrideFunction(
const char *func_name, uptr new_func, uptr *orig_old_func) {
- static const char *kNtDllIgnore[] = {
- "memcmp", "memcpy", "memmove", "memset"
- };
+ static const char *kNtDllIgnore[] = {"memcmp", "memcpy", "memmove", "memset"};
bool hooked = false;
void **DLLs = InterestingDLLsAvailable();
|
|
I think this strikes the right balance between hacky and pragmatic, and is in line with @rnk's advice in #114793 (comment) @rnk, @zacklj89, @barcharcraz, @yjugl What do you think? |
|
@zmodem firstly, thanks for the review on #120110 and tagging here! I work on ASan at MS, and issues/conversations like these are helpful. I think it's a reasonable implementation and tradeoff. My initial impression was that there are programs that use If this is preferred over #120110, let me know. |
Are such programs common, and do they avoid the regular CRT for size or other reasons? And do we know if they use ASan? I think it would be fair to say that ASan only supports programs using the regular CRT (https://learn.microsoft.com/en-us/cpp/sanitizers/asan-building?view=msvc-170#fsanitizeaddress-compiler-option kind of says so already: "The code produced with this option works with static and dynamic CRTs (for example, /MD, /MDd, /MT, and /MTd).") If we can make that assumption, I think we should intercept ntdll as little as possible, and then I think my patch is preferable. |
I think they're uncommon or nearly(?) nonexistent.
Sounds good to me! |
|
For Mozilla, Nonetheless this patch sounds like a reasonable compromise to me. I got progressively convinced that it's probably just best to not have any instrumentation for these two For the record here is a summary of my own attempts at fixing this issue:
#120110 by @zacklj89 is another attempt at force-committing the shadow memory, this time in a smarter way since it only does that for callers that live in As far as I am concerned, the correct way forward for Mozilla is probably to stop relying on |
| // This is the last DLL, i.e. NTDLL. It exports some functions that | ||
| // we only want to override in the CRT. | ||
| for (const char *ignored : kNtDllIgnore) { | ||
| if (strcmp(func_name, ignored) == 0) |
There was a problem hiding this comment.
Is there some internal_strcmp function we should be calling instead in order to avoid libc re-entrancy, like hitting the ASan strcmp interceptor, for example?
| // we only want to override in the CRT. | ||
| for (const char *ignored : kNtDllIgnore) { | ||
| if (strcmp(func_name, ignored) == 0) | ||
| return hooked; |
There was a problem hiding this comment.
Nit, let's just break or continue so the reader doesn't have to think about what hooked is, we just hit the shared return codepath.
There was a problem hiding this comment.
It's a nested loop, I think returning is the most straight-forward.
Probably the consequence of how things are ordered on the linker command line. No specific intent there, just happenstance. |
|
It sounds like we have consensus on going with this approach. I'll push the button. |
|
This patch seems to have broken asan, when running on top of msvcrt.dll (rather than UCRT); this came up when trying to build a release of 20.1.0 RC 1 with msvcrt.dll, at https://github.com/mstorsjo/llvm-mingw/actions/runs/13123676548/job/36623780321. Whenever a process is started, it errors out with this message: Given the list of potential DLLs that are attempted to intercept at llvm-project/compiler-rt/lib/interception/interception_win.cpp Lines 1175 to 1195 in 69ebac7 msvcrt.dll in there. I guess that means that previously, the only case of e.g. memcpy that was intercepted, was from ntdll.dll. For e.g. UCRT builds, we'd also find one in ucrtbase.dll, but here we no longer find any at all.
Intercepting I did try adding For further context; asan wasn't entirely working in I'm wondering what others think about this, which way we should go:
What do e.g. @alvinhochun @bernhardu @cjacek @lazka @mati865 think about that? |
|
@mstorsjo just adding some comments here to help with the conversation.
Presumably this is happening as a result of calls to
Perhaps there should be a runtime option (defaulting off) that allows this, as if I remember correctly Mozilla was having both reentrancy and performance issues as a result of this. FWIW, MSVC behaves very similarly to #120110. |
|
First, this is just venting, I think everyone who works on Windows already knows this and accepts this, but: The Windows C runtime ecosystem is such a nightmare. ⚡🧠⚡ At a really high level, I think the best direction here is to de-escalate the hotpatching wars. Regarding If we want to intentionally support msvcrt.dll, there is prior art for relaxing the CHECK failure in Adding checks for heap ownership on the hot malloc/free codepaths are prohibitively expensive, so we want to minimize those, but these malloc_usable_size APIs are usually much colder and its probably acceptable to add a wide variety of defensive checks there in the name of compatibility and ease of adoption. My assumption is that calls to malloc from /instrumented DSOs/ are going to be very hot, while malloc calls from precompiled DLLs, system or third party, are going to be much colder. If we could find a way to separate the malloc entry points between instrumented and non-instrumented code, that could allow us to make the non-instrumented memory allocator entry points more flexible, meaning we have fewer CHECK failures and more recovery paths. This would all be worth doing if it makes future Windows updates smoother. |
|
My personal stance regarding msvcrt doesn't change - there is no real reason to use it when building for any somewhat modern Windows. |
I'm not entirely sure; surprisingly enough, if I include the same change (to intercept functions in
TBH, I'm not really sure if it is worth it. And as @rnk says:
Yeah - ASAN is already very complex, so I also agree that we should be wary of adding any extra complexity for something so unimportant as this target. Especially as it already is known to not really be entirely working; see e.g. mstorsjo/llvm-mingw#224 for a user case. I guess it's mostly surprising that I have been able to include this case in my smoke tests and having those tests passing up until now. FWIW, to judge the tradeoff a bit better, to what extent it is worth spending effort on it, I wanted to have some sort of measurement on how well it was working before; was it working up to 95% of cases (then perhaps it would be worth spending some more effort on keeping it alive), or did it only manage to work for trivial cases? I tried running the compiler-rt testsuite with such a toolchain. In mingw UCRT builds, I get clean runs. With msvcrt, I got the following results: So ubsan is mostly working, only a curious handful of testcases that seem to fail for some reason. Asan has got a much higher percentage of tests failing, and I had to disable the gtest based unit tests entirely as they crashed to the point of not getting any json output, so the lit test runner couldn't produce a summary. So therefore, I guess I'll work around this in my smoke tests, to stop trying to use asan in msvcrt builds. And I'll consider entirely stripping out the asan libraries from those toolchains, so users get a clearer linker error, rather than more obscure runtime errors. |
Asan doesn't really run properly on msvcrt.dll; skip testing it in that configuration. See discussion on llvm/llvm-project#120397 and #224 for context around this.
When ntdll was added to the list of of "interesting DLLs" list (in
d58230b), the intention was not to
intercept the "mini CRT" functions it exports. OverrideFunction would
only intercept the first function it found when searching the list of
DLLs, and ntdll was put last in that list.
However, after 42cdfbc,
OverrideFunction intercepts all matching functions in those DLLs. As
a side-effect, the runtime would now intercept functions like memset
etc. also in ntdll.
This causes a problem when ntdll-internal functions like
RtlDispatchException call the intercepted memset, which tries to
inspect uncommitted shadow memory, raising an exception, and getting
stuck in that loop until the stack overflows.
Since we never intended to intercept ntdll's memset etc., the simplest
fix seems to be to actively ignore ntdll when intercepting those
functions.
Fixes #114793