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 <jwkozac...@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

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/osv-dev/20210528131639.1159251-1-jwkozaczuk%40gmail.com.

Reply via email to