[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

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

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

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

[vitalybuka]

compiler-rt/CMakeLists.txt

512 ↗

(On Diff #481097)

514 ↗

(On Diff #481097)

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

[vitalybuka]

compiler-rt/CMakeLists.txt

514 ↗

(On Diff #481097)

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/lib/lsan/CMakeLists.txt

 include_directories(..)
 
 set(LSAN_CFLAGS ${SANITIZER_COMMON_CFLAGS})
 append_rtti_flag(OFF LSAN_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.
+#
+# Keep as =zero to mitigate any performance costs as opposed to =pattern.
+#
+# Note that, in the general case, this may hide some bugs in lsan runtime
+# itself that would involve reading uninitialized memory.
+check_cxx_compiler_flag(-ftrivial-auto-var-init=zero COMPILER_RT_HAS_AUTO_VAR_INIT_ZERO)
+append_list_if(COMPILER_RT_HAS_AUTO_VAR_INIT_ZERO -ftrivial-auto-var-init=zero LSAN_CFLAGS)
+
 # Too many existing bugs, needs cleanup.
 append_list_if(COMPILER_RT_HAS_WNO_FORMAT -Wno-format LSAN_CFLAGS)
 
 set(LSAN_COMMON_SOURCES
   lsan_common.cpp
   lsan_common_fuchsia.cpp
   lsan_common_linux.cpp
   lsan_common_mac.cpp
   )