signal handler fixes/improvements

5 files changed, 131 insertions, 21 deletions

diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index a3d3dfcd4..03ad25f75 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -551,6 +551,7 @@ void disable_file_or_dir(const char *fname);
 void disable_file_path(const char *path, const char *file);
 int safe_fd(const char *path, int flags);
 int invalid_sandbox(const pid_t pid);
+int has_handler(pid_t pid, int signal);
 void enter_network_namespace(pid_t pid);
 
 // Get info regarding the last kernel mount operation from /proc/self/mountinfo
diff --git a/src/firejail/join.c b/src/firejail/join.c
index 4259644f7..d05a4a465 100644
--- a/src/firejail/join.c
+++ b/src/firejail/join.c
@@ -41,7 +41,15 @@ static void signal_handler(int sig){
         exit(sig);
 }
 
-
+static void install_handler(void) {
+        struct sigaction sga;
+
+        // handle SIGTERM
+        sigemptyset(&sga.sa_mask);
+        sga.sa_handler = signal_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGTERM, &sga, NULL);
+}
 
 static void extract_x11_display(pid_t pid) {
         char *fname;
@@ -465,8 +473,19 @@ void join(pid_t pid, int argc, char **argv, int index) {
         }
 
         int status = 0;
+        //*****************************
+        // following code is signal-safe
+
+        install_handler();
+
         // wait for the child to finish
         waitpid(child, &status, 0);
+
+        // restore default signal action
+        signal(SIGTERM, SIG_DFL);
+
+        // end of signal-safe code
+        //*****************************
         flush_stdin();
 
         if (WIFEXITED(status)) {
diff --git a/src/firejail/main.c b/src/firejail/main.c
index faa4972e2..61f507f36 100644
--- a/src/firejail/main.c
+++ b/src/firejail/main.c
@@ -157,12 +157,35 @@ static void myexit(int rv) {
         exit(rv);
 }
 
-static void my_handler(int s){
-        EUID_ROOT();
+static void my_handler(int s) {
         fmessage("\nParent received signal %d, shutting down the child process...\n", s);
         logsignal(s);
-        kill(child, SIGTERM);
-        myexit(1);
+        if (waitpid(child, NULL, WNOHANG) == 0) {
+                if (has_handler(child, s)) // signals are not delivered if there is no handler yet
+                        kill(child, s);
+                else
+                        kill(child, SIGKILL);
+                waitpid(child, NULL, 0);
+        }
+        myexit(s);
+}
+
+static void install_handler(void) {
+        struct sigaction sga;
+
+        // block SIGTERM while handling SIGINT
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGTERM);
+        sga.sa_handler = my_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGINT, &sga, NULL);
+
+        // block SIGINT while handling SIGTERM
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGINT);
+        sga.sa_handler = my_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGTERM, &sga, NULL);
 }
 
 // return 1 if error, 0 if a valid pid was found
@@ -2601,16 +2624,25 @@ int main(int argc, char **argv) {
                 flock(lockfd_network, LOCK_UN);
                 close(lockfd_network);
         }
+        EUID_USER();
+
+        int status = 0;
+        //*****************************
+        // following code is signal-safe
 
         // handle CTRL-C in parent
-        signal (SIGINT, my_handler);
-        signal (SIGTERM, my_handler);
+        install_handler();
 
         // wait for the child to finish
-        EUID_USER();
-        int status = 0;
         waitpid(child, &status, 0);
 
+        // restore default signal actions
+        signal(SIGTERM, SIG_DFL);
+        signal(SIGINT, SIG_DFL);
+
+        // end of signal-safe code
+        //*****************************
+
         // free globals
         if (cfg.profile) {
                 ProfileEntry *prf = cfg.profile;
diff --git a/src/firejail/sandbox.c b/src/firejail/sandbox.c
index 475779f47..ba9a36250 100644
--- a/src/firejail/sandbox.c
+++ b/src/firejail/sandbox.c
@@ -53,6 +53,7 @@ static int force_nonewprivs = 0;
 
 static int monitored_pid = 0;
 static void sandbox_handler(int sig){
+        usleep(10000); // don't race to print a message
         fmessage("\nChild received signal %d, shutting down the sandbox...\n", sig);
 
         // broadcast sigterm to all processes in the group
@@ -81,10 +82,8 @@ static void sandbox_handler(int sig){
                         monsec--;
                 }
                 free(monfile);
-
         }
 
-
         // broadcast a SIGKILL
         kill(-1, SIGKILL);
         flush_stdin();
@@ -92,6 +91,23 @@ static void sandbox_handler(int sig){
         exit(sig);
 }
 
+static void install_handler(void) {
+        struct sigaction sga;
+
+        // block SIGTERM while handling SIGINT
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGTERM);
+        sga.sa_handler = sandbox_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGINT, &sga, NULL);
+
+        // block SIGINT while handling SIGTERM
+        sigemptyset(&sga.sa_mask);
+        sigaddset(&sga.sa_mask, SIGINT);
+        sga.sa_handler = sandbox_handler;
+        sga.sa_flags = 0;
+        sigaction(SIGTERM, &sga, NULL);
+}
 
 static void set_caps(void) {
         if (arg_caps_drop_all)
@@ -123,7 +139,6 @@ static void save_nogroups(void) {
                 fprintf(stderr, "Error: cannot save nogroups state\n");
                 exit(1);
         }
-
 }
 
 static void save_nonewprivs(void) {
@@ -235,9 +250,17 @@ static void chk_chroot(void) {
 }
 
 static int monitor_application(pid_t app_pid) {
+        EUID_ASSERT();
         monitored_pid = app_pid;
-        signal (SIGTERM, sandbox_handler);
-        EUID_USER();
+
+        // block signals and install handler
+        sigset_t oldmask, newmask;
+        sigemptyset(&oldmask);
+        sigemptyset(&newmask);
+        sigaddset(&newmask, SIGTERM);
+        sigaddset(&newmask, SIGINT);
+        sigprocmask(SIG_BLOCK, &newmask, &oldmask);
+        install_handler();
 
         // handle --timeout
         int options = 0;;
@@ -260,16 +283,25 @@ static int monitor_application(pid_t app_pid) {
 
                 pid_t rv;
                 do {
+                        // handle signals asynchronously
+                        sigprocmask(SIG_SETMASK, &oldmask, NULL);
+
                         rv = waitpid(-1, &status, options);
-                        if (rv == -1)
+
+                        // block signals again
+                        sigprocmask(SIG_BLOCK, &newmask, NULL);
+
+                        if (rv == -1) { // we can get here if we have processes joining the sandbox (ECHILD)
+                                sleep(1);
                                 break;
+                        }
 
                         // handle --timeout
                         if (options) {
                                 if (--timeout == 0)  {
                                         kill(-1, SIGTERM);
-                                        flush_stdin();
                                         sleep(1);
+                                        flush_stdin();
                                         _exit(1);
                                 }
                                 else
@@ -279,11 +311,6 @@ static int monitor_application(pid_t app_pid) {
                 while(rv != monitored_pid);
                 if (arg_debug)
                         printf("Sandbox monitor: waitpid %d retval %d status %d\n", monitored_pid, rv, status);
-                if (rv == -1) { // we can get here if we have processes joining the sandbox (ECHILD)
-                        if (arg_debug)
-                                perror("waitpid");
-                        sleep(1);
-                }
 
                 DIR *dir;
                 if (!(dir = opendir("/proc"))) {
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 8c474f966..6b30ab5fe 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -1264,6 +1264,37 @@ int invalid_sandbox(const pid_t pid) {
         return 0;
 }
 
+int has_handler(pid_t pid, int signal) {
+        if (signal > 0) {
+                char *fname;
+                if (asprintf(&fname, "/proc/%d/status", pid) == -1)
+                        errExit("asprintf");
+                EUID_ROOT();
+                FILE *fp = fopen(fname, "re");
+                EUID_USER();
+                free(fname);
+                if (fp) {
+                        char buf[BUFLEN];
+                        while (fgets(buf, BUFLEN, fp)) {
+                                if (strncmp(buf, "SigCgt:", 7) == 0) {
+                                        char *ptr = buf + 7;
+                                        unsigned long long val;
+                                        if (sscanf(ptr, "%llx", &val) != 1) {
+                                                fprintf(stderr, "Error: cannot read /proc file\n");
+                                                exit(1);
+                                        }
+                                        val >>= (signal - 1);
+                                        val &= 1;
+                                        fclose(fp);
+                                        return val;  // 1 if process has a handler for the signal, else 0
+                                }
+                        }
+                        fclose(fp);
+                }
+        }
+        return 0;
+}
+
 void enter_network_namespace(pid_t pid) {
         // in case the pid is that of a firejail process, use the pid of the first child process
         pid_t child = switch_to_child(pid);