Merge pull request #218 from jerch/signals_fork

block signals during forking

src/unix/pty.cc

@@ -28,6 +28,7 @@
 #include <sys/ioctl.h>
 #include <sys/wait.h>
 #include <fcntl.h>
+#include <signal.h>
 
 /* forkpty */
 /* http://www.gnu.org/software/gnulib/manual/html_node/forkpty.html */
@@ -71,6 +72,11 @@ extern char **environ;
 #include <libproc.h>
 #endif
 
+/* NSIG - macro for highest signal + 1, should be defined */
+#ifndef NSIG
+#define NSIG 32
+#endif
+
 /**
  * Structs
  */
@@ -143,9 +149,6 @@ NAN_METHOD(PtyFork) {
         "Usage: pty.fork(file, args, env, cwd, cols, rows, uid, gid, utf8, onexit)");
   }
 
-  // Make sure the process still listens to SIGINT
-  signal(SIGINT, SIG_DFL);
-
   // file
   Nan::Utf8String file(info[0]);
 
@@ -228,8 +231,31 @@ NAN_METHOD(PtyFork) {
 
   // fork the pty
   int master = -1;
+
+  sigset_t newmask, oldmask;
+  struct sigaction sig_action;
+
+  // temporarily block all signals
+  // this is needed due to a race condition in openpty
+  // and to avoid running signal handlers in the child
+  // before exec* happened
+  sigfillset(&newmask);
+  pthread_sigmask(SIG_SETMASK, &newmask, &oldmask);
+
   pid_t pid = pty_forkpty(&master, nullptr, term, &winp);
 
+  if (!pid) {
+    // remove all signal handler from child
+    sig_action.sa_handler = SIG_DFL;
+    sig_action.sa_flags = 0;
+    sigemptyset(&sig_action.sa_mask);
+    for (int i = 0 ; i < NSIG ; i++) {    // NSIG is a macro for all signals + 1
+      sigaction(i, &sig_action, NULL);
+    }
+  }
+  // reenable signals
+  pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+
   if (pid) {
     for (i = 0; i < argl; i++) free(argv[i]);
     delete[] argv;
@@ -659,13 +685,12 @@ pty_forkpty(int *amaster,
   pid_t pid = fork();
 
   switch (pid) {
-    case -1:
+    case -1:  // error in fork, we are still in parent
       close(master);
       close(slave);
       return -1;
-    case 0:
+    case 0:  // we are in the child process
       close(master);
-
       setsid();
 
 #if defined(TIOCSCTTY)
@@ -682,7 +707,7 @@ pty_forkpty(int *amaster,
       if (slave > 2) close(slave);
 
       return 0;
-    default:
+    default:  // we are in the parent process
       close(slave);
       return pid;
   }
@@ -693,6 +718,7 @@ pty_forkpty(int *amaster,
 #endif
 }
 
+
 /**
  * Init
  */

src/unixTerminal.test.ts

@@ -5,6 +5,7 @@
 
 import { UnixTerminal } from './unixTerminal';
 import * as assert from 'assert';
+import * as cp from 'child_process';
 import * as path from 'path';
 import { pollUntil } from './testUtils.test';
 
@@ -104,5 +105,140 @@ if (process.platform !== 'win32') {
         term.master.write('master\n');
       });
     });
+    describe('signals in parent and child', () => {
+      it('SIGINT - custom in parent and child', done => {
+        // this test is cumbersome - we have to run it in a sub process to
+        // see behavior of SIGINT handlers
+        const data = `
+        var pty = require('./lib/index');
+        process.on('SIGINT', () => console.log('SIGINT in parent'));
+        var ptyProcess = pty.spawn('node', ['-e', 'process.on("SIGINT", ()=>console.log("SIGINT in child"));setTimeout(() => null, 300);'], {
+          name: 'xterm-color',
+          cols: 80,
+          rows: 30,
+          cwd: process.env.HOME,
+          env: process.env
+        });
+        ptyProcess.on('data', function (data) {
+          console.log(data);
+        });
+        setTimeout(() => null, 500);
+        console.log('ready', ptyProcess.pid);
+        `;
+        let buffer: string[] = [];
+        const p = cp.spawn('node', ['-e', data]);
+        let sub = '';
+        p.stdout.on('data', (data) => {
+          if (!data.toString().indexOf('ready')) {
+            sub = data.toString().split(' ')[1].slice(0, -1);
+            setTimeout(() => {
+              process.kill(parseInt(sub), 'SIGINT');  // SIGINT to child
+              p.kill('SIGINT');                       // SIGINT to parent
+            }, 200);
+          } else {
+            buffer.push(data.toString().replace(/^\s+|\s+$/g, ''));
+          }
+        });
+        p.on('close', () => {
+          // handlers in parent and child should have been triggered
+          assert.equal(buffer.indexOf('SIGINT in child') !== -1, true);
+          assert.equal(buffer.indexOf('SIGINT in parent') !== -1, true);
+          done();
+        });
+      });
+      it('SIGINT - custom in parent, default in child', done => {
+        // this tests the original idea of the signal(...) change in pty.cc:
+        // to make sure the SIGINT handler of a pty child is reset to default
+        // and does not interfere with the handler in the parent
+        const data = `
+        var pty = require('./lib/index');
+        process.on('SIGINT', () => console.log('SIGINT in parent'));
+        var ptyProcess = pty.spawn('node', ['-e', 'setTimeout(() => console.log("should not be printed"), 300);'], {
+          name: 'xterm-color',
+          cols: 80,
+          rows: 30,
+          cwd: process.env.HOME,
+          env: process.env
+        });
+        ptyProcess.on('data', function (data) {
+          console.log(data);
+        });
+        setTimeout(() => null, 500);
+        console.log('ready', ptyProcess.pid);
+        `;
+        let buffer: string[] = [];
+        const p = cp.spawn('node', ['-e', data]);
+        let sub = '';
+        p.stdout.on('data', (data) => {
+          if (!data.toString().indexOf('ready')) {
+            sub = data.toString().split(' ')[1].slice(0, -1);
+            setTimeout(() => {
+              process.kill(parseInt(sub), 'SIGINT');  // SIGINT to child
+              p.kill('SIGINT');                       // SIGINT to parent
+            }, 200);
+          } else {
+            buffer.push(data.toString().replace(/^\s+|\s+$/g, ''));
+          }
+        });
+        p.on('close', () => {
+          // handlers in parent and child should have been triggered
+          assert.equal(buffer.indexOf('should not be printed') !== -1, false);
+          assert.equal(buffer.indexOf('SIGINT in parent') !== -1, true);
+          done();
+        });
+      });
+      it('SIGHUP default (child only)', done => {
+        const term = new UnixTerminal('node', [ '-e', `
+        console.log('ready');
+        setTimeout(()=>console.log('timeout'), 200);`
+        ]);
+        let buffer = '';
+        term.on('data', (data) => {
+          if (data === 'ready\r\n') {
+            term.kill();
+          } else {
+            buffer += data;
+          }
+        });
+        term.on('exit', () => {
+          // no timeout in buffer
+          assert.equal(buffer, '');
+          done();
+        });
+      });
+      it('SIGUSR1 - custom in parent and child', done => {
+        let pHandlerCalled = 0;
+        const handleSigUsr = function(h: any): any {
+          return function(): void {
+            pHandlerCalled += 1;
+            process.removeListener('SIGUSR1', h);
+          };
+        };
+        process.on('SIGUSR1', handleSigUsr(handleSigUsr));
+
+        const term = new UnixTerminal('node', [ '-e', `
+        process.on('SIGUSR1', () => {
+          console.log('SIGUSR1 in child');
+        });
+        console.log('ready');
+        setTimeout(()=>null, 200);`
+        ]);
+        let buffer = '';
+        term.on('data', (data) => {
+          if (data === 'ready\r\n') {
+            process.kill(process.pid, 'SIGUSR1');
+            term.kill('SIGUSR1');
+          } else {
+            buffer += data;
+          }
+        });
+        term.on('exit', () => {
+          // should have called both handlers and only once
+          assert.equal(pHandlerCalled, 1);
+          assert.equal(buffer, 'SIGUSR1 in child\r\n');
+          done();
+        });
+      });
+    });
   });
 }