Make signalfd wrapper more robust
I realized afterwards it's only used in tests, but whatever.
Change-Id: I23f654a0119faefec6a3888dc7d7c8ba38cf1b4f
diff --git a/aos/ipc_lib/BUILD b/aos/ipc_lib/BUILD
index 908cb80..57adc6d 100644
--- a/aos/ipc_lib/BUILD
+++ b/aos/ipc_lib/BUILD
@@ -164,6 +164,19 @@
],
)
+cc_test(
+ name = "signalfd_test",
+ srcs = [
+ "signalfd_test.cc",
+ ],
+ deps = [
+ ":signalfd",
+ "//aos/testing:googletest",
+ "//aos/testing:test_logging",
+ "@com_github_google_glog//:glog",
+ ],
+)
+
cc_library(
name = "index",
srcs = ["index.cc"],
diff --git a/aos/ipc_lib/signalfd.cc b/aos/ipc_lib/signalfd.cc
index af95598..363bf4a 100644
--- a/aos/ipc_lib/signalfd.cc
+++ b/aos/ipc_lib/signalfd.cc
@@ -12,21 +12,35 @@
SignalFd::SignalFd(::std::initializer_list<unsigned int> signals) {
// Build up the mask with the provided signals.
- sigemptyset(&mask_);
+ CHECK_EQ(0, sigemptyset(&blocked_mask_));
for (int signal : signals) {
- sigaddset(&mask_, signal);
+ CHECK_EQ(0, sigaddset(&blocked_mask_, signal));
}
// Then build a signalfd. Make it nonblocking so it works well with an epoll
// loop, and have it close on exec.
- PCHECK((fd_ = signalfd(-1, &mask_, SFD_NONBLOCK | SFD_CLOEXEC)) != 0);
+ PCHECK((fd_ = signalfd(-1, &blocked_mask_, SFD_NONBLOCK | SFD_CLOEXEC)) != 0);
// Now that we have a consumer of the signal, block the signals so the
- // signalfd gets them.
- pthread_sigmask(SIG_BLOCK, &mask_, nullptr);
+ // signalfd gets them. Record which ones we actually blocked, so we can
+ // unblock just those later.
+ sigset_t old_mask;
+ CHECK_EQ(0, pthread_sigmask(SIG_BLOCK, &blocked_mask_, &old_mask));
+ for (int signal : signals) {
+ if (sigismember(&old_mask, signal)) {
+ CHECK_EQ(0, sigdelset(&blocked_mask_, signal));
+ }
+ }
}
SignalFd::~SignalFd() {
- // Unwind the constructor. Unblock the signals and close the fd.
- pthread_sigmask(SIG_UNBLOCK, &mask_, nullptr);
+ // Unwind the constructor. Unblock the signals and close the fd. Verify nobody
+ // else unblocked the signals we're supposed to unblock in the meantime.
+ sigset_t old_mask;
+ CHECK_EQ(0, pthread_sigmask(SIG_UNBLOCK, &blocked_mask_, &old_mask));
+ sigset_t unblocked_mask;
+ CHECK_EQ(0, sigandset(&unblocked_mask, &blocked_mask_, &old_mask));
+ if (memcmp(&unblocked_mask, &blocked_mask_, sizeof(unblocked_mask)) != 0) {
+ LOG(FATAL) << "Some other code unblocked one or more of our signals";
+ }
PCHECK(close(fd_) == 0);
}
@@ -39,6 +53,8 @@
// by setting the signal number to 0.
if (ret != static_cast<int>(sizeof(signalfd_siginfo))) {
result.ssi_signo = 0;
+ } else {
+ CHECK_NE(0u, result.ssi_signo);
}
return result;
}
diff --git a/aos/ipc_lib/signalfd.h b/aos/ipc_lib/signalfd.h
index 7d2021a..b3a9063 100644
--- a/aos/ipc_lib/signalfd.h
+++ b/aos/ipc_lib/signalfd.h
@@ -29,7 +29,8 @@
private:
int fd_ = -1;
- sigset_t mask_;
+ // The signals we blocked in the constructor.
+ sigset_t blocked_mask_;
};
} // namespace ipc_lib
diff --git a/aos/ipc_lib/signalfd_test.cc b/aos/ipc_lib/signalfd_test.cc
new file mode 100644
index 0000000..b4fe74b
--- /dev/null
+++ b/aos/ipc_lib/signalfd_test.cc
@@ -0,0 +1,76 @@
+#include "aos/ipc_lib/signalfd.h"
+
+#include "gtest/gtest.h"
+#include "glog/logging.h"
+#include "aos/testing/test_logging.h"
+
+namespace aos {
+namespace ipc_lib {
+namespace testing {
+
+// Tests in this file use separate threads to isolate all manipulation of signal
+// masks between test cases.
+
+// Verify that SignalFd will leave signals unblocked if we ask it to.
+TEST(SignalFdTest, LeaveSignalBlocked) {
+ ::aos::testing::EnableTestLogging();
+ std::thread thread([]() {
+ {
+ sigset_t test_mask;
+ CHECK_EQ(0, sigemptyset(&test_mask));
+ CHECK_EQ(0, sigaddset(&test_mask, SIGUSR1));
+ PCHECK(sigprocmask(SIG_BLOCK, &test_mask, nullptr) == 0);
+ }
+ SignalFd({SIGUSR1});
+ {
+ sigset_t blocked_now;
+ PCHECK(sigprocmask(SIG_BLOCK, nullptr, &blocked_now) == 0);
+ ASSERT_TRUE(sigismember(&blocked_now, SIGUSR1));
+ }
+ });
+ thread.join();
+}
+
+// Verify that SignalFd actually blocks the requested signals, and unblocks them
+// afterwards.
+TEST(SignalFdTest, BlockSignal) {
+ ::aos::testing::EnableTestLogging();
+ std::thread thread([]() {
+ {
+ sigset_t blocked_now;
+ PCHECK(sigprocmask(SIG_BLOCK, nullptr, &blocked_now) == 0);
+ ASSERT_FALSE(sigismember(&blocked_now, SIGUSR1));
+ }
+ {
+ SignalFd signalfd({SIGUSR1});
+ sigset_t blocked_now;
+ PCHECK(sigprocmask(SIG_BLOCK, nullptr, &blocked_now) == 0);
+ ASSERT_TRUE(sigismember(&blocked_now, SIGUSR1));
+ }
+ {
+ sigset_t blocked_now;
+ PCHECK(sigprocmask(SIG_BLOCK, nullptr, &blocked_now) == 0);
+ ASSERT_FALSE(sigismember(&blocked_now, SIGUSR1));
+ }
+ });
+ thread.join();
+}
+
+// Verify that SignalFd responds correctly when some other code unblocks one of
+// its signals.
+TEST(SignalFdDeathTest, ExternalUnblockSignal) {
+ ::aos::testing::EnableTestLogging();
+ EXPECT_DEATH(
+ {
+ SignalFd signalfd({SIGUSR1});
+ sigset_t test_mask;
+ CHECK_EQ(0, sigemptyset(&test_mask));
+ CHECK_EQ(0, sigaddset(&test_mask, SIGUSR1));
+ PCHECK(sigprocmask(SIG_UNBLOCK, &test_mask, nullptr) == 0);
+ },
+ "Some other code unblocked one or more of our signals");
+}
+
+} // namespace testing
+} // namespace ipc_lib
+} // namespace aos