Skip to content

[libc][LIBC_ADD_NULL_CHECKS] replace volatile deref with __builtin_trap #123401

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

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

nickdesaulniers
Copy link
Member

@nickdesaulniers nickdesaulniers commented Jan 17, 2025

Also, update the unit tests that were checking for SIGSEGV to not check for a specific signal.

To further improve this check, it may be worth:

  • renaming the configuration option/macro/docs to be clearer about intent.
  • swap __builtin_trap for __builtin_unreachable, removing the preprocessor
    variants of LIBC_CRASH_ON_NULLPTR, then unconditionally using
    -fsanitize=unreachable -fsanitize-trap=unreachable in cmake flags when
    LIBC_ADD_NULL_CHECKS is enabled.
  • building with -fno-delete-null-pointer-checks when LIBC_ADD_NULL_CHECKS (or
    when some larger yet to be added hardening config) is enabled.

Link: #111546

Also, update the unit tests that were checking for SIGSEGV to check for SIGILL.

To further improve this check, it may be worth:
- renaming the configuration option/macro/docs to be clearer about intent.
- swap __builtin_trap for __builtin_unreachable, removing the preprocessor
  variants of LIBC_CRASH_ON_NULLPTR, then unconditionally using
  `-fsanitize=unreachable -fsanitize-trap=unreachable` in cmake flags when
  LIBC_ADD_NULL_CHECKS is enabled.
- building with `-fno-delete-null-pointer-checks` when LIBC_ADD_NULL_CHECKS (or
  when some larger yet to be added hardening config) is enabled.
@nickdesaulniers
Copy link
Member Author

cc @AlyElashram #116262 #111546

@llvmbot
Copy link
Member

llvmbot commented Jan 17, 2025

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

Also, update the unit tests that were checking for SIGSEGV to check for SIGILL.

To further improve this check, it may be worth:

  • renaming the configuration option/macro/docs to be clearer about intent.
  • swap __builtin_trap for __builtin_unreachable, removing the preprocessor
    variants of LIBC_CRASH_ON_NULLPTR, then unconditionally using
    -fsanitize=unreachable -fsanitize-trap=unreachable in cmake flags when
    LIBC_ADD_NULL_CHECKS is enabled.
  • building with -fno-delete-null-pointer-checks when LIBC_ADD_NULL_CHECKS (or
    when some larger yet to be added hardening config) is enabled.

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

6 Files Affected:

  • (modified) libc/src/__support/macros/null_check.h (+2-7)
  • (modified) libc/test/src/math/smoke/nan_test.cpp (+2-2)
  • (modified) libc/test/src/math/smoke/nanf128_test.cpp (+2-2)
  • (modified) libc/test/src/math/smoke/nanf16_test.cpp (+2-2)
  • (modified) libc/test/src/math/smoke/nanf_test.cpp (+2-2)
  • (modified) libc/test/src/math/smoke/nanl_test.cpp (+2-2)
diff --git a/libc/src/__support/macros/null_check.h b/libc/src/__support/macros/null_check.h
index 400f7d809db4fa..eda19f889235e4 100644
--- a/libc/src/__support/macros/null_check.h
+++ b/libc/src/__support/macros/null_check.h
@@ -14,15 +14,10 @@
 #include "src/__support/macros/sanitizer.h"
 
 #if defined(LIBC_ADD_NULL_CHECKS) && !defined(LIBC_HAS_SANITIZER)
-// Use volatile to prevent undefined behavior of dereferencing nullptr.
-// Intentionally crashing with SIGSEGV.
-#define LIBC_CRASH_ON_NULLPTR(PTR)                                             \
+#define LIBC_CRASH_ON_NULLPTR(ptr)                                             \
   do {                                                                         \
-    if (LIBC_UNLIKELY(PTR == nullptr)) {                                       \
-      volatile auto *crashing = PTR;                                           \
-      [[maybe_unused]] volatile auto crash = *crashing;                        \
+    if (LIBC_UNLIKELY((ptr) == nullptr))                                       \
       __builtin_trap();                                                        \
-    }                                                                          \
   } while (0)
 #else
 #define LIBC_CRASH_ON_NULLPTR(ptr)                                             \
diff --git a/libc/test/src/math/smoke/nan_test.cpp b/libc/test/src/math/smoke/nan_test.cpp
index 46b9e9aa9563ab..579e37c4268f7d 100644
--- a/libc/test/src/math/smoke/nan_test.cpp
+++ b/libc/test/src/math/smoke/nan_test.cpp
@@ -44,8 +44,8 @@ TEST_F(LlvmLibcNanTest, RandomString) {
   run_test("123 ", 0x7ff8000000000000);
 }
 
-#if !defined(LIBC_HAS_ADDRESS_SANITIZER) && defined(LIBC_TARGET_OS_IS_LINUX)
+#if defined(LIBC_ADD_NULL_CHECKS) && !defined(LIBC_HAS_SANITIZER)
 TEST_F(LlvmLibcNanTest, InvalidInput) {
-  EXPECT_DEATH([] { LIBC_NAMESPACE::nan(nullptr); }, WITH_SIGNAL(SIGSEGV));
+  EXPECT_DEATH([] { LIBC_NAMESPACE::nan(nullptr); }, WITH_SIGNAL(SIGILL));
 }
 #endif // LIBC_HAS_ADDRESS_SANITIZER
diff --git a/libc/test/src/math/smoke/nanf128_test.cpp b/libc/test/src/math/smoke/nanf128_test.cpp
index 25dd2ef1d5b1ca..bc5b4f0d9afc6d 100644
--- a/libc/test/src/math/smoke/nanf128_test.cpp
+++ b/libc/test/src/math/smoke/nanf128_test.cpp
@@ -55,8 +55,8 @@ TEST_F(LlvmLibcNanf128Test, RandomString) {
            QUIET_NAN);
 }
 
-#if !defined(LIBC_HAS_ADDRESS_SANITIZER) && defined(LIBC_TARGET_OS_IS_LINUX)
+#if defined(LIBC_ADD_NULL_CHECKS) && !defined(LIBC_HAS_SANITIZER)
 TEST_F(LlvmLibcNanf128Test, InvalidInput) {
-  EXPECT_DEATH([] { LIBC_NAMESPACE::nanf128(nullptr); }, WITH_SIGNAL(SIGSEGV));
+  EXPECT_DEATH([] { LIBC_NAMESPACE::nanf128(nullptr); }, WITH_SIGNAL(SIGILL));
 }
 #endif // LIBC_HAS_ADDRESS_SANITIZER
diff --git a/libc/test/src/math/smoke/nanf16_test.cpp b/libc/test/src/math/smoke/nanf16_test.cpp
index ec640a3b9eef92..66f8963c971305 100644
--- a/libc/test/src/math/smoke/nanf16_test.cpp
+++ b/libc/test/src/math/smoke/nanf16_test.cpp
@@ -43,8 +43,8 @@ TEST_F(LlvmLibcNanf16Test, RandomString) {
   run_test("123 ", 0x7e00);
 }
 
-#if !defined(LIBC_HAS_ADDRESS_SANITIZER) && defined(LIBC_TARGET_OS_IS_LINUX)
+#if defined(LIBC_ADD_NULL_CHECKS) && !defined(LIBC_HAS_SANITIZER)
 TEST_F(LlvmLibcNanf16Test, InvalidInput) {
-  EXPECT_DEATH([] { LIBC_NAMESPACE::nanf16(nullptr); }, WITH_SIGNAL(SIGSEGV));
+  EXPECT_DEATH([] { LIBC_NAMESPACE::nanf16(nullptr); }, WITH_SIGNAL(SIGILL));
 }
 #endif // LIBC_HAS_ADDRESS_SANITIZER
diff --git a/libc/test/src/math/smoke/nanf_test.cpp b/libc/test/src/math/smoke/nanf_test.cpp
index dd3124ee9c5112..7795803ac203ef 100644
--- a/libc/test/src/math/smoke/nanf_test.cpp
+++ b/libc/test/src/math/smoke/nanf_test.cpp
@@ -43,8 +43,8 @@ TEST_F(LlvmLibcNanfTest, RandomString) {
   run_test("123 ", 0x7fc00000);
 }
 
-#if !defined(LIBC_HAS_ADDRESS_SANITIZER) && defined(LIBC_TARGET_OS_IS_LINUX)
+#if defined(LIBC_ADD_NULL_CHECKS) && !defined(LIBC_HAS_SANITIZER)
 TEST_F(LlvmLibcNanfTest, InvalidInput) {
-  EXPECT_DEATH([] { LIBC_NAMESPACE::nanf(nullptr); }, WITH_SIGNAL(SIGSEGV));
+  EXPECT_DEATH([] { LIBC_NAMESPACE::nanf(nullptr); }, WITH_SIGNAL(SIGILL));
 }
 #endif // LIBC_HAS_ADDRESS_SANITIZER
diff --git a/libc/test/src/math/smoke/nanl_test.cpp b/libc/test/src/math/smoke/nanl_test.cpp
index ef3f9c15dafd9f..e07297813d6040 100644
--- a/libc/test/src/math/smoke/nanl_test.cpp
+++ b/libc/test/src/math/smoke/nanl_test.cpp
@@ -71,8 +71,8 @@ TEST_F(LlvmLibcNanlTest, RandomString) {
   run_test("123 ", expected);
 }
 
-#if !defined(LIBC_HAS_ADDRESS_SANITIZER) && defined(LIBC_TARGET_OS_IS_LINUX)
+#if defined(LIBC_ADD_NULL_CHECKS) && !defined(LIBC_HAS_SANITIZER)
 TEST_F(LlvmLibcNanlTest, InvalidInput) {
-  EXPECT_DEATH([] { LIBC_NAMESPACE::nanl(nullptr); }, WITH_SIGNAL(SIGSEGV));
+  EXPECT_DEATH([] { LIBC_NAMESPACE::nanl(nullptr); }, WITH_SIGNAL(SIGILL));
 }
 #endif // LIBC_HAS_ADDRESS_SANITIZER

@nickdesaulniers nickdesaulniers requested a review from lntue January 21, 2025 21:53
@nickdesaulniers nickdesaulniers merged commit 8e79ade into llvm:main Jan 22, 2025
11 checks passed
@nickdesaulniers nickdesaulniers deleted the trap_null branch January 22, 2025 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants