[compiler-rt] [win/asan] Don't intercept memset etc. in ntdll (PR #120397)

Thu Feb 6 02:56:29 PST 2025

mstorsjo wrote:

> > I did try adding msvcrt.dll to this list, but that doesn't seem to fix the issue entirely either - I'm ending up with errors like this instead:
> > ==5236==ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned:
> 
> Presumably this is happening as a result of calls to `_Crt*` APIs or msvcrt initialization routines that are dealing with memory that ASan is not aware of that end up making calls to intercepted APIs?

I'm not entirely sure; surprisingly enough, if I include the same change (to intercept functions in `msvcrt.dll`) in an UCRT build, I'm not hitting the same issue, so the issue is at least not triggered by the background use of `msvcrt.dll` in other system DLLs.

> > Reinstate intercepting ntdll.dll, if building for mingw + msvcrt.dll. While it isn't the entirely right thing to do, we should reach the same level of functionality as before
> 
> 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.

TBH, I'm not really sure if it is worth it.

And as @rnk says:

> Regarding `msvcrt.dll` ASan compatibility, I cannot remember any attempt to intentionally support WinASan usage in this configuration, so I'm somewhat inclined to say that it was an accident that it ever worked at all. Contributions to support it would be welcome, but we should keep this change in to prioritize the users (Chromium & Firefox) who benefit from this change.

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. https://github.com/mstorsjo/llvm-mingw/issues/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:

```
check-asan-dynamic:

Total Discovered Tests: 500
  Unsupported      : 243 (48.60%)
  Passed           : 155 (31.00%)
  Expectedly Failed:  19 (3.80%)
  Failed           :  83 (16.60%)

check-interception:

Total Discovered Tests: 14
  Passed: 14 (100.00%)

check-builtins:

Total Discovered Tests: 221
  Unsupported      :  63 (28.51%)
  Passed           : 157 (71.04%)
  Expectedly Failed:   1 (0.45%)

check-profile:

Total Discovered Tests: 145
  Unsupported      : 91 (62.76%)
  Passed           : 52 (35.86%)
  Expectedly Failed:  2 (1.38%)

check-sanitizer:

Total Discovered Tests: 804
  Unsupported: 456 (56.72%)
  Passed     : 348 (43.28%)

check-ubsan:

Total Discovered Tests: 88
  Unsupported      : 16 (18.18%)
  Passed           : 65 (73.86%)
  Expectedly Failed:  2 (2.27%)
  Failed           :  5 (5.68%)
```

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.

https://github.com/llvm/llvm-project/pull/120397