Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nsan] Add nsan_preinit.cpp and make it static library only #98564

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jul 11, 2024

#94322 defines .preinit_array to initialize nsan early.
DT_PREINIT_ARRAY can only be used with the main executable. GNU ld would
complain when a DSO has .preinit_array. Therefore,
nsan_preinit.cpp cannot be linked into libclang_rt.nsan.so (#98415).

Working with @alexander-shaposhnikov, we noticed that Nsan-x86_64-Test --gtest_output=json without .preinit_array will sigsegv. This is
because googletest with the JSON output calls localtime_r , which
calls free(0) and fails when REAL(free) remains uninitialized
(nullptr). This is benign with the default output because malloc/free
are all paired and REAL(free)(ptr) is not called.

To fix the unittest failure, __nsan_init needs to be called early
(.preinit_array).
asan/tests/CMakeLists.txt:ASAN_UNITTEST_INSTRUMENTED_LINK_FLAGS ues
-fsanitize=address to ensure asan_preinit.cpp.o is linked into the
unittest executable. Port the approach and remove
NSAN_TEST_RUNTIME_OBJECTS.

Fix #98523

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 11, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Fangrui Song (MaskRay)

Changes

#94322 defines .preinit_array to initialize nsan early.
DT_PREINIT_ARRAY can only be used with the main executable. GNU ld would
complain when a DSO has .preinit_array. Therefore,
nsan_preinit.cpp cannot be linked into libclang_rt.nsan.so.

Working with @alexander-shaposhnikov, we noticed that Nsan-x86_64-Test --gtest_output=json without .preinit_array will sigsegv.
This is because googletest calls timing functions, which call free (intercepted).
__nsan_init needs to be called early (.preinit_array) to make the
free interceptor work.

The robust way to ensure nsan_preinit.cpp.o is used is to link the
unittest executable with -fsanitize=numerical and remove
NSAN_TEST_RUNTIME_OBJECTS.


Full diff: https://github.com/llvm/llvm-project/pull/98564.diff

5 Files Affected:

  • (modified) compiler-rt/lib/nsan/CMakeLists.txt (+10-1)
  • (modified) compiler-rt/lib/nsan/nsan.cpp (-5)
  • (modified) compiler-rt/lib/nsan/nsan.h (+2)
  • (added) compiler-rt/lib/nsan/nsan_preinit.cpp (+21)
  • (modified) compiler-rt/lib/nsan/tests/CMakeLists.txt (+3-18)
diff --git a/compiler-rt/lib/nsan/CMakeLists.txt b/compiler-rt/lib/nsan/CMakeLists.txt
index d67c3ac434d3a..1e138d4560c89 100644
--- a/compiler-rt/lib/nsan/CMakeLists.txt
+++ b/compiler-rt/lib/nsan/CMakeLists.txt
@@ -11,6 +11,9 @@ set(NSAN_SOURCES
   nsan_suppressions.cpp
 )
 
+set(NSAN_PREINIT_SOURCES
+  nsan_preinit.cpp)
+
 set(NSAN_HEADERS
   nsan.h
   nsan_flags.h
@@ -61,6 +64,12 @@ if(NOT APPLE)
       ADDITIONAL_HEADERS ${NSAN_HEADERS}
       CFLAGS ${NSAN_CFLAGS})
 
+  add_compiler_rt_object_libraries(RTNsan_preinit
+    ARCHS ${NSAN_SUPPORTED_ARCH}
+    SOURCES ${NSAN_PREINIT_SOURCES}
+    ADDITIONAL_HEADERS ${NSAN_HEADERS}
+    CFLAGS ${NSAN_CFLAGS})
+
   file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/dummy.cpp "")
   add_compiler_rt_object_libraries(RTNsan_dynamic_version_script_dummy
     ARCHS ${NSAN_SUPPORTED_ARCH}
@@ -72,7 +81,7 @@ add_compiler_rt_runtime(
   clang_rt.nsan
   STATIC
   ARCHS ${NSAN_SUPPORTED_ARCH}
-  OBJECT_LIBS RTNsan
+  OBJECT_LIBS RTNsan_preinit RTNsan
               ${NSAN_COMMON_RUNTIME_OBJECT_LIBS}
   CFLAGS ${NSAN_CFLAGS}
   PARENT_TARGET nsan)
diff --git a/compiler-rt/lib/nsan/nsan.cpp b/compiler-rt/lib/nsan/nsan.cpp
index f7b2ce2048290..7a5f013579dfb 100644
--- a/compiler-rt/lib/nsan/nsan.cpp
+++ b/compiler-rt/lib/nsan/nsan.cpp
@@ -801,8 +801,3 @@ extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __nsan_init() {
   nsan_init_is_running = false;
   nsan_initialized = true;
 }
-
-#if SANITIZER_CAN_USE_PREINIT_ARRAY
-__attribute__((section(".preinit_array"),
-               used)) static void (*nsan_init_ptr)() = __nsan_init;
-#endif
diff --git a/compiler-rt/lib/nsan/nsan.h b/compiler-rt/lib/nsan/nsan.h
index 0fb998b049854..8277e3d778047 100644
--- a/compiler-rt/lib/nsan/nsan.h
+++ b/compiler-rt/lib/nsan/nsan.h
@@ -32,6 +32,8 @@ using __sanitizer::uptr;
 // Private nsan interface. Used e.g. by interceptors.
 extern "C" {
 
+void __nsan_init();
+
 // This marks the shadow type of the given block of application memory as
 // unknown.
 // printf-free (see comment in nsan_interceptors.cc).
diff --git a/compiler-rt/lib/nsan/nsan_preinit.cpp b/compiler-rt/lib/nsan/nsan_preinit.cpp
new file mode 100644
index 0000000000000..5d869bbdf9668
--- /dev/null
+++ b/compiler-rt/lib/nsan/nsan_preinit.cpp
@@ -0,0 +1,21 @@
+//===- nsan_preinit.cpp ---------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Call __nsan_init early using ELF DT_PREINIT_ARRAY.
+//
+//===----------------------------------------------------------------------===//
+
+#include "nsan/nsan.h"
+#include "sanitizer_common/sanitizer_internal_defs.h"
+
+#if SANITIZER_CAN_USE_PREINIT_ARRAY
+
+__attribute__((section(".preinit_array"), used)) static auto nsan_preinit =
+    __nsan_init;
+
+#endif
diff --git a/compiler-rt/lib/nsan/tests/CMakeLists.txt b/compiler-rt/lib/nsan/tests/CMakeLists.txt
index e472fc5c06f90..ecb1aa9c56cf2 100644
--- a/compiler-rt/lib/nsan/tests/CMakeLists.txt
+++ b/compiler-rt/lib/nsan/tests/CMakeLists.txt
@@ -15,6 +15,8 @@ set(NSAN_UNITTEST_LINK_FLAGS
   ${COMPILER_RT_UNITTEST_LINK_FLAGS}
   ${COMPILER_RT_UNWINDER_LINK_LIBS}
   ${SANITIZER_TEST_CXX_LIBRARIES})
+set(NSAN_UNITTEST_INSTRUMENTED_LINK_FLAGS ${NSAN_UNITTEST_LINK_FLAGS})
+list(APPEND NSAN_UNITTEST_INSTRUMENTED_LINK_FLAGS -fsanitize=numerical)
 
 file(GLOB NSAN_HEADERS ../*.h)
 set(NSAN_UNITTESTS
@@ -27,30 +29,13 @@ if(COMPILER_RT_DEFAULT_TARGET_ARCH IN_LIST NSAN_SUPPORTED_ARCH)
   # NSan unit tests are only run on the host machine.
   set(arch ${COMPILER_RT_DEFAULT_TARGET_ARCH})
 
-  set(NSAN_TEST_RUNTIME RTNsanTest.${arch})
-
-  set(NSAN_TEST_RUNTIME_OBJECTS
-    $<TARGET_OBJECTS:RTNsan.${arch}>
-    $<TARGET_OBJECTS:RTInterception.${arch}>
-    $<TARGET_OBJECTS:RTSanitizerCommon.${arch}>
-    $<TARGET_OBJECTS:RTSanitizerCommonLibc.${arch}>
-    $<TARGET_OBJECTS:RTSanitizerCommonSymbolizer.${arch}>)
-
-  add_library(${NSAN_TEST_RUNTIME} STATIC
-    ${NSAN_TEST_RUNTIME_OBJECTS})
-
-  set_target_properties(${NSAN_TEST_RUNTIME} PROPERTIES
-    ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
-    FOLDER "Compiler-RT Runtime tests")
-
   set(NsanTestObjects)
   generate_compiler_rt_tests(NsanTestObjects
     NsanUnitTests "Nsan-${arch}-Test" ${arch}
     SOURCES ${NSAN_UNITTESTS} ${COMPILER_RT_GTEST_SOURCE}
-    RUNTIME ${NSAN_TEST_RUNTIME}
     DEPS ${NSAN_UNIT_TEST_HEADERS}
     CFLAGS ${NSAN_UNITTEST_CFLAGS}
-    LINK_FLAGS ${NSAN_UNITTEST_LINK_FLAGS})
+    LINK_FLAGS ${NSAN_UNITTEST_INSTRUMENTED_LINK_FLAGS})
   set_target_properties(NsanUnitTests PROPERTIES
     RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
 endif()

@alexander-shaposhnikov
Copy link
Collaborator

Thanks! I guess this PR resolves #98523 and the issues reported on #98415 .

Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit a853fe2 into main Jul 12, 2024
3 of 5 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/nsan-add-nsan_preinitcpp-and-make-it-static-library-only branch July 12, 2024 01:22
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
llvm#94322 defines .preinit_array to initialize nsan early.
DT_PREINIT_ARRAY can only be used with the main executable. GNU ld would
complain when a DSO has .preinit_array. Therefore,
nsan_preinit.cpp cannot be linked into `libclang_rt.nsan.so` (llvm#98415).

Working with @alexander-shaposhnikov, we noticed that `Nsan-x86_64-Test
--gtest_output=json` without `.preinit_array` will sigsegv. This is
because googletest with the JSON output calls `localtime_r` , which
calls `free(0)` and fails when `REAL(free)` remains uninitialized
(nullptr). This is benign with the default output because malloc/free
are all paired and `REAL(free)(ptr)` is not called.

To fix the unittest failure, `__nsan_init` needs to be called early
(.preinit_array).
`asan/tests/CMakeLists.txt:ASAN_UNITTEST_INSTRUMENTED_LINK_FLAGS` ues
`-fsanitize=address` to ensure `asan_preinit.cpp.o` is linked into the
unittest executable. Port the approach and remove
`NSAN_TEST_RUNTIME_OBJECTS`.

Fix llvm#98523

Pull Request: llvm#98564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attempt to build llvm 19 keeps failing recently
3 participants