[compiler-rt][builtins] Add compiler flags to catch potential errors that can lead to security vulnerabilities

[ahatanak]

Summary

Also, fix a few places that were causing -Wshadow and -Wformat-nonliteral warnings to be emitted.

[ahatanak]

compiler-rt/lib/builtins/CMakeLists.txt

702

I'm passing -1 here since I wasn't sure whether the definition of eprintf is needed on non-apple platforms. If it isn't needed. we can pass 0 instead.

[ahatanak]

AFAICT, on Apple's platforms, __eprintf is needed only on OSX and only if the version is older than 10.7.

https://opensource.apple.com/source/Libc/Libc-1439.40.11/include/assert.h.auto.html

[ravikandhadai]

Thanks Akira.

[ahatanak]

@dcoughlin @kubamracek @compnerd any comments before I commit this patch?

[glandium]

FWIW, it also broke compiler-rt standalone builds:

CMake Error at lib/builtins/CMakeLists.txt:702 (add_security_warnings):
  Unknown CMake command "add_security_warnings".

[vitalybuka]

This patches breaks windows bot https://lab.llvm.org/buildbot/#/builders/127/builds/34791

[ahatanak]

Fixed in 387a05dbfa21631e924ee8dc30a0f8db8e29c3b2.

[ahatanak]

It's still failing. It looks like I have to add checks for other flags too.

[haowei]

We are also saw failures on Fuchsia builders when using ToT clang:

[31281/267743] CXX host_x64/obj/src/lib/llvm-profdata/llvm-profdata-for-test.llvm-profdata.cc.o
FAILED: host_x64/obj/src/lib/llvm-profdata/llvm-profdata-for-test.llvm-profdata.cc.o
../../../recipe_cleanup/clang2x5u_bg_/bin/clang++ -MD -MF host_x64/obj/src/lib/llvm-profdata/llvm-profdata-for-test.llvm-profdata.cc.o.d -DHAVE_PROFDATA=1 -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -...
In file included from ../../src/lib/llvm-profdata/llvm-profdata.cc:224:
../../../recipe_cleanup/clang2x5u_bg_/lib/clang/16.0.0/include/profile/InstrProfData.inc:132:43: error: use of undeclared identifier 'DataSizeInitVal'
INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSizeInitVal)
                                          ^
../../../recipe_cleanup/clang2x5u_bg_/lib/clang/16.0.0/include/profile/InstrProfData.inc:135:47: error: use of undeclared identifier 'CountersSizeInitVal'
INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSizeInitVal)
                                              ^
2 errors generated.
clang++: error: failing because '-gen-reproducer' is used
Fuchsia clang version 16.0.0 (https://llvm.googlesource.com/llvm-project 0473ac8876b94fc27f145c48106675b9dedcb20d)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: ../../../recipe_cleanup/clang2x5u_bg_/bin
clang++: note: diagnostic msg:
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang++: note: diagnostic msg: clang-crashreports/llvm-profdata-424b79.cpp
clang++: note: diagnostic msg: clang-crashreports/llvm-profdata-424b79.sh
clang++: note: diagnostic msg:

********************

Failing task: https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-debug-tot-build_only/b8804902579506128897/overview

Could you revert your change if it takes a while to fix forward?

[haowei]

After looking into the code, I think I should point out that compiler-rt/include/profile/InstrProfData.inc is a published header and its API details cannot be changed trivially without coordination with external users, so seemingly cosmetic changes should be avoided if they might require changes for users of the header, which includes changing any of the identifiers. In our case, Fuchsia uses the API in this file and due to this change, we are seeing build breakages.

At this point. I recommend to revert all related changes to this code review instead of trying to attempt to fix the problem multiple times without success.

[vitalybuka]

In D131714#3750091, @ahatanak wrote:

It's still failing. It looks like I have to add checks for other flags too.

Thank you! Windows bot is fixed https://lab.llvm.org/buildbot/#/builders/127/builds/34851

[ahatanak]

Revert the changes made to InstrProfData.inc and renamed the variables in InstrProfilingWriter.c instead.

[wlei]

Hi @ahatanak

This or its child diff broke our internal build, I saw it was reverted but the reapplied diff(https://github.com/llvm/llvm-project/commit/2e9df860468425645dcd1b241c5dbf76c072e314) still broke our build.

see the log:
(1)floatundidf.S.o :

FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundidf.S.o
/home/engshare/third-party2/gcc/11.x/centos7-native/886b5eb/bin/gcc -DVISIBILITY_HIDDEN -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins -Iinclude -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/llvm/include -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/llvm-project=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/build-platform010/build=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fPIC -O3 -DNDEBUG -m64 -fno-lto -Werror=array-bounds -Werror=uninitialized -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess -Werror=sizeof-array-argument -Werror=memset-transposed-args -Werror=format-security -std=c11 -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundidf.S.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundidf.S.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundidf.S.o -c /home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins/x86_64/floatundidf.S
cc1: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]
cc1: some warnings being treated as errors
[794/5411] /home/engshare/third-party2/gcc/11.x/centos7-native/886b5eb/bin/gcc -DVISIBILITY_HIDDEN -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins -Iinclude -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/llvm/include -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/llvm-project=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/build-platform010/build=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fPIC -O3 -DNDEBUG -m64 -fno-lto -Werror=array-bounds -Werror=uninitialized -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess -Werror=sizeof-array-argument -Werror=memset-transposed-args -Werror=format-security -std=c11 -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -c /home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins/x86_64/floatundisf.S
FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o
/home/engshare/third-party2/gcc/11.x/centos7-native/886b5eb/bin/gcc -DVISIBILITY_HIDDEN -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins -Iinclude -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/llvm/include -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/llvm-project=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/build-platform010/build=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fPIC -O3 -DNDEBUG -m64 -fno-lto -Werror=array-bounds -Werror=uninitialized -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess -Werror=sizeof-array-argument -Werror=memset-transposed-args -Werror=format-security -std=c11 -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundisf.S.o -c /home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins/x86_64/floatundisf.S
cc1: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]

(2) floatundixf.S.o

FAILED: projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundixf.S.o 
/home/engshare/third-party2/gcc/11.x/centos7-native/886b5eb/bin/gcc -DVISIBILITY_HIDDEN -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/builtins -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins -Iinclude -I/home/engshare/third-party2/llvm-fb/12/src/llvm-project/llvm/include -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/llvm-project=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fdebug-prefix-map=/home/engshare/third-party2/llvm-fb/12/src/build-platform010/build=/home/engshare/third-party2/llvm-fb/12/src/llvm-project -fPIC -O3 -DNDEBUG -m64 -fno-lto -Werror=array-bounds -Werror=uninitialized -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess -Werror=sizeof-array-argument -Werror=memset-transposed-args -Werror=format-security -std=c11 -MD -MT projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundixf.S.o -MF projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundixf.S.o.d -o projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins-x86_64.dir/x86_64/floatundixf.S.o -c /home/engshare/third-party2/llvm-fb/12/src/llvm-project/compiler-rt/lib/builtins/x86_64/floatundixf.S
cc1: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]
cc1: some warnings being treated as errors

Please take a look and let me know if you need any thing from my side. Also please revert it if you think it will takes long time to fix it, Thank you!

[ahatanak]

Fixed in f051c1ded40970169cda84b0966331ae7ad424ed. It looks like gcc, unlike clang, doesn't allow passing -Werror=format-security without passing -format too.

[wlei]

In D131714#3754399, @ahatanak wrote:

Fixed in f051c1ded40970169cda84b0966331ae7ad424ed. It looks like gcc, unlike clang, doesn't allow passing -Werror=format-security without passing -format too.

Confirmed it's fixed, thanks @ahatanak!

compiler-rt/cmake/Modules/CompilerRTCompile.cmake

foreach(flag ${msvc_flags})
     elseif ("${flag}" MATCHES "^-O[2x]")
       # Canonicalize normal optimization flags to -O2.
       list(APPEND clang_flags "-O2")
     endif()
   endforeach()
   set(${out_flags} "${clang_flags}" PARENT_SCOPE)
 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}}" -Werror=builtin-memcpy-chk-size -Werror=format-security
+      -Werror=array-bounds -Werror=uninitialized -Werror=array-bounds-pointer-arithmetic
+      -Werror=shadow -Werror=empty-body -Werror=sizeof-pointer-memaccess
+      -Werror=return-stack-address -Werror=sizeof-array-decay -Werror=sizeof-array-argument
+      -Werror=memset-transposed-args -Werror=format-insufficient-args)
+
+  # Add -Wformat-nonliteral only if we can avoid adding the defintion 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 OR
+      "${macosx_sdk_version}" EQUAL 0)
+    set(flags "${flags}" -Werror=format-nonliteral -DDONT_DEFINE_EPRINTF)
+  endif()
+
+  set(${out_flags} "${flags}" PARENT_SCOPE)
+endfunction()
+
 # Compile a sanitizer test with a freshly built clang
 # for a given architecture, adding the result to the object list.
 #  - obj_list: output list of objects, populated by path
 #              of a generated object file.
 #  - source:   source file of a test.
 #  - arch:     architecture to compile for.
 # sanitizer_test_compile(<obj_list> <source> <arch>
 #                        KIND <custom namespace>

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

macro(darwin_add_builtin_libraries)
 
   set(PROFILE_SOURCES ../profile/InstrProfiling.c
                       ../profile/InstrProfilingBuffer.c
                       ../profile/InstrProfilingPlatformDarwin.c
                       ../profile/InstrProfilingWriter.c
                       ../profile/InstrProfilingInternal.c
                       ../profile/InstrProfilingVersionVar.c)
   foreach (os ${ARGN})
+    set(macosx_sdk_version 0)
+    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)
       add_custom_target(
         lse_builtin_symlinks
         BYPRODUCTS ${lse_builtins}
         ${arm64_lse_commands}
       )

compiler-rt/include/profile/InstrProfData.inc

 #define INSTR_PROF_RAW_HEADER(Type, Name, Initializer)
 #else
 #define INSTR_PROF_DATA_DEFINED
 #endif
 INSTR_PROF_RAW_HEADER(uint64_t, Magic, __llvm_profile_get_magic())
 INSTR_PROF_RAW_HEADER(uint64_t, Version, __llvm_profile_get_version())
 INSTR_PROF_RAW_HEADER(uint64_t, BinaryIdsSize, __llvm_write_binary_ids(NULL))
 /* FIXME: A more accurate name is NumData */
-INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSize)
+INSTR_PROF_RAW_HEADER(uint64_t, DataSize, DataSizeInitVal)
 INSTR_PROF_RAW_HEADER(uint64_t, PaddingBytesBeforeCounters, PaddingBytesBeforeCounters)
 /* FIXME: A more accurate name is NumCounters */
-INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSize)
+INSTR_PROF_RAW_HEADER(uint64_t, CountersSize, CountersSizeInitVal)
 INSTR_PROF_RAW_HEADER(uint64_t, PaddingBytesAfterCounters, PaddingBytesAfterCounters)
 INSTR_PROF_RAW_HEADER(uint64_t, NamesSize,  NamesSize)
 INSTR_PROF_RAW_HEADER(uint64_t, CountersDelta,
                       (uintptr_t)CountersBegin - (uintptr_t)DataBegin)
 INSTR_PROF_RAW_HEADER(uint64_t, NamesDelta, (uintptr_t)NamesBegin)
 INSTR_PROF_RAW_HEADER(uint64_t, ValueKindLast, IPVK_Last)
 #undef INSTR_PROF_RAW_HEADER
 /* INSTR_PROF_RAW_HEADER  end */

compiler-rt/lib/builtins/CMakeLists.txt

 set_target_properties(builtins PROPERTIES FOLDER "Compiler-RT Misc")
 
 if (APPLE)
   add_subdirectory(Darwin-excludes)
   add_subdirectory(macho_embedded)
   darwin_add_builtin_libraries(${BUILTIN_SUPPORTED_OS})
 else ()
   set(BUILTIN_CFLAGS "")
+  add_security_warnings(BUILTIN_CFLAGS -1)
 
   if (COMPILER_RT_HAS_FCF_PROTECTION_FLAG)
     append_list_if(COMPILER_RT_ENABLE_CET -fcf-protection=full BUILTIN_CFLAGS)
   endif()
 
   append_list_if(COMPILER_RT_HAS_FLOAT16 -DCOMPILER_RT_HAS_FLOAT16 BUILTIN_CFLAGS)
 
   append_list_if(COMPILER_RT_HAS_STD_C11_FLAG -std=c11 BUILTIN_CFLAGS)

compiler-rt/lib/builtins/eprintf.c

 #include <stdio.h>
 
 // __eprintf() was used in an old version of <assert.h>.
 // It can eventually go away, but it is needed when linking
 // .o files built with the old <assert.h>.
 //
 // It should never be exported from a dylib, so it is marked
 // visibility hidden.
+#ifndef DONT_DEFINE_EPRINTF
 #ifndef _WIN32
 __attribute__((visibility("hidden")))
 #endif
 COMPILER_RT_ABI void
 __eprintf(const char *format, const char *assertion_expression,
           const char *line, const char *file) {
   fprintf(stderr, format, assertion_expression, line, file);
   fflush(stderr);
   compilerrt_abort();
 }
+#endif

compiler-rt/lib/profile/InstrProfiling.c

if (!DI->Values)
       continue;
 
     ValueProfNode **ValueCounters = (ValueProfNode **)DI->Values;
 
     for (VKI = IPVK_First; VKI <= IPVK_Last; ++VKI)
       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;
       }
     }
   }
   lprofSetProfileDumped(0);
 }

compiler-rt/lib/profile/InstrProfilingWriter.c

lprofWriteDataImpl(ProfDataWriter *Writer, const __llvm_profile_data *DataBegin,
   __llvm_profile_get_padding_sizes_for_counters(
       DataSize, CountersSize, NamesSize, &PaddingBytesBeforeCounters,
       &PaddingBytesAfterCounters, &PaddingBytesAfterNames);
 
   {
     // TODO: Unfortunately the header's fields are named DataSize and
     // CountersSize when they should be named NumData and NumCounters,
     // respectively.
-    const uint64_t CountersSize = NumCounters;
-    const uint64_t DataSize = NumData;
+    const uint64_t CountersSizeInitVal = NumCounters;
+    const uint64_t DataSizeInitVal = NumData;
 /* Initialize header structure.  */
 #define INSTR_PROF_RAW_HEADER(Type, Name, Init) Header.Name = Init;
 #include "profile/InstrProfData.inc"
   }
 
   /* On WIN64, label differences are truncated 32-bit values. Truncate
    * CountersDelta to match. */
 #ifdef _WIN64