[PATCH] sigaction: handle null sa_sigaction as SIG_DFL

[Waldemar Kozaczuk]

As the comment of the issue #1047 - https://github.com/cloudius-systems/osv/issues/1047#issuecomment-846535939
- explains, Linux treats null value of the sa_sigaction parameter of
the sigaction() function as SIG_DFL even if SA_SIGINFO is set per sa_flags
parameter. This behavior does not seem to be standard as this stack
overflow - https://stackoverflow.com/questions/24079875/what-does-it-mean-if-sa-sigaction-is-set-to-null-when-calling-sigaction
- explains and probably stems from the fact that both sa_sigaction and
sa_handler are fields of the same union.

The above means, that if sa_sigaction is null we should treat
it the same same as if sa_handler == SIG_DFL and invoke default
signal handler if any when corresponding signal arrived. To that effect
this patch refines is_sig_dfl() helper function to implemented the
expected behavior.

Finally this patch adds new test - tst-sigaction.cc - that can be built
and run on both Linux and OSv to verify the same handling of null
sa_sigaction. Ideally, we would have wanted to expand existing
tst-kill.cc test, however we need a separate test to verify the
default action to terminate the app when SIGTERM is received is testable.

Fixes #1047

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
libc/signal.cc | 6 +++++-
modules/tests/Makefile | 3 ++-
tests/tst-sigaction.cc | 43 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 2 deletions(-)
create mode 100644 tests/tst-sigaction.cc

diff --git a/libc/signal.cc b/libc/signal.cc
index aaf8f62a..f346552d 100644
--- a/libc/signal.cc
+++ b/libc/signal.cc
@@ -57,7 +57,11 @@ sigset* thread_signals(sched::thread *t)
}

inline bool is_sig_dfl(const struct sigaction &sa) {
- return (!(sa.sa_flags & SA_SIGINFO) && sa.sa_handler == SIG_DFL);
+ if (sa.sa_flags & SA_SIGINFO) {
+ return sa.sa_sigaction == nullptr; // a non-standard Linux extension
+ } else {
+ return sa.sa_handler == SIG_DFL;
+ }
}

inline bool is_sig_ign(const struct sigaction &sa) {
diff --git a/modules/tests/Makefile b/modules/tests/Makefile
index bb531c25..ebe1dd72 100644
--- a/modules/tests/Makefile
+++ b/modules/tests/Makefile
@@ -111,7 +111,8 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so tst-bsd-evh.so \
tst-calloc.so tst-crypt.so tst-non-fpic.so tst-small-malloc.so \
tst-getopt.so tst-getopt-pie.so tst-non-pie.so tst-semaphore.so \
tst-elf-init.so tst-realloc.so tst-setjmp.so \
- libtls.so libtls_gold.so tst-tls.so tst-tls-gold.so tst-tls-pie.so
+ libtls.so libtls_gold.so tst-tls.so tst-tls-gold.so tst-tls-pie.so \
+ tst-sigaction.so
# libstatic-thread-variable.so tst-static-thread-variable.so \

#TODO For now let us disable these tests for aarch64 until
diff --git a/tests/tst-sigaction.cc b/tests/tst-sigaction.cc
new file mode 100644
index 00000000..850fc719
--- /dev/null
+++ b/tests/tst-sigaction.cc
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2021 Waldemar Kozaczuk
+ *
+ * This work is open source software, licensed under the terms of the
+ * BSD license as described in the LICENSE file in the top-level directory.
+ */
+// To compile on Linux, use: g++ -std=c++11 tests/tst-sigaction.cc
+#include <stdio.h>
+#include <unistd.h>
+#include <signal.h>
+#include <cassert>
+
+static int global = 0;
+void test_handler(int sig, siginfo_t *info, void *ucontext)
+{
+ printf("test_handler called, sig=%d, global=%d\n", sig, global);
+ global = 1;
+}
+
+void test_sigaction_with_handler(void (*handler)(int, siginfo_t *, void *))
+{
+ struct sigaction act = {};
+ act.sa_flags = SA_SIGINFO;
+ sigemptyset(&act.sa_mask);
+ act.sa_sigaction = handler;
+ assert(0 == sigaction(SIGTERM, &act, nullptr));
+ assert(0 == kill(getpid(), SIGTERM));
+}
+
+int main(int ac, char** av)
+{
+ test_sigaction_with_handler(test_handler);
+ for(int i = 0; i < 100; i++){
+ if(global == 1) break;
+ usleep(10000);
+ }
+ assert(global == 1);
+ printf("global now 1, test_handler called\n");
+
+ test_sigaction_with_handler(nullptr);
+ sleep(1);
+ assert(0); //We should not get here as the app and OSv should have already terminated by this time
+}
--
2.30.2

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

sigaction: handle null sa_sigaction as SIG_DFL

As the comment of the issue #1047 - https://github.com/cloudius-systems/osv/issues/1047#issuecomment-846535939
- explains, Linux treats null value of the sa_sigaction parameter of
the sigaction() function as SIG_DFL even if SA_SIGINFO is set per sa_flags
parameter. This behavior does not seem to be standard as this stack
overflow - https://stackoverflow.com/questions/24079875/what-does-it-mean-if-sa-sigaction-is-set-to-null-when-calling-sigaction
- explains and probably stems from the fact that both sa_sigaction and
sa_handler are fields of the same union.

The above means, that if sa_sigaction is null we should treat
it the same same as if sa_handler == SIG_DFL and invoke default
signal handler if any when corresponding signal arrived. To that effect
this patch refines is_sig_dfl() helper function to implemented the
expected behavior.

Finally this patch adds new test - tst-sigaction.cc - that can be built
and run on both Linux and OSv to verify the same handling of null
sa_sigaction. Ideally, we would have wanted to expand existing
tst-kill.cc test, however we need a separate test to verify the
default action to terminate the app when SIGTERM is received is testable.

Fixes #1047

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

---

diff --git a/libc/signal.cc b/libc/signal.cc

--- a/libc/signal.cc
+++ b/libc/signal.cc
@@ -57,7 +57,11 @@ sigset* thread_signals(sched::thread *t)
}

inline bool is_sig_dfl(const struct sigaction &sa) {
- return (!(sa.sa_flags & SA_SIGINFO) && sa.sa_handler == SIG_DFL);
+ if (sa.sa_flags & SA_SIGINFO) {
+ return sa.sa_sigaction == nullptr; // a non-standard Linux extension
+ } else {
+ return sa.sa_handler == SIG_DFL;
+ }
}

inline bool is_sig_ign(const struct sigaction &sa) {
diff --git a/modules/tests/Makefile b/modules/tests/Makefile

--- a/modules/tests/Makefile
+++ b/modules/tests/Makefile
@@ -111,7 +111,8 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so tst-bsd-evh.so \
tst-calloc.so tst-crypt.so tst-non-fpic.so tst-small-malloc.so \
tst-getopt.so tst-getopt-pie.so tst-non-pie.so tst-semaphore.so \
tst-elf-init.so tst-realloc.so tst-setjmp.so \
- libtls.so libtls_gold.so tst-tls.so tst-tls-gold.so tst-tls-pie.so
+ libtls.so libtls_gold.so tst-tls.so tst-tls-gold.so tst-tls-pie.so \
+ tst-sigaction.so
# libstatic-thread-variable.so tst-static-thread-variable.so \

#TODO For now let us disable these tests for aarch64 until
diff --git a/tests/tst-sigaction.cc b/tests/tst-sigaction.cc

--- a/tests/tst-sigaction.cc