[compiler-rt] Add opt-in -ftrivial-auto-var-init flag for writing over uninitialized stack variiables

[leonardchan]

Summary

This might allow lsan to find more leaks that would have gone undetected. When lsan searches for leaked pointers on the stack, if a leaked pointer that was pushed to the stack in a prior function call would not be scrubbed on a future function call, then the scan will see the pointer on the stack and not mark it as leaked. Such holes can exist in the lsan runtime where there may be uninitialized data. Adding auto-var-init can scrub some of that data and might be able to catch more leaks that would've gone undetected this way.

See https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=111351 for more details.

[mcgrathr]

compiler-rt/lib/lsan/CMakeLists.txt

12 ↗

(On Diff #466932)

This should note that it won't cover all possible uninitialized stack contents, such as those used for register spill slots or unused portions for alignment, or even local variables not yet in scope at a certain point in the function.

[vitalybuka]

I would prefer it's just externally provided CFLAG
or at most opt-in cmake flag

[vitalybuka]

BTW, would this one solve the problem D130237 this patch is trying to address?

[mcgrathr]

In D135716#3866291, @vitalybuka wrote:

BTW, would this one solve the problem D130237 this patch is trying to address?

It would solve it for many cases, but not necessarily all. As mentioned earlier, spill slots and unused alignment space (and perhaps even x86-64 red zone) might be left uninitialized and thus perhaps contain falsely-live pointer values.
D130237 is more thorough in one sense, because it minimizes the depth of stack that will be scanned somewhat. Thus anything deeper in the call stack than CheckForLeaks() is already immune and the auto-var-init there isn't helping anything.
However, CheckForLeaks() is called in a few places at various depths in different call stacks inside the runtime. So this may address some of the holes left by that, but not necessarily all.

The most thorough option would be for the SP at each top-level entry into the runtime code that can lead to the leak-check path to be captured at the outset and propagated down. Then the runtime would know that it's never scanning its own stack frames at all (aside from e.g. other threads suspended while in the lsan allocator code).

[vitalybuka]

Bumping in case it missed:

In D135716#3851143, @vitalybuka wrote:

I would prefer it's just externally provided CFLAG
or at most opt-in cmake flag

I am wondering, if don't make it default, why just don't provide external CFLAG into CMAKE when building runtime
But If we enable it by default, maybe do this for for sanitizer_common and everything that depends on it?

[vitalybuka]

compiler-rt/lib/lsan/CMakeLists.txt

21 ↗

(On Diff #468628)

from my experience, zero/patter perf difference is mostly noticeable on functions with large arrays on stack.
This is uncommon for compiler-rt, so maybe pattern only as additional uninitialized bug trigger?

[leonardchan]

In D135716#3869059, @vitalybuka wrote:

Bumping in case it missed:

In D135716#3851143, @vitalybuka wrote:

I would prefer it's just externally provided CFLAG
or at most opt-in cmake flag

I am wondering, if don't make it default, why just don't provide external CFLAG into CMAKE when building runtime
But If we enable it by default, maybe do this for for sanitizer_common and everything that depends on it?

I believe the initial idea behind this was it addresses an lsan-specific issue and should be limited to lsan, but if it can affect multiple sanitizers, then either of these suggestions works for me also.

[vitalybuka]

compiler-rt/lib/lsan/CMakeLists.txt

16–22 ↗

(On Diff #468628)

I don't see a reason to not enabled for the rest if it's available

[vitalybuka]

compiler-rt/CMakeLists.txt

512

514

compiler-rt/cmake/config-ix.cmake needs to set COMPILER_RT_HAS_TRIVIAL_AUTO_INIT

[vitalybuka]

compiler-rt/CMakeLists.txt

514

check_cxx_compiler_flag(-Werror -ftrivial-auto-var-init=pattern COMPILER_RT_HAS_TRIVIAL_AUTO_INIT)

[vitalybuka]

I just realized that this change may introduce unwanted memsets in compiler rt. It's still the problem even if =pattern is applied to the lsan only.

[melver]

I noticed -ftrivial-auto-var-init=pattern was added to default builds of compiler-rt, and am seeing that the tsan test for memcpy/set is failing.

I started sprinkling attribute((uninitialized)) in the places that introduce the memset to make the tsan test pass, but I don't think that's the right solution as it doesn't scale.

Are there any patches to solve this issue once and for all?

compiler-rt/CMakeLists.txt

 append_list_if(COMPILER_RT_HAS_WD4146_FLAG /wd4146 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4291_FLAG /wd4291 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4391_FLAG /wd4391 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4722_FLAG /wd4722 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4800_FLAG /wd4800 SANITIZER_COMMON_CFLAGS)
 
 append_list_if(MINGW -fms-extensions SANITIZER_COMMON_CFLAGS)
 
+# When lsan scans the stack for detecting reachable pointers, it's possible for
+# a leaked pointer, which was pushed to the stack on an earlier function call,
+# to still exist on the stack when doing a leak check if that part of the stack
+# was not overwritten. In particular, if there's any uninitialized data in the
+# lsan runtime, and the SP we start from is sufficiently deep into the runtime,
+# then a leaked pointer could be marked as reachable. Such instances could be
+# mitigated by clobbering any uninitialized data. Note that this won't cover
+# all possible uninitialized stack contents, such as those used for register
+# spill slots, unused portions for alignment, or even local variables not
+# yet in scope at a certain point in the function.
+#
+# Note that this type of issue was discovered with lsan, but can apply to other
+# sanitizers.
+append_list_if(COMPILER_RT_HAS_TRIVIAL_AUTO_INIT -ftrivial-auto-var-init=pattern SANITIZER_COMMON_CFLAGS)
+
 # Set common link flags.
 # TODO: We should consider using the same model as libc++, that is use either
 # -nostdlib++ and --unwindlib=none if supported, or -nodefaultlibs otherwise.
 append_list_if(C_SUPPORTS_NODEFAULTLIBS_FLAG -nodefaultlibs SANITIZER_COMMON_LINK_FLAGS)
 append_list_if(COMPILER_RT_HAS_Z_TEXT -Wl,-z,text SANITIZER_COMMON_LINK_FLAGS)
 
 # Only necessary for 32-bit SPARC.  Solaris 11.2+ ld uses -z ignore/-z record
 # natively, but supports --as-needed/--no-as-needed for GNU ld compatibility.

compiler-rt/cmake/config-ix.cmake

 check_cxx_compiler_flag(-fno-profile-instr-use COMPILER_RT_HAS_FNO_PROFILE_INSTR_USE_FLAG)
 check_cxx_compiler_flag(-fno-coverage-mapping COMPILER_RT_HAS_FNO_COVERAGE_MAPPING_FLAG)
 check_cxx_compiler_flag("-Werror -mcrc32"    COMPILER_RT_HAS_MCRC32_FLAG)
 check_cxx_compiler_flag("-Werror -msse3" COMPILER_RT_HAS_MSSE3_FLAG)
 check_cxx_compiler_flag("-Werror -msse4.2"   COMPILER_RT_HAS_MSSE4_2_FLAG)
 check_cxx_compiler_flag(--sysroot=.          COMPILER_RT_HAS_SYSROOT_FLAG)
 check_cxx_compiler_flag("-Werror -mcrc"      COMPILER_RT_HAS_MCRC_FLAG)
 check_cxx_compiler_flag(-fno-partial-inlining COMPILER_RT_HAS_FNO_PARTIAL_INLINING_FLAG)
+check_cxx_compiler_flag(-Werror -ftrivial-auto-var-init=pattern COMPILER_RT_HAS_TRIVIAL_AUTO_INIT)
 
 if(NOT WIN32 AND NOT CYGWIN)
   # MinGW warns if -fvisibility-inlines-hidden is used.
   check_cxx_compiler_flag("-fvisibility-inlines-hidden" COMPILER_RT_HAS_FVISIBILITY_INLINES_HIDDEN_FLAG)
 endif()
 
 check_cxx_compiler_flag(/GR COMPILER_RT_HAS_GR_FLAG)
 check_cxx_compiler_flag(/GS COMPILER_RT_HAS_GS_FLAG)