Page MenuHomePhabricator

[scudo] Port scudo sanitizer to Windows
AbandonedPublic

Authored by russell.gallop on Feb 5 2021, 4:30 AM.

Details

Summary

Based on https://reviews.llvm.org/D42519, this ports the sanitizer version of scudo to Windows.

Passes lit tests and when used as the allocator for LLVM, that passes check-all. Have noticed that on LLVM with Scudo, https://bugs.llvm.org/show_bug.cgi?id=24978 does intermittently occur when running lli tests.

For more details of evaluation see https://reviews.llvm.org/D86694

A separate review allows hooking this in as the memory allocator for LLVM (https://reviews.llvm.org/D96133).

I'm aware that scudo sanitizer version is not under active development. This is intended as a step to porting scudo standalone.

Diff Detail

Event Timeline

russell.gallop created this revision.Feb 5 2021, 4:30 AM
russell.gallop requested review of this revision.Feb 5 2021, 4:30 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 5 2021, 4:30 AM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript
aganea added a comment.Feb 5 2021, 7:50 AM

A few questions: Does this work on x86 targets? Are the scudo tests below being built with /MD or with /MT? Would this change compile/work on MinGW as well?

compiler-rt/lib/scudo/scudo_platform.h
72

It is not clear for readers why some platforms have a value different than others. It'd be really nice if there was a short comment explaining that, at least for Windows. For Windows at least, the reason is explained here: https://reviews.llvm.org/D86694#2277371

As is, this breaks compilation for mingw. With the three modifications I suggest here, it no longer breaks compilation for me - I have no idea if it actually works in mingw configurations though, but not breaking compilation is at least the first step.

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
28

The includes need to be all-lowercase for mingw. (The windows SDK isn't self-consistent so it can't be used as-is on case sensitive filesystems, thus the all-lowercase convention of mingw headers is the only one that works for case sentisitivity for now.)

compiler-rt/lib/scudo/CMakeLists.txt
18–19

This needs an extra append_list_if(MINGW "${MINGW_LIBRARIES}" SCUDO_MINIMAL_DYNAMIC_LIBS) here (similar to a line in compiler-rt/lib/asan/CMakeLists.txt) to fix building in mingw configurations. (That's needed because these libs are built with -nodefaultlibs, which omits a few essential libs.)

compiler-rt/lib/scudo/scudo_new_delete.cpp
30

These don't work in this form for mingw targets (which use the itanium c++ abi).

By changing #if SANTIZER_WINDOWS into #if SANITIZER_WINDOWS && !defined(__MINGW32__), I fixed the linker errors at least.

russell.gallop edited the summary of this revision. (Show Details)Feb 8 2021, 2:51 AM

As is, this breaks compilation for mingw. With the three modifications I suggest here, it no longer breaks compilation for me - I have no idea if it actually works in mingw configurations though, but not breaking compilation is at least the first step.

Thanks for the information I'll try to run up a mingw environment and check it works.

A few questions: Does this work on x86 targets?

I haven't tried. I'll test this out.

Are the scudo tests below being built with /MD or with /MT?

The lit tests? They don't specify either. Does that imply /MT?

Would this change compile/work on MinGW as well?

I'll check.

As is, this breaks compilation for mingw. With the three modifications I suggest here, it no longer breaks compilation for me - I have no idea if it actually works in mingw configurations though, but not breaking compilation is at least the first step.

Thanks for the information I'll try to run up a mingw environment and check it works.

In case that turns out to be tricky, I might be able to help with that, at least for building a simple test program with it and running it, if you say how it's supposed to be used (building/linking with -fsanitize=scudo?) and how to inspect whether it actually works.

Added comment on AllocatorSize.

Applied Mingw changes suggested by @mstorsjo.

russell.gallop marked 2 inline comments as done.Feb 26 2021, 9:37 AM

As is, this breaks compilation for mingw. With the three modifications I suggest here, it no longer breaks compilation for me - I have no idea if it actually works in mingw configurations though, but not breaking compilation is at least the first step.

Thanks for the information I'll try to run up a mingw environment and check it works.

In case that turns out to be tricky, I might be able to help with that, at least for building a simple test program with it and running it, if you say how it's supposed to be used (building/linking with -fsanitize=scudo?) and how to inspect whether it actually works.

Hi @mstorsjo. Thanks for the suggestions. I tried running up an mingw environment with msys but had trouble getting it working (running into cmake issues). Would you be able to help?

Yes, building and linking a program with -fsanitize=scudo is the simple way to build a simple program. The support for this is in clang/lib/Driver/ToolChains/MSVC.cpp. Would this require support in clang/lib/Driver/ToolChains/MinGW.cpp?

I think the best way to test is to run the scudo LIT tests with (e.g.) "ninja check-scudo". These build and run some simple test programs and check that problems are detected.

Beyond this I built LLVM with scudo and ran check-all. I don't know whether you want to go this far. Note that -fsanitize=scudo doesn't work for the MSVC LLVM build which uses link/lld-link for linking rather than going via the compiler driver so I hooked it into LLVM_INTEGRATED_CRT_ALLOC (see https://reviews.llvm.org/D96133). If mingw uses the compiler driver to link (like Linux) then you may be able to use -DLLVM_USE_SANITIZER=Scudo on a stage2 build. This would require some changes to llvm/cmake/modules/HandleLLVMOptions.cmake to permit it (I had similar changes in https://reviews.llvm.org/D86694). It's harder to tell whether this has worked correctly. With MSVC faster ThinLTO links on multi-core machines are a good indicator but I guess mingw wouldn't be using the MSVC allocator.

Thanks
Russ

compiler-rt/lib/scudo/scudo_new_delete.cpp
30

I've applied this suggestion.

Hi @mstorsjo. Thanks for the suggestions. I tried running up an mingw environment with msys but had trouble getting it working (running into cmake issues). Would you be able to help?

Yeah, building things in that environment is a bit challenging.

Yes, building and linking a program with -fsanitize=scudo is the simple way to build a simple program. The support for this is in clang/lib/Driver/ToolChains/MSVC.cpp. Would this require support in clang/lib/Driver/ToolChains/MinGW.cpp?

Yes, it would need something similar - I tried whipping something together, which after some tweaks seems to work:

diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index f6cead412236..aab141377204 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -260,6 +260,14 @@ void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
         }
       }
 
+      if (Sanitize.needsScudoRt()) {
+        for (const auto &Lib : {"scudo", "scudo_cxx"}) {
+          CmdArgs.push_back(TC.getCompilerRTArgString(Args, Lib));
+        }
+        CmdArgs.push_back("--require-defined");
+        CmdArgs.push_back(Args.MakeArgString("malloc"));
+      }
+
       AddLibGCC(Args, CmdArgs);
 
       if (Args.hasArg(options::OPT_pg))
@@ -492,6 +500,7 @@ SanitizerMask toolchains::MinGW::getSupportedSanitizers() const {
   Res |= SanitizerKind::PointerCompare;
   Res |= SanitizerKind::PointerSubtract;
   Res |= SanitizerKind::Vptr;
+  Res |= SanitizerKind::Scudo;
   return Res;
 }

Feel free to squash that into your patch (which saves me a bit of effort) :-)

(First I had placed this later in the function, after the asan bits, but in that case, it ended up linking against the normal malloc function instead of the one from scudo - the libraries that provide malloc end up added via the AddLibGCC() call.

I think the best way to test is to run the scudo LIT tests with (e.g.) "ninja check-scudo". These build and run some simple test programs and check that problems are detected.

I didn't try this right now (I primarily cross compile so running tests for the runtimes is a bit challenging), but I did test building the double-free.cpp test with -fsanitize=scudo, and it seems to work as it should for all three cases it tests, so with that it seems to be at least roughly working, so I think that's good enough to include the clang driver bits in ToolChains/MinGW.cpp.

This is intended as a step to porting scudo standalone.

Why this is needed for scudo stadalone?

This is intended as a step to porting scudo standalone.

Why this is needed for scudo stadalone?

It’s not strictly needed. It seemed like it was easier, as some of the work had already been done, and some of the issues worked through on this would probably also come up when porting standalone.

Update with suggested changes to MinGW.cpp

Yes, it would need something similar - I tried whipping something together, which after some tweaks seems to work:
Feel free to squash that into your patch (which saves me a bit of effort) :-)

Thanks for doing that. I've added this in.

...I did test building the double-free.cpp test with -fsanitize=scudo, and it seems to work as it should for all three cases it tests...

Great, thanks.

kcc added a subscriber: kcc.

We can't possibly maintain two variants of scudo.
All effort is currently spent on the newer (standalone) version.
I am afraid we will have to delete the older (non-standalone) variant entirely.
(And the sooner the better)

vitalybuka added inline comments.Mar 2 2021, 11:35 AM
clang/lib/Driver/ToolChains/MSVC.cpp
489

We need this for both legacy and standalone scudo?

compiler-rt/lib/scudo/scudo_platform.h
19

For the reason @kcc mentioned, if possible could you not update legacy scudo at at all, and keep only stuff needed for standalone version?

BTW. Patches to remove legacy one are welcomed.

russell.gallop abandoned this revision.Mar 9 2021, 1:54 PM
In D96120#2597877, @kcc wrote:

I am afraid we will have to delete the older (non-standalone) variant entirely.
(And the sooner the better)

Okay, think we found a few things out by doing this before porting standalone.

If I find time I’ll have a go at removing the sanitizer version.

The scudo documentation (https://llvm.org/docs/ScudoHardenedAllocator.html) still seems to refer to the sanitizer version so will need updating.

hctim added a comment.Mar 9 2021, 2:46 PM
In D96120#2597877, @kcc wrote:

I am afraid we will have to delete the older (non-standalone) variant entirely.
(And the sooner the better)

Okay, think we found a few things out by doing this before porting standalone.

If I find time I’ll have a go at removing the sanitizer version.

The scudo documentation (https://llvm.org/docs/ScudoHardenedAllocator.html) still seems to refer to the sanitizer version so will need updating.

When we remove the non-standalone variant, it would be great to move -fsanitize=scudo over to scudo-standalone :).