Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[sanitizer][asan][win][msvc] Correct interception of strdup on windows, additionally correclty intercept memmove/memcpy on i386 windows.
Needs RevisionPublic

Authored by barcharcraz on May 3 2023, 10:01 AM.

Details

Summary

We (Microsoft) have a bunch of changes on top of LLVM-14 to enable asan to work on windows with msvc. We'd like to start working in the open, on LLVM main, but our branch has diverged somewhat. We've thrown some changesets over the wall previously and I'm going to be resuming the effort.

In any event this is a very self-contained set of fixes to interception. It intercepts _strdup on windows instead of strdup and changes the way memcpy is intercepted on I386 windows. For strdup windows does not define an actual strdup function, only _strdup (because strdup is in POSIX, not C, until C23). Note that _strdup_dbg ends up calling _strdup. For memcpy the behavior is changed on I386 windows to intercept with asan's version of memmove. On I386 windows memcpy and memmove are actually separate functions, even though they have identical implementations, thus memcpy needs to be intercepted with asan's memmove to avoid having asan check for overlapping ranges, which could break programs relying on this behavior (this is a bit of a judgement call though, we think preserving the memmoving behavior of memcpy makes asan more useful for catching real bugs, but our memcpy isn't actually documented as being memmove on all architectures.)

[asan][win][msvc] Intercept memcpy in addition to memmove on i386 windows.

[asan][win][test] Add tests ensuring memcpy and memmove are properly intercepted on windows

Diff Detail

Event Timeline

barcharcraz created this revision.May 3 2023, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 10:01 AM
Herald added a subscriber: Enna1. · View Herald Transcript
barcharcraz retitled this revision from [asan][win][msvc] Correct interception of strdup on windows, additionally correclty intercept memmove/memcpy on i386 windows. to [sanitizer][asan][win][msvc] Correct interception of strdup on windows, additionally correclty intercept memmove/memcpy on i386 windows..May 3 2023, 10:10 AM
barcharcraz added a project: Restricted Project.
barcharcraz edited the summary of this revision. (Show Details)May 3 2023, 11:53 AM
barcharcraz added a reviewer: vitalybuka.
  • clang-format
barcharcraz published this revision for review.May 3 2023, 12:05 PM
Herald added a subscriber: Restricted Project. · View Herald TranscriptMay 3 2023, 12:05 PM

Please split patches, changes are unrelated.

compiler-rt/lib/asan/asan_interceptors.cpp
466 ↗(On Diff #519198)

please use:
#if WIndows
#define strdup _strdup
#endif

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
907

isn't this constant for the platform?

remove the changes to strdup from this diff.

barcharcraz added inline comments.May 4 2023, 11:10 AM
compiler-rt/lib/asan/asan_interceptors.cpp
466 ↗(On Diff #519198)

This is a little dangerous if <string.h> ends up getting included in this file, since we define strdup to _strdup_dbg or _strdup there (and in asan we want to intercept _strdup even in debug mode).

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
907

I'm not 100% sure for armv7. I think it's not the case for IA64, but .... it's IA64, we don't really support it anymore.

barcharcraz added inline comments.May 8 2023, 10:12 AM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
907

are you saying that you'd prefer to have the preprocessor logic on lines 202-210 inlined here (and at other usages)

memmove and strdup should be two different patches

compiler-rt/lib/asan/asan_interceptors.cpp
466 ↗(On Diff #519198)

Is this even would compile if you include string.h here?

We are doing this for other platforms, and I would like consistency.
and i don't like repeating this code 3 times

alternative approach:
in a separate patch extract common code from 2 functions
in this patch use the code for the third one

470–481 ↗(On Diff #519198)

Move GET_STACK_TRACE_MALLOC and extract this
You need REAL(memcpy) as parameters

485 ↗(On Diff #519198)

Please introduce ASAN_INTERCEPT_STRDUP and ASAN_INTERCEPT__STRDUP and use instead of SANITIZER_WINDOWS

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
906–907

I am asking to remove selected condition condition

  • revert changes to INIT_MEMCPY as they are redundant
  • clang format
barcharcraz added inline comments.May 11 2023, 11:23 AM
compiler-rt/lib/asan/asan_interceptors.cpp
466 ↗(On Diff #519198)

I ended up using push/pop macro, and those changes appear in https://reviews.llvm.org/D149876 (which contains only the strdup changes, as requested).

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
906–907

Ah I see, it's redundant with the code already in the memcpy interceptor. I've reverted the changes to INIT_MEMCPY.

vitalybuka accepted this revision.May 11 2023, 1:34 PM
This revision is now accepted and ready to land.May 11 2023, 1:34 PM

Thanks for adding me in the loop. The change itself looks fine, though I haven't tried it on MinGW-w64 target yet. Some comments on the tests:

  • It seems the two tests memcpy_sanity.cpp and memmove_sanity.cpp does the same thing for the most part. Can they be merged into one single test file and switch between testing memmove and memcpy by checking argc/argv?
  • These tests will not work on MinGW-w64 as is because of the use of clang-cl command line flags. I have been fixing the asan tests to work on MinGW-w64 targets (see for example D150269) and there are now nightly builds that run asan tests. Please add // UNSUPPORTED: target={{.*-windows-gnu}} to disable the new tests for now. (It would be nice if you can make the test compatible with MinGW-w64, but that might be asking for too much.)

Thanks for adding me in the loop. The change itself looks fine, though I haven't tried it on MinGW-w64 target yet. Some comments on the tests:

  • It seems the two tests memcpy_sanity.cpp and memmove_sanity.cpp does the same thing for the most part. Can they be merged into one single test file and switch between testing memmove and memcpy by checking argc/argv?
  • These tests will not work on MinGW-w64 as is because of the use of clang-cl command line flags. I have been fixing the asan tests to work on MinGW-w64 targets (see for example D150269) and there are now nightly builds that run asan tests. Please add // UNSUPPORTED: target={{.*-windows-gnu}} to disable the new tests for now. (It would be nice if you can make the test compatible with MinGW-w64, but that might be asking for too much.)

Honestly I do want to make them work with mingw, but there's an additional complication ....

Our internal test harness treats "%clang_cl_asan" as "real" cl.exe, so clang style flags won't actually work at all (although in this case I had to add the no-builtin flags anyway). In the end I'll probably add some kind of conditional macro in lit to allow specifying separate cl and clang style flags. For now, I agree that just making them unsupported on -gnu would be easiest.

Also, on mingw could lit make clang_cl_asan work by just telling it to build for the gnu target instead of the msvc one? I'm not sure how well this is supported.

  • Make memcpy and memmove tests unsupported on mingw
  • merge the memcpy and memmove tests
  • clang-format
alvinhochun added inline comments.May 19 2023, 4:36 AM
compiler-rt/test/asan/TestCases/Windows/memcpy_memmove_sanity.cpp
1–13

This will allow the test to run on mingw-w64. @vitalybuka how do you feel about this?

strega-nil requested changes to this revision.May 25 2023, 10:22 AM
strega-nil added a subscriber: strega-nil.
strega-nil added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
204

In line with our conversation yesterday, we'd prefer to not lose the memcpy/memmove distinction when it comes to diagnostics; we still want to diagnose:

cxx
int main() {
  char buf[] = "Hello!";
  memcpy(&buf[1], &buf[0], (sizeof buf) - 1);
}
This revision now requires changes to proceed.May 25 2023, 10:22 AM

Probably good to rename the title as well.