This change adds tests to make sure that SCUDO is being properly
included with llvm libc. This change also adds the toggles to properly
use SCUDO, as GWP-ASan is enabled by default and must be included for
SCUDO to function.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| compiler-rt/CMakeLists.txt | ||
|---|---|---|
| 51 | s/COMPILER_RT_BUILD_ORC/COMPILER_RT_BUILD_GWP_ASAN/ | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | COMPILER_RT_HAS_GWP_ASAN | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | Should this be under if(LLVM_LIBC_INCLUDE_SCUDO)? | |
| libc/test/integration/CMakeLists.txt | ||
| 2 | Fix. | |
| libc/test/integration/scudo/CMakeLists.txt | ||
| 11 | We don't need this as you have added a target_link_libraries call below to link to llvmlibc. | |
| 18 | Remove this? | |
| 40 | Fix. | |
| libc/test/integration/scudo/integration_test.cpp | ||
| 13 | Add calls to all allocator related libc functions? For example, follow malloc with free and do the same with realloc, calloc and aligned_alloc? | |
Comment Actions
responding to comments.
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | I can't make this COMPILER_RT_HAS_GWP_ASAN since that variable won't be defined yet because the llvm libc cmake code runs before the compiler-rt cmake code does. If this was under if(LLVM_LIBC_INCLUDE_SCUDO) then you couldn't include GWP-ASan without also including SCUDO, and I figured it was better to give the user the option. | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | Is GWP-ASan useful without something like SCUDO calling into it? | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | That seems weird that libc's full cmake runs before compiler-rt's config-ix. I think you might run into a problem where you want GWP-ASan, but GWP-ASan isn't supported on your architecture (like mips). In these cases COMPILER_RT_BUILD_GWP_ASAN == True but COMPILER_RT_HAS_GWP_ASAN == False.
Nope, GWP-ASan needs be called from malloc(). | |
Comment Actions
move the gwp-asan if statement inside of the scudo if statement
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | based on this I've moved the if(COMPILER_RT_BUILD_GWP_ASAN) inside of the if(LLVM_LIBC_INCLUDE_SCUDO). | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | Try setting COMPILER_RT_HAS_GWP_ASAN in the PARENT_SCOPE? | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | I have now tried that, and it didn't seem to work. I'm not sure why though, given that I think the parent scope should be the one above the compiler-rt directory. My guess would be that the problem is the execution order, but if you have other things to try feel free to suggest them. | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | I have no more straightforward ideas. But, a question for @hctim - Why do we have code like this in config-ix.cmake: list_intersect(GWP_ASAN_SUPPORTED_ARCH ALL_GWP_ASAN_SUPPORTED_ARCH SANITIZER_COMMON_SUPPORTED_ARCH) The reason I am asking this is because, if GWP-ASan is standalone, why should the supported archs be deduced from what sanitizer common supports? If we can decouple GWP-ASan CMake logic from the rest of compiler-rt logic, we can potentially put GWP-ASan related pieces in a .cmake file and include that file from within config-ix.cmake and libc CMake files. | |
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 19 | It doesn't have a dependency on sanitizer-common, but the files are in compiler-rt for licensing/ease-of-setup/historical reasons. | |
Comment Actions
I've added some new CMake machinery to allow for building the SCUDO standalone and GWP-ASan more separately from compiler-rt as a whole, specifically by adding extra conditions that only build the object files. In addition I've separated out the definitions for the different parts of compiler-rt so that llvm libc can check against those.
| compiler-rt/lib/gwp_asan/CMakeLists.txt | ||
|---|---|---|
| 81 | If GWP-ASan is a standalone (as it does not depend on rest of compiler-rt), can we add the following object libraries unconditionally with ARCHS ${ALL_GWP_ASAN_SUPPORTED_ARCH}? Otherwise, there is again a disconnect between GWP-ASan build and libc build - GWP-ASan object libraries are condition to GWP_ASAN_BUILD_OBJECTS, which depends on GWP_ASAN_SUPPORTED_ARCH, which in turn depends on COMPILER_RT_SUPPORTED_ARCH. May be we can even add the above runtime libraries also unconditionally? | |
| compiler-rt/lib/scudo/standalone/CMakeLists.txt | ||
| 171 ↗ | (On Diff #363163) | A similar question for this as well. |
| compiler-rt/lib/gwp_asan/CMakeLists.txt | ||
|---|---|---|
| 81 | If I set this to ARCHS ${ALL_GWP_ASAN_SUPPORTED_ARCH} then I get errors due to the supported platform checking in the add_compiler_rt_object_libraries function. Without modifying that I simply cannot tell it to build for archs not supported by compiler-rt as a whole. If we made a change (such as adding a new function for building just object libraries for all possible archs) I think adding the libraries unconditionally would be fine. | |
| compiler-rt/cmake/config-ix.cmake | ||
|---|---|---|
| 703 | why create a copy of SCUDO_STANDALONE_SUPPORTED_ARCH? | |
| 741 | remove this | |
| 749–753 | not necessary if we delete COMPILER_RT_HAS_SANITIZER_COMMON, then we can guard things by COMPILER_RT_HAS_GWP_ASAN | |
| compiler-rt/lib/gwp_asan/CMakeLists.txt | ||
| 36 | remove this | |
| 40 | add append_list_if(COMPILER_RT_HAS_SANITIZER_COMMON, ${SANITIZER_COMMON_CFLAGS} GWP_ASAN_CFLAGS) | |
| 80–82 | can revert this diff now that COMPILER_RT_HAS_GWP_ASAN is back to being the source of truth | |
Comment Actions
address comments
| compiler-rt/cmake/config-ix.cmake | ||
|---|---|---|
| 703 | I figured it was probably better to condition off of a proper bool as opposed to a list, but if we're not using this at all then I can remove it. | |
| compiler-rt/lib/gwp_asan/CMakeLists.txt | ||
| 80–82 | COMPILER_RT_HAS_GWP_ASAN being the source of truth is useful, but from what I can tell it's still conditioned on the compiler-rt supported architectures. The problem there is that I don't see an easy way to tell compiler-rt that you want to compile for an architecture that isn't the current one. If there's a cmake flag to do that that I have missed, then that would be very helpful. | |
Comment Actions
Have you tested that GWP-ASan works? I know your test runner can't capture stderr/stdout right now, but can you link a program with scudo+llvm-libc+gwp-asan and run a malloc->free->uaf loop and see GWP-ASan crash? Something like compiler-rt/test/gwp_asan/use_after_free.cpp.
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 6 | nit: break across multiple lines | |
| 11 | nit: break across multiple lines | |
Comment Actions
I added a file copied from use_after_free.cpp but it's not crashing like it's supposed to, which to me means that something might be wrong with the gwp-asan include? I will investigate it further.
Comment Actions
Yeah, sorry, the bad access should be done in a loop or with GWP_ASAN_OPTIONS=SampleRate=1, i.e.
int main() {
for (unsigned i = 0; i < 0x10000; ++i) {
char *Ptr = reinterpret_cast<char *>(malloc(10));
for (unsigned i = 0; i < 10; ++i) {
*(Ptr + i) = 0x0;
}
free(Ptr);
volatile char x = *Ptr;
}
return x;
}I should've made that more clear.
Comment Actions
LGTM w/ nit. Thanks for doing this!
| libc/test/integration/scudo/gwp_asan_should_crash.cpp | ||
|---|---|---|
| 23 | is this necessary? | |
Comment Actions
final nits
| libc/lib/CMakeLists.txt | ||
|---|---|---|
| 5 | Correct. If compiler-rt and llvm libc are set to compile to different architectures then there will be an error here, but hopefully that won't be a problem in the real world. | |
| libc/test/integration/scudo/gwp_asan_should_crash.cpp | ||
| 23 | yes, because otherwise the compiler warns about x being an unused variable. | |
s/COMPILER_RT_BUILD_ORC/COMPILER_RT_BUILD_GWP_ASAN/