Page MenuHomePhabricator

[Scudo] Make -fsanitize=scudo use standalone. Migrate tests.
AcceptedPublic

Authored by hctim on May 14 2021, 5:58 PM.

Details

Summary

This patch moves -fsanitize=scudo to link the standalone scudo library,
rather than the original compiler-rt based library. This is one of the
major remaining roadblocks to deleting the compiler-rt based scudo,
which should not be used any more. The standalone Scudo is better in
pretty much every way and is much more suitable for production usage.

As well as patching the litmus tests for checking that the
scudo_standalone lib is linked instead of the scudo lib, this patch also
ports all the scudo lit tests to run under scudo standalone.

This patch also adds a feature to scudo standalone that was under test
in the original scudo - that arguments passed to an aligned operator new
were checked that the alignment was a power of two.

Some lit tests could not be migrated, due to the following issues:

  1. Features that aren't supported in scudo standalone, like the rss limit.
  2. Different quarantine implementation where the test needs some more thought.
  3. Small bugs in scudo standalone that should probably be fixed, like the Secondary allocator having a full page on the LHS of an allocation that only contains the chunk header, so underflows by <= a page aren't caught.
  4. Slight differences in behaviour that's technically correct, like 'realloc(malloc(1), 0)' returns nullptr in standalone, but a real pointer in old scudo.
  5. Some tests that might be migratable, but not easily.

Tests that are obviously not applicable to scudo standalone (like
testing that no sanitizer symbols made it into the DSO) have been
deleted.

After this patch, the remaining work is:

  1. Update the Scudo documentation. The flags have changed, etc.
  2. Delete the old version of scudo.
  3. Patch up the tests in lit-unmigrated, or fix Scudo standalone.

Diff Detail

Event Timeline

hctim created this revision.May 14 2021, 5:58 PM
hctim requested review of this revision.May 14 2021, 5:58 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 14 2021, 5:58 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript

Thanks a lot Mitch! Couple of comments from a first look, I'll give it another go later.

compiler-rt/lib/scudo/standalone/wrappers_cpp.cpp
41

I don't think errno carry over to C++, there seems to be no sign of it in https://en.cppreference.com/w/cpp/memory/new/operator_new

compiler-rt/test/scudo/standalone/lit.cfg.py
21

-ldl might not be necessary anymore without the sanitizer dependencies?

I think one of the remaining things to address is the supported architectures:

set(ALL_SCUDO_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64} ${PPC64})
set(ALL_SCUDO_STANDALONE_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64})

I am unclear as to whether or not there are consumers for the missing ones, but if there are we might break -fsanitize=scudo for them.
We have fallback for non-x86/arm processors in https://github.com/llvm/llvm-project/blob/e0921655b1ff8d4ba7c14be59252fe05b705920e/compiler-rt/lib/scudo/standalone/checksum.cpp#L79 , the rest looks fine so I think adding them should work.

hctim marked 2 inline comments as done.May 17 2021, 9:05 AM

I think one of the remaining things to address is the supported architectures:

set(ALL_SCUDO_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM32} ${ARM64} ${MIPS32} ${MIPS64} ${PPC64})
set(ALL_SCUDO_STANDALONE_SUPPORTED_ARCH ${X86} ${X86_64} ${ARM64})

I am unclear as to whether or not there are consumers for the missing ones, but if there are we might break -fsanitize=scudo for them.
We have fallback for non-x86/arm processors in https://github.com/llvm/llvm-project/blob/e0921655b1ff8d4ba7c14be59252fe05b705920e/compiler-rt/lib/scudo/standalone/checksum.cpp#L79 , the rest looks fine so I think adding them should work.

Sure, I've added them but I don't have anything to test with. Well - I'm sure they'll let us know if they break.

compiler-rt/lib/scudo/standalone/wrappers_cpp.cpp
41

Removed.

compiler-rt/test/scudo/standalone/lit.cfg.py
21

Done.

hctim updated this revision to Diff 345906.May 17 2021, 9:06 AM
hctim marked 2 inline comments as done.

Address Kostya's comments.

cryptoad accepted this revision.May 17 2021, 10:48 AM

I think this is good, with the additional arch

compiler-rt/cmake/config-ix.cmake
338 ↗(On Diff #345906)

${ARM32} as well please

This revision is now accepted and ready to land.May 17 2021, 10:48 AM
hctim updated this revision to Diff 345939.May 17 2021, 10:53 AM

Add arm32 to supported.

Would you mind to clang-format entire existing compiler-rt/test/scudo/
and than rebase this patch on top?

clang-format

vitalybuka accepted this revision.May 17 2021, 12:31 PM
vitalybuka added inline comments.May 17 2021, 12:35 PM
compiler-rt/cmake/config-ix.cmake
338 ↗(On Diff #345967)

I would recommend to change this line in a separate patch after this one

hctim updated this revision to Diff 345977.May 17 2021, 12:53 PM

Let's submit the "adding archs" part of the patch first, then land this.

vitalybuka added inline comments.May 18 2021, 9:04 PM
compiler-rt/test/scudo/standalone/CMakeLists.txt
14–15

This places breaks ppc build and I don't understand why we need it

compiler-rt/test/scudo/standalone/lit.site.cfg.py.in
6

see above

hctim marked 2 inline comments as done.Tue, May 25, 3:42 PM
hctim added inline comments.
compiler-rt/test/scudo/standalone/CMakeLists.txt
14–15

Yeah, get_test_cc_for_arch looks wrong to me. But we do still need one of its children, get_target_flags_for_arch, otherwise host cross-compile (from amd64 -> i386) fails.

hctim updated this revision to Diff 347814.Tue, May 25, 3:43 PM
hctim marked an inline comment as done.

Address Vitaly's suggestions, make some slight changes so that cross-compilation and running under QEMU doesn't require staging the runtimes in the host compiler.

hctim reopened this revision.Wed, May 26, 12:59 PM
This revision is now accepted and ready to land.Wed, May 26, 12:59 PM
hctim updated this revision to Diff 348116.Wed, May 26, 3:35 PM

Move lit tests behind the cmake guard:
"if(COMPILER_RT_INCLUDE_TESTS AND COMPILER_RT_HAS_SCUDO_STANDALONE)"

I saw some bots failure for preinit.c:

FAIL: ScudoStandalone-i386 :: preinit.c (768 of 856)
******************** TEST 'ScudoStandalone-i386 :: preinit.c' FAILED ********************
Script:
--
: 'RUN: at line 1';      /b/sanitizer-x86_64-linux/build/clang_build/./bin/clang   -m32  -pthread -fPIE -pie -O0 -UNDEBUG -Wl,--gc-sections -resource-dir=/b/sanitizer-x86_64-linux/build/clang_build/./lib/clang/13.0.0/lib/linux/../../ -fsanitize=scudo /b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/scudo/standalone/preinit.c -o /b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.tmp
: 'RUN: at line 2';    /b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.tmp 2>&1
--
Exit Code: 139

Command Output (stderr):
--
/b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.script: line 2: 20628 Segmentation fault      /b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.tmp 2>&1

Do you know what went on with those?

I saw some bots failure for preinit.c:

FAIL: ScudoStandalone-i386 :: preinit.c (768 of 856)
******************** TEST 'ScudoStandalone-i386 :: preinit.c' FAILED ********************
Script:
--
: 'RUN: at line 1';      /b/sanitizer-x86_64-linux/build/clang_build/./bin/clang   -m32  -pthread -fPIE -pie -O0 -UNDEBUG -Wl,--gc-sections -resource-dir=/b/sanitizer-x86_64-linux/build/clang_build/./lib/clang/13.0.0/lib/linux/../../ -fsanitize=scudo /b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/test/scudo/standalone/preinit.c -o /b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.tmp
: 'RUN: at line 2';    /b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.tmp 2>&1
--
Exit Code: 139

Command Output (stderr):
--
/b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.script: line 2: 20628 Segmentation fault      /b/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/scudo/standalone/I386LinuxConfig/Output/preinit.c.tmp 2>&1

Do you know what went on with those?

Hmm, nope, didn't see them or know what's going on. It doesn't repro using one of my configs - do you have a link to the buildbot?

There were two other issues I spotted:

  1. Tests were being run when scudo wasn't supported (COMPILER_RT_HAS_SCUDO_STANDALONE != true), which popped up because the Android bots tried to run Scudo standalone tests before D103200 landed. This is fixed in the patchset.
  2. The qemu sanitizer bot failed to find stdint.h. I have the same problem on my local checkout, most of the libc headers come from one directory, but stdint.h and friends need to come from the compiler, and the compiler's include dir isn't part of the default set. Needs to be fixed on bot, but I haven't got around to that.

sanitizer-x86_64-linux https://lab.llvm.org/buildbot/#/builders/37/builds/4244 FAIL: ScudoStandalone-x86_64 :: preinit.c (772 of 856)
ppc64be-clang-test https://lab.llvm.org/buildbot#builders/52/builds/7794 TEST 'ScudoStandalone-powerpc64 :: preinit.c' FAILED
among others