[compiler-rt] [sanitizer_common] AND signals in BlockSignals instead of deleting (PR #113443)

[Thurston Dang via llvm-commits]

https://github.com/thurstond updated https://github.com/llvm/llvm-project/pull/113443

>From 0672505f025c23c22f1aea75c69b557216d2c9a2 Mon Sep 17 00:00:00 2001
From: Thurston Dang <thurston at google.com>
Date: Wed, 23 Oct 2024 11:04:27 +0000
Subject: [PATCH 1/5] [sanitizer_common] AND signals in BlockSignals instead of
 deleting

My earlier patch https://github.com/llvm/llvm-project/pull/98200 caused a regression because it unconditionally unblocked synchronous signals, even if the user program had deliberately blocked them. This patch fixes the issue by checking the current signal mask, as suggested by Vitaly.

Fixes #113385
---
 .../lib/sanitizer_common/sanitizer_linux.cpp  | 36 ++++++++++++-------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
index 33107eb0b42993..1f74abfb39b31e 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
@@ -164,33 +164,45 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) {
   CHECK_EQ(0, internal_sigprocmask(SIG_SETMASK, set, oldset));
 }
 
+// Deletes the specified signal from newset, if it is not present in oldset
+// Equivalently: newset[signum] = newset[signum] & oldset[signum]
+void internal_sigandset_individual(__sanitizer_sigset_t *newset,
+                                   __sanitizer_sigset_t *oldset, int signum) {
+  if (!internal_sigismember(oldset, signum))
+    internal_sigdelset(newset, signum);
+}
+
 // Block asynchronous signals
 void BlockSignals(__sanitizer_sigset_t *oldset) {
-  __sanitizer_sigset_t set;
-  internal_sigfillset(&set);
+  __sanitizer_sigset_t currentset;
+  SetSigProcMask(NULL, &currentset);
+
+  __sanitizer_sigset_t newset;
+  internal_sigfillset(&newset);
 #  if SANITIZER_LINUX && !SANITIZER_ANDROID
   // Glibc uses SIGSETXID signal during setuid call. If this signal is blocked
   // on any thread, setuid call hangs.
   // See test/sanitizer_common/TestCases/Linux/setuid.c.
-  internal_sigdelset(&set, 33);
+  internal_sigdelset(&newset, 33);
 #  endif
 #  if SANITIZER_LINUX
   // Seccomp-BPF-sandboxed processes rely on SIGSYS to handle trapped syscalls.
   // If this signal is blocked, such calls cannot be handled and the process may
   // hang.
-  internal_sigdelset(&set, 31);
+  internal_sigdelset(&newset, 31);
 
   // Don't block synchronous signals
-  internal_sigdelset(&set, SIGSEGV);
-  internal_sigdelset(&set, SIGBUS);
-  internal_sigdelset(&set, SIGILL);
-  internal_sigdelset(&set, SIGTRAP);
-  internal_sigdelset(&set, SIGABRT);
-  internal_sigdelset(&set, SIGFPE);
-  internal_sigdelset(&set, SIGPIPE);
+  // but also don't unblock signals that the user had deliberately blocked.
+  internal_sigandset_individual(&newset, &currentset, SIGSEGV);
+  internal_sigandset_individual(&newset, &currentset, SIGBUS);
+  internal_sigandset_individual(&newset, &currentset, SIGILL);
+  internal_sigandset_individual(&newset, &currentset, SIGTRAP);
+  internal_sigandset_individual(&newset, &currentset, SIGABRT);
+  internal_sigandset_individual(&newset, &currentset, SIGFPE);
+  internal_sigandset_individual(&newset, &currentset, SIGPIPE);
 #  endif
 
-  SetSigProcMask(&set, oldset);
+  SetSigProcMask(&newset, oldset);
 }
 
 ScopedBlockSignals::ScopedBlockSignals(__sanitizer_sigset_t *copy) {

>From d510d43f8b8c416c6ed851d930ce83c121252457 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at gmail.com>
Date: Fri, 25 Oct 2024 13:13:52 -0700
Subject: [PATCH 2/5] Rename and use for all signals

---
 .../lib/sanitizer_common/sanitizer_linux.cpp  | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
index 1f74abfb39b31e..111591d6f63c60 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
@@ -166,8 +166,8 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) {
 
 // Deletes the specified signal from newset, if it is not present in oldset
 // Equivalently: newset[signum] = newset[signum] & oldset[signum]
-void internal_sigandset_individual(__sanitizer_sigset_t *newset,
-                                   __sanitizer_sigset_t *oldset, int signum) {
+static void KeepUnblocked(__sanitizer_sigset_t &newset,
+                          __sanitizer_sigset_t &oldset, int signum) {
   if (!internal_sigismember(oldset, signum))
     internal_sigdelset(newset, signum);
 }
@@ -183,23 +183,23 @@ void BlockSignals(__sanitizer_sigset_t *oldset) {
   // Glibc uses SIGSETXID signal during setuid call. If this signal is blocked
   // on any thread, setuid call hangs.
   // See test/sanitizer_common/TestCases/Linux/setuid.c.
-  internal_sigdelset(&newset, 33);
+  KeepUnblocked(&newset, &currentset, 33);
 #  endif
 #  if SANITIZER_LINUX
   // Seccomp-BPF-sandboxed processes rely on SIGSYS to handle trapped syscalls.
   // If this signal is blocked, such calls cannot be handled and the process may
   // hang.
-  internal_sigdelset(&newset, 31);
+  KeepUnblocked(&newset, &currentset, 31);
 
   // Don't block synchronous signals
   // but also don't unblock signals that the user had deliberately blocked.
-  internal_sigandset_individual(&newset, &currentset, SIGSEGV);
-  internal_sigandset_individual(&newset, &currentset, SIGBUS);
-  internal_sigandset_individual(&newset, &currentset, SIGILL);
-  internal_sigandset_individual(&newset, &currentset, SIGTRAP);
-  internal_sigandset_individual(&newset, &currentset, SIGABRT);
-  internal_sigandset_individual(&newset, &currentset, SIGFPE);
-  internal_sigandset_individual(&newset, &currentset, SIGPIPE);
+  KeepUnblocked(&newset, &currentset, SIGSEGV);
+  KeepUnblocked(&newset, &currentset, SIGBUS);
+  KeepUnblocked(&newset, &currentset, SIGILL);
+  KeepUnblocked(&newset, &currentset, SIGTRAP);
+  KeepUnblocked(&newset, &currentset, SIGABRT);
+  KeepUnblocked(&newset, &currentset, SIGFPE);
+  KeepUnblocked(&newset, &currentset, SIGPIPE);
 #  endif
 
   SetSigProcMask(&newset, oldset);

>From e8f090e5eb49fb6eb7e0c47d24ead4567d29e48e Mon Sep 17 00:00:00 2001
From: Thurston Dang <thurston at google.com>
Date: Tue, 29 Oct 2024 21:13:29 +0000
Subject: [PATCH 3/5] Fix up &

---
 .../lib/sanitizer_common/sanitizer_linux.cpp  | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
index 111591d6f63c60..82bc21d341a8be 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
@@ -168,8 +168,8 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) {
 // Equivalently: newset[signum] = newset[signum] & oldset[signum]
 static void KeepUnblocked(__sanitizer_sigset_t &newset,
                           __sanitizer_sigset_t &oldset, int signum) {
-  if (!internal_sigismember(oldset, signum))
-    internal_sigdelset(newset, signum);
+  if (!internal_sigismember(&oldset, signum))
+    internal_sigdelset(&newset, signum);
 }
 
 // Block asynchronous signals
@@ -183,23 +183,23 @@ void BlockSignals(__sanitizer_sigset_t *oldset) {
   // Glibc uses SIGSETXID signal during setuid call. If this signal is blocked
   // on any thread, setuid call hangs.
   // See test/sanitizer_common/TestCases/Linux/setuid.c.
-  KeepUnblocked(&newset, &currentset, 33);
+  KeepUnblocked(newset, currentset, 33);
 #  endif
 #  if SANITIZER_LINUX
   // Seccomp-BPF-sandboxed processes rely on SIGSYS to handle trapped syscalls.
   // If this signal is blocked, such calls cannot be handled and the process may
   // hang.
-  KeepUnblocked(&newset, &currentset, 31);
+  KeepUnblocked(newset, currentset, 31);
 
   // Don't block synchronous signals
   // but also don't unblock signals that the user had deliberately blocked.
-  KeepUnblocked(&newset, &currentset, SIGSEGV);
-  KeepUnblocked(&newset, &currentset, SIGBUS);
-  KeepUnblocked(&newset, &currentset, SIGILL);
-  KeepUnblocked(&newset, &currentset, SIGTRAP);
-  KeepUnblocked(&newset, &currentset, SIGABRT);
-  KeepUnblocked(&newset, &currentset, SIGFPE);
-  KeepUnblocked(&newset, &currentset, SIGPIPE);
+  KeepUnblocked(newset, currentset, SIGSEGV);
+  KeepUnblocked(newset, currentset, SIGBUS);
+  KeepUnblocked(newset, currentset, SIGILL);
+  KeepUnblocked(newset, currentset, SIGTRAP);
+  KeepUnblocked(newset, currentset, SIGABRT);
+  KeepUnblocked(newset, currentset, SIGFPE);
+  KeepUnblocked(newset, currentset, SIGPIPE);
 #  endif
 
   SetSigProcMask(&newset, oldset);

>From d46231d6936ccfcd8a5fd61affdd414c60941361 Mon Sep 17 00:00:00 2001
From: Thurston Dang <thurston at google.com>
Date: Tue, 29 Oct 2024 21:14:34 +0000
Subject: [PATCH 4/5] Add tests for BlockSignals

---
 .../lib/sanitizer_common/tests/CMakeLists.txt |  1 +
 .../tests/sanitizer_block_signals.cpp         | 87 +++++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp

diff --git a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
index 2b4c15125263a9..fef8bb772e0e0d 100644
--- a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
+++ b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt
@@ -15,6 +15,7 @@ set(SANITIZER_UNITTESTS
   sanitizer_array_ref_test.cpp
   sanitizer_atomic_test.cpp
   sanitizer_bitvector_test.cpp
+  sanitizer_block_signals.cpp
   sanitizer_bvgraph_test.cpp
   sanitizer_chained_origin_depot_test.cpp
   sanitizer_common_test.cpp
diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp
new file mode 100644
index 00000000000000..391b122ba49737
--- /dev/null
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp
@@ -0,0 +1,87 @@
+//===-- sanitizer_block_signals.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
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of sanitizer_common unit tests.
+//
+//===----------------------------------------------------------------------===//
+#include "gtest/gtest.h"
+#include "sanitizer_common/sanitizer_linux.h"
+#include <signal.h>
+#include <stdio.h>
+
+namespace __sanitizer {
+
+#if SANITIZER_LINUX
+volatile int received_sig = -1;
+
+void signal_handler (int signum) {
+    received_sig = signum;
+}
+
+TEST(SanitizerCommon, BlockSignals) {
+    // No signals blocked
+    {
+        signal (SIGUSR1, signal_handler);
+        raise (SIGUSR1);
+        while (received_sig == -1)
+            sleep(1);
+        EXPECT_EQ(received_sig, SIGUSR1);
+
+        received_sig = -1;
+        signal (SIGPIPE, signal_handler);
+        raise (SIGPIPE);
+        while (received_sig == -1)
+            sleep(1);
+        EXPECT_EQ(received_sig, SIGPIPE);
+    }
+
+    // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE
+    {
+        __sanitizer_sigset_t sigset = {};
+        ScopedBlockSignals block(&sigset);
+
+        received_sig = -1;
+        signal (SIGUSR1, signal_handler);
+        raise (SIGUSR1);
+        sleep(1);
+        EXPECT_EQ(received_sig, -1);
+
+        received_sig = -1;
+        signal (SIGPIPE, signal_handler);
+        raise (SIGPIPE);
+        while (received_sig == -1)
+            sleep(1);
+        EXPECT_EQ(received_sig, SIGPIPE);
+    }
+    while (received_sig == -1)
+        sleep(1);
+    EXPECT_EQ(received_sig, SIGUSR1);
+
+    // Manually block SIGPIPE; ScopedBlockSignals should not unblock this
+    sigset_t block_sigset;
+    sigemptyset(&block_sigset);
+    sigaddset(&block_sigset, SIGPIPE);
+    sigprocmask(SIG_BLOCK, &block_sigset, NULL);
+    {
+        __sanitizer_sigset_t sigset = {};
+        ScopedBlockSignals block(&sigset);
+
+        received_sig = -1;
+        signal (SIGPIPE, signal_handler);
+        raise (SIGPIPE);
+        sleep(1);
+        EXPECT_EQ(received_sig, -1);
+    }
+    sigprocmask(SIG_UNBLOCK, &block_sigset, NULL);
+    while (received_sig == -1)
+        sleep(1);
+    EXPECT_EQ(received_sig, SIGPIPE);
+}
+#endif // SANITIZER_LINUX
+
+}  // namespace __sanitizer

>From 9ad1184442cfe6258d726f9024e6bac4612a58a1 Mon Sep 17 00:00:00 2001
From: Thurston Dang <thurston at google.com>
Date: Tue, 29 Oct 2024 21:32:25 +0000
Subject: [PATCH 5/5] clang-format

---
 .../tests/sanitizer_block_signals.cpp         | 108 +++++++++---------
 1 file changed, 51 insertions(+), 57 deletions(-)

diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp
index 391b122ba49737..e5b98fc3d73fc3 100644
--- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp
+++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp
@@ -9,79 +9,73 @@
 // This file is a part of sanitizer_common unit tests.
 //
 //===----------------------------------------------------------------------===//
-#include "gtest/gtest.h"
-#include "sanitizer_common/sanitizer_linux.h"
 #include <signal.h>
 #include <stdio.h>
 
+#include "gtest/gtest.h"
+#include "sanitizer_common/sanitizer_linux.h"
+
 namespace __sanitizer {
 
 #if SANITIZER_LINUX
 volatile int received_sig = -1;
 
-void signal_handler (int signum) {
-    received_sig = signum;
-}
+void signal_handler(int signum) { received_sig = signum; }
 
 TEST(SanitizerCommon, BlockSignals) {
-    // No signals blocked
-    {
-        signal (SIGUSR1, signal_handler);
-        raise (SIGUSR1);
-        while (received_sig == -1)
-            sleep(1);
-        EXPECT_EQ(received_sig, SIGUSR1);
+  // No signals blocked
+  {
+    signal(SIGUSR1, signal_handler);
+    raise(SIGUSR1);
+    while (received_sig == -1) sleep(1);
+    EXPECT_EQ(received_sig, SIGUSR1);
 
-        received_sig = -1;
-        signal (SIGPIPE, signal_handler);
-        raise (SIGPIPE);
-        while (received_sig == -1)
-            sleep(1);
-        EXPECT_EQ(received_sig, SIGPIPE);
-    }
+    received_sig = -1;
+    signal(SIGPIPE, signal_handler);
+    raise(SIGPIPE);
+    while (received_sig == -1) sleep(1);
+    EXPECT_EQ(received_sig, SIGPIPE);
+  }
 
-    // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE
-    {
-        __sanitizer_sigset_t sigset = {};
-        ScopedBlockSignals block(&sigset);
+  // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE
+  {
+    __sanitizer_sigset_t sigset = {};
+    ScopedBlockSignals block(&sigset);
 
-        received_sig = -1;
-        signal (SIGUSR1, signal_handler);
-        raise (SIGUSR1);
-        sleep(1);
-        EXPECT_EQ(received_sig, -1);
+    received_sig = -1;
+    signal(SIGUSR1, signal_handler);
+    raise(SIGUSR1);
+    sleep(1);
+    EXPECT_EQ(received_sig, -1);
 
-        received_sig = -1;
-        signal (SIGPIPE, signal_handler);
-        raise (SIGPIPE);
-        while (received_sig == -1)
-            sleep(1);
-        EXPECT_EQ(received_sig, SIGPIPE);
-    }
-    while (received_sig == -1)
-        sleep(1);
-    EXPECT_EQ(received_sig, SIGUSR1);
+    received_sig = -1;
+    signal(SIGPIPE, signal_handler);
+    raise(SIGPIPE);
+    while (received_sig == -1) sleep(1);
+    EXPECT_EQ(received_sig, SIGPIPE);
+  }
+  while (received_sig == -1) sleep(1);
+  EXPECT_EQ(received_sig, SIGUSR1);
 
-    // Manually block SIGPIPE; ScopedBlockSignals should not unblock this
-    sigset_t block_sigset;
-    sigemptyset(&block_sigset);
-    sigaddset(&block_sigset, SIGPIPE);
-    sigprocmask(SIG_BLOCK, &block_sigset, NULL);
-    {
-        __sanitizer_sigset_t sigset = {};
-        ScopedBlockSignals block(&sigset);
+  // Manually block SIGPIPE; ScopedBlockSignals should not unblock this
+  sigset_t block_sigset;
+  sigemptyset(&block_sigset);
+  sigaddset(&block_sigset, SIGPIPE);
+  sigprocmask(SIG_BLOCK, &block_sigset, NULL);
+  {
+    __sanitizer_sigset_t sigset = {};
+    ScopedBlockSignals block(&sigset);
 
-        received_sig = -1;
-        signal (SIGPIPE, signal_handler);
-        raise (SIGPIPE);
-        sleep(1);
-        EXPECT_EQ(received_sig, -1);
-    }
-    sigprocmask(SIG_UNBLOCK, &block_sigset, NULL);
-    while (received_sig == -1)
-        sleep(1);
-    EXPECT_EQ(received_sig, SIGPIPE);
+    received_sig = -1;
+    signal(SIGPIPE, signal_handler);
+    raise(SIGPIPE);
+    sleep(1);
+    EXPECT_EQ(received_sig, -1);
+  }
+  sigprocmask(SIG_UNBLOCK, &block_sigset, NULL);
+  while (received_sig == -1) sleep(1);
+  EXPECT_EQ(received_sig, SIGPIPE);
 }
-#endif // SANITIZER_LINUX
+#endif  // SANITIZER_LINUX
 
 }  // namespace __sanitizer