From 2e9df860468425645dcd1b241c5dbf76c072e314 Mon Sep 17 00:00:00 2001 From: Akira Hatanaka Date: Thu, 25 Aug 2022 21:34:09 -0700 Subject: [PATCH] [compiler-rt][builtins] Add compiler flags to catch potential errors that can lead to security vulnerabilities Also, fix a few places that were causing -Wshadow and -Wformat-nonliteral warnings to be emitted. This reapplies the patch that was reverted in caaafe4ae250 because it broke Fuchsia builders. I reverted the changes I made to InstrProfData.inc and instead renamed the variables in InstrProfilingWriter.c. Also fixed a bug in function add_security_warnings that was causing it to pass -Wformat-nonliteral when the compiler doesn't support it. --- .../cmake/Modules/CompilerRTDarwinUtils.cmake | 6 ++++ .../cmake/Modules/CompilerRTUtils.cmake | 29 +++++++++++++++++++ compiler-rt/cmake/config-ix.cmake | 14 +++++++++ compiler-rt/lib/builtins/CMakeLists.txt | 1 + compiler-rt/lib/builtins/eprintf.c | 2 ++ compiler-rt/lib/profile/InstrProfiling.c | 8 ++--- .../lib/profile/InstrProfilingWriter.c | 14 +++++---- 7 files changed, 64 insertions(+), 10 deletions(-) diff --git a/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake b/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake index 2c9983c6a1ae..e2506872751f 100644 --- a/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake +++ b/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake @@ -413,6 +413,12 @@ macro(darwin_add_builtin_libraries) ../profile/InstrProfilingInternal.c ../profile/InstrProfilingVersionVar.c) foreach (os ${ARGN}) + set(macosx_sdk_version 99999) + if ("${os}" STREQUAL "osx") + find_darwin_sdk_version(macosx_sdk_version "macosx") + endif() + add_security_warnings(CFLAGS ${macosx_sdk_version}) + list_intersect(DARWIN_BUILTIN_ARCHS DARWIN_${os}_BUILTIN_ARCHS BUILTIN_SUPPORTED_ARCH) if((arm64 IN_LIST DARWIN_BUILTIN_ARCHS OR arm64e IN_LIST DARWIN_BUILTIN_ARCHS) AND NOT TARGET lse_builtin_symlinks) diff --git a/compiler-rt/cmake/Modules/CompilerRTUtils.cmake b/compiler-rt/cmake/Modules/CompilerRTUtils.cmake index 9b5e03a6607b..debd8a5a8bf1 100644 --- a/compiler-rt/cmake/Modules/CompilerRTUtils.cmake +++ b/compiler-rt/cmake/Modules/CompilerRTUtils.cmake @@ -593,3 +593,32 @@ function(add_compiler_rt_install_targets name) endif() endif() endfunction() + +# Add warnings to catch potential errors that can lead to security +# vulnerabilities. +function(add_security_warnings out_flags macosx_sdk_version) + set(flags "${${out_flags}}") + + append_list_if(COMPILER_RT_HAS_ARRAY_BOUNDS_FLAG -Werror=array-bounds flags) + append_list_if(COMPILER_RT_HAS_UNINITIALIZED_FLAG -Werror=uninitialized flags) + append_list_if(COMPILER_RT_HAS_SHADOW_FLAG -Werror=shadow flags) + append_list_if(COMPILER_RT_HAS_EMPTY_BODY_FLAG -Werror=empty-body flags) + append_list_if(COMPILER_RT_HAS_SIZEOF_POINTER_MEMACCESS_FLAG -Werror=sizeof-pointer-memaccess flags) + append_list_if(COMPILER_RT_HAS_SIZEOF_ARRAY_ARGUMENT_FLAG -Werror=sizeof-array-argument flags) + append_list_if(COMPILER_RT_HAS_MEMSET_TRANSPOSED_ARGS_FLAG -Werror=memset-transposed-args flags) + append_list_if(COMPILER_RT_HAS_BUILTIN_MEMCPY_CHK_SIZE_FLAG -Werror=builtin-memcpy-chk-size flags) + append_list_if(COMPILER_RT_HAS_ARRAY_BOUNDS_POINTER_ARITHMETIC_FLAG -Werror=array-bounds-pointer-arithmetic flags) + append_list_if(COMPILER_RT_HAS_RETURN_STACK_ADDRESS_FLAG -Werror=return-stack-address flags) + append_list_if(COMPILER_RT_HAS_SIZEOF_ARRAY_DECAY_FLAG -Werror=sizeof-array-decay flags) + append_list_if(COMPILER_RT_HAS_FORMAT_INSUFFICIENT_ARGS_FLAG -Werror=format-insufficient-args flags) + append_list_if(COMPILER_RT_HAS_BUILTIN_FORMAL_SECURITY_FLAG -Werror=format-security flags) + + # Add -Wformat-nonliteral only if we can avoid adding the definition of + # eprintf. On Apple platforms, eprintf is needed only on macosx and only if + # its version is older than 10.7. + if ("${macosx_sdk_version}" VERSION_GREATER_EQUAL 10.7) + list(APPEND flags -Werror=format-nonliteral -DDONT_DEFINE_EPRINTF) + endif() + + set(${out_flags} "${flags}" PARENT_SCOPE) +endfunction() diff --git a/compiler-rt/cmake/config-ix.cmake b/compiler-rt/cmake/config-ix.cmake index afa3596b1aa7..a1f8a200ef3b 100644 --- a/compiler-rt/cmake/config-ix.cmake +++ b/compiler-rt/cmake/config-ix.cmake @@ -137,6 +137,20 @@ check_cxx_compiler_flag(/wd4391 COMPILER_RT_HAS_WD4391_FLAG) check_cxx_compiler_flag(/wd4722 COMPILER_RT_HAS_WD4722_FLAG) check_cxx_compiler_flag(/wd4800 COMPILER_RT_HAS_WD4800_FLAG) +check_cxx_compiler_flag(-Warray-bounds COMPILER_RT_HAS_ARRAY_BOUNDS_FLAG) +check_cxx_compiler_flag(-Wuninitialized COMPILER_RT_HAS_UNINITIALIZED_FLAG) +check_cxx_compiler_flag(-Wshadow COMPILER_RT_HAS_SHADOW_FLAG) +check_cxx_compiler_flag(-Wempty-body COMPILER_RT_HAS_EMPTY_BODY_FLAG) +check_cxx_compiler_flag(-Wsizeof-pointer-memaccess COMPILER_RT_HAS_SIZEOF_POINTER_MEMACCESS_FLAG) +check_cxx_compiler_flag(-Wsizeof-array-argument COMPILER_RT_HAS_SIZEOF_ARRAY_ARGUMENT_FLAG) +check_cxx_compiler_flag(-Wmemset-transposed-args COMPILER_RT_HAS_MEMSET_TRANSPOSED_ARGS_FLAG) +check_cxx_compiler_flag(-Wbuiltin-memcpy-chk-size COMPILER_RT_HAS_BUILTIN_MEMCPY_CHK_SIZE_FLAG) +check_cxx_compiler_flag(-Warray-bounds-pointer-arithmetic COMPILER_RT_HAS_ARRAY_BOUNDS_POINTER_ARITHMETIC_FLAG) +check_cxx_compiler_flag(-Wreturn-stack-address COMPILER_RT_HAS_RETURN_STACK_ADDRESS_FLAG) +check_cxx_compiler_flag(-Wsizeof-array-decay COMPILER_RT_HAS_SIZEOF_ARRAY_DECAY_FLAG) +check_cxx_compiler_flag(-Wformat-insufficient-args COMPILER_RT_HAS_FORMAT_INSUFFICIENT_ARGS_FLAG) +check_cxx_compiler_flag(-Wformat-security COMPILER_RT_HAS_BUILTIN_FORMAL_SECURITY_FLAG) + # Symbols. check_symbol_exists(__func__ "" COMPILER_RT_HAS_FUNC_SYMBOL) diff --git a/compiler-rt/lib/builtins/CMakeLists.txt b/compiler-rt/lib/builtins/CMakeLists.txt index bbba2497fce3..716ececdea83 100644 --- a/compiler-rt/lib/builtins/CMakeLists.txt +++ b/compiler-rt/lib/builtins/CMakeLists.txt @@ -699,6 +699,7 @@ if (APPLE) darwin_add_builtin_libraries(${BUILTIN_SUPPORTED_OS}) else () set(BUILTIN_CFLAGS "") + add_security_warnings(BUILTIN_CFLAGS 0) if (COMPILER_RT_HAS_FCF_PROTECTION_FLAG) append_list_if(COMPILER_RT_ENABLE_CET -fcf-protection=full BUILTIN_CFLAGS) diff --git a/compiler-rt/lib/builtins/eprintf.c b/compiler-rt/lib/builtins/eprintf.c index 89fb0e315b2e..daf90b4993ec 100644 --- a/compiler-rt/lib/builtins/eprintf.c +++ b/compiler-rt/lib/builtins/eprintf.c @@ -15,6 +15,7 @@ // // It should never be exported from a dylib, so it is marked // visibility hidden. +#ifndef DONT_DEFINE_EPRINTF #ifndef _WIN32 __attribute__((visibility("hidden"))) #endif @@ -25,3 +26,4 @@ __eprintf(const char *format, const char *assertion_expression, fflush(stderr); compilerrt_abort(); } +#endif diff --git a/compiler-rt/lib/profile/InstrProfiling.c b/compiler-rt/lib/profile/InstrProfiling.c index 4bf8463f1ef9..fdb7b7cd806c 100644 --- a/compiler-rt/lib/profile/InstrProfiling.c +++ b/compiler-rt/lib/profile/InstrProfiling.c @@ -64,11 +64,11 @@ COMPILER_RT_VISIBILITY void __llvm_profile_reset_counters(void) { CurrentVSiteCount += DI->NumValueSites[VKI]; for (i = 0; i < CurrentVSiteCount; ++i) { - ValueProfNode *CurrentVNode = ValueCounters[i]; + ValueProfNode *CurrVNode = ValueCounters[i]; - while (CurrentVNode) { - CurrentVNode->Count = 0; - CurrentVNode = CurrentVNode->Next; + while (CurrVNode) { + CurrVNode->Count = 0; + CurrVNode = CurrVNode->Next; } } } diff --git a/compiler-rt/lib/profile/InstrProfilingWriter.c b/compiler-rt/lib/profile/InstrProfilingWriter.c index 800103d199ab..6fe13308a522 100644 --- a/compiler-rt/lib/profile/InstrProfilingWriter.c +++ b/compiler-rt/lib/profile/InstrProfilingWriter.c @@ -263,11 +263,11 @@ lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin, (__llvm_profile_get_version() & VARIANT_MASK_DBG_CORRELATE) != 0ULL; /* Calculate size of sections. */ - const uint64_t DataSize = + const uint64_t DataSectionSize = DebugInfoCorrelate ? 0 : __llvm_profile_get_data_size(DataBegin, DataEnd); const uint64_t NumData = DebugInfoCorrelate ? 0 : __llvm_profile_get_num_data(DataBegin, DataEnd); - const uint64_t CountersSize = + const uint64_t CountersSectionSize = __llvm_profile_get_counters_size(CountersBegin, CountersEnd); const uint64_t NumCounters = __llvm_profile_get_num_counters(CountersBegin, CountersEnd); @@ -284,8 +284,9 @@ lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin, uint64_t PaddingBytesBeforeCounters, PaddingBytesAfterCounters, PaddingBytesAfterNames; __llvm_profile_get_padding_sizes_for_counters( - DataSize, CountersSize, NamesSize, &PaddingBytesBeforeCounters, - &PaddingBytesAfterCounters, &PaddingBytesAfterNames); + DataSectionSize, CountersSectionSize, NamesSize, + &PaddingBytesBeforeCounters, &PaddingBytesAfterCounters, + &PaddingBytesAfterNames); { // TODO: Unfortunately the header's fields are named DataSize and @@ -321,9 +322,10 @@ lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin, /* Write the profile data. */ ProfDataIOVec IOVecData[] = { - {DebugInfoCorrelate ? NULL : DataBegin, sizeof(uint8_t), DataSize, 0}, + {DebugInfoCorrelate ? NULL : DataBegin, sizeof(uint8_t), DataSectionSize, + 0}, {NULL, sizeof(uint8_t), PaddingBytesBeforeCounters, 1}, - {CountersBegin, sizeof(uint8_t), CountersSize, 0}, + {CountersBegin, sizeof(uint8_t), CountersSectionSize, 0}, {NULL, sizeof(uint8_t), PaddingBytesAfterCounters, 1}, {(SkipNameDataWrite || DebugInfoCorrelate) ? NULL : NamesBegin, sizeof(uint8_t), NamesSize, 0},