From 8639f6cad297cce6f1405fce822c6d73f5392730 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Mon, 27 Aug 2018 22:10:28 +0200 Subject: [PATCH 1/6] block signals during forking --- src/unix/pty.cc | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 57121b67..b9e110eb 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -147,9 +147,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 v8::String::Utf8Value file(info[0]->ToString()); @@ -645,6 +642,10 @@ pty_openpty(int *amaster, #endif } +#ifndef NSIG +# define NSIG 32 +#endif + static pid_t pty_forkpty(int *amaster, char *name, @@ -652,6 +653,14 @@ pty_forkpty(int *amaster, const struct winsize *winp) { #if defined(__sun) int master, slave; + sigset_t newmask, oldmask; + + // 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); + sigprocmask(SIG_SETMASK, &newmask, &oldmask); int ret = pty_openpty(&master, &slave, name, termp, winp); if (ret == -1) return -1; @@ -660,13 +669,24 @@ pty_forkpty(int *amaster, pid_t pid = fork(); switch (pid) { - case -1: + case -1: // error in fork, we are still in parent + sigprocmask(SIG_SETMASK, &oldmask, NULL); close(master); close(slave); return -1; - case 0: - close(master); + case 0: // we are in the child process + // remove all signal handler from child + struct sigaction sig_action; + sig_action.sa_handler = SIG_DFL; // set default as fallback + sig_action.sa_flags = 0; + sigemptyset(&sig_action.sa_mask); + for (i = 0 ; i < NSIG ; i++) { // NSIG is a macro for all signals + 1 + sigaction(i, &sig_action, NULL); + } + // reenable signals + sigprocmask(SIG_SETMASK, &oldmask, NULL); + close(master); setsid(); #if defined(TIOCSCTTY) @@ -682,8 +702,12 @@ pty_forkpty(int *amaster, if (slave > 2) close(slave); + + return 0; - default: + default: // we are in the parent process + // reenable signals + sigprocmask(SIG_SETMASK, &oldmask, NULL); close(slave); return pid; } From 88e0e22a3a49b1e61e81529f9e5df39f7ae75f17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Mon, 27 Aug 2018 23:31:32 +0200 Subject: [PATCH 2/6] fix: inserted code at wrong location --- src/unix/pty.cc | 57 +++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index b9e110eb..99cd948f 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -71,6 +71,11 @@ extern char **environ; #include #endif +/* NSIG - macro for highest signal + 1, should be defined */ +#ifndef NSIG +#define NSIG 32 +#endif + /** * Structs */ @@ -229,8 +234,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); + sigprocmask(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; // set default as fallback + 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 + sigprocmask(SIG_SETMASK, &oldmask, NULL); + if (pid) { for (i = 0; i < argl; i++) free(argv[i]); delete[] argv; @@ -642,10 +670,6 @@ pty_openpty(int *amaster, #endif } -#ifndef NSIG -# define NSIG 32 -#endif - static pid_t pty_forkpty(int *amaster, char *name, @@ -653,14 +677,6 @@ pty_forkpty(int *amaster, const struct winsize *winp) { #if defined(__sun) int master, slave; - sigset_t newmask, oldmask; - - // 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); - sigprocmask(SIG_SETMASK, &newmask, &oldmask); int ret = pty_openpty(&master, &slave, name, termp, winp); if (ret == -1) return -1; @@ -670,22 +686,10 @@ pty_forkpty(int *amaster, switch (pid) { case -1: // error in fork, we are still in parent - sigprocmask(SIG_SETMASK, &oldmask, NULL); close(master); close(slave); return -1; case 0: // we are in the child process - // remove all signal handler from child - struct sigaction sig_action; - sig_action.sa_handler = SIG_DFL; // set default as fallback - sig_action.sa_flags = 0; - sigemptyset(&sig_action.sa_mask); - for (i = 0 ; i < NSIG ; i++) { // NSIG is a macro for all signals + 1 - sigaction(i, &sig_action, NULL); - } - // reenable signals - sigprocmask(SIG_SETMASK, &oldmask, NULL); - close(master); setsid(); @@ -702,12 +706,8 @@ pty_forkpty(int *amaster, if (slave > 2) close(slave); - - return 0; default: // we are in the parent process - // reenable signals - sigprocmask(SIG_SETMASK, &oldmask, NULL); close(slave); return pid; } @@ -718,6 +718,7 @@ pty_forkpty(int *amaster, #endif } + /** * Init */ From 83cfbbd3bd1c8702f1c8d9d9f24cce8e1dffd675 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Tue, 28 Aug 2018 01:05:15 +0200 Subject: [PATCH 3/6] use threadsafe version --- src/unix/pty.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 99cd948f..60543cde 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -28,6 +28,7 @@ #include #include #include +#include /* forkpty */ /* http://www.gnu.org/software/gnulib/manual/html_node/forkpty.html */ @@ -243,13 +244,13 @@ NAN_METHOD(PtyFork) { // and to avoid running signal handlers in the child // before exec* happened sigfillset(&newmask); - sigprocmask(SIG_SETMASK, &newmask, &oldmask); + 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; // set default as fallback + 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 @@ -257,7 +258,7 @@ NAN_METHOD(PtyFork) { } } // reenable signals - sigprocmask(SIG_SETMASK, &oldmask, NULL); + pthread_sigmask(SIG_SETMASK, &oldmask, NULL); if (pid) { for (i = 0; i < argl; i++) free(argv[i]); From 20458295e3ce6a0facd7e7834e629f4d7ce4ef09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Tue, 28 Aug 2018 15:10:43 +0200 Subject: [PATCH 4/6] test correct celivery of signals in parent and child --- src/unixTerminal.test.ts | 94 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/src/unixTerminal.test.ts b/src/unixTerminal.test.ts index d0c9b6d5..b417b203 100644 --- a/src/unixTerminal.test.ts +++ b/src/unixTerminal.test.ts @@ -103,5 +103,99 @@ if (process.platform !== 'win32') { term.master.write('master\n'); }); }); + describe('signals in parent and child - TEST SHOULD NOT WITH THIS LINE', function(): void { + it('SIGINT', function(done): void { + let pHandlerCalled = 0; + // tricky one: we have to remove all SIGINT listeners + // so mocha will not stop here due to some other listener + const listeners = process.listeners('SIGINT'); + const handleSigInt = function(h: any): any { + return function(): void { + pHandlerCalled += 1; + process.removeListener('SIGINT', h); + for (let i = 0; i < listeners.length; ++i) { + process.on('SIGINT', listeners[i]); + } + } + }; + process.removeAllListeners('SIGINT'); + process.on('SIGINT', handleSigInt(handleSigInt)); + + const term = new UnixTerminal('node', [ '-e', ` + process.on('SIGINT', () => { + console.log('SIGINT in child'); + }); + console.log('ready'); + setTimeout(()=>null, 200);` + ]); + let buffer = ''; + term.on('data', (data) => { + if (data === 'ready\r\n') { + process.kill(process.pid, 'SIGINT'); + term.kill('SIGINT'); + } else { + buffer += data; + } + }); + term.on('exit', () => { + // should have called both handlers + assert.equal(pHandlerCalled, 1); + assert.equal(buffer, 'SIGINT in child\r\n'); + done(); + }); + }); + it('SIGHUP (child only)', function(done): void { + 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', function(done): void { + 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(); + }); + }); + }); }); } From 8404f1ab9b5aa45a375de2e8bd1add5cf68a6f47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Tue, 28 Aug 2018 16:10:14 +0200 Subject: [PATCH 5/6] better test for SIGINT handlers --- src/unixTerminal.test.ts | 69 ++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/src/unixTerminal.test.ts b/src/unixTerminal.test.ts index b417b203..5a9fc759 100644 --- a/src/unixTerminal.test.ts +++ b/src/unixTerminal.test.ts @@ -103,44 +103,45 @@ if (process.platform !== 'win32') { term.master.write('master\n'); }); }); - describe('signals in parent and child - TEST SHOULD NOT WITH THIS LINE', function(): void { + describe('signals in parent and child', function(): void { it('SIGINT', function(done): void { - let pHandlerCalled = 0; - // tricky one: we have to remove all SIGINT listeners - // so mocha will not stop here due to some other listener - const listeners = process.listeners('SIGINT'); - const handleSigInt = function(h: any): any { - return function(): void { - pHandlerCalled += 1; - process.removeListener('SIGINT', h); - for (let i = 0; i < listeners.length; ++i) { - process.on('SIGINT', listeners[i]); - } - } - }; - process.removeAllListeners('SIGINT'); - process.on('SIGINT', handleSigInt(handleSigInt)); - - const term = new UnixTerminal('node', [ '-e', ` - process.on('SIGINT', () => { - console.log('SIGINT in child'); - }); - console.log('ready'); - setTimeout(()=>null, 200);` - ]); - let buffer = ''; - term.on('data', (data) => { - if (data === 'ready\r\n') { - process.kill(process.pid, 'SIGINT'); - term.kill('SIGINT'); + // 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 cp = require('child_process'); + 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 += data; + buffer.push(data.toString().replace(/^\s+|\s+$/g, '')); } }); - term.on('exit', () => { - // should have called both handlers - assert.equal(pHandlerCalled, 1); - assert.equal(buffer, 'SIGINT in child\r\n'); + 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(); }); }); From fac4ad6eb7f6ac6bc9739611ae9bc7e414956d57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Breitbart?= Date: Tue, 28 Aug 2018 16:34:24 +0200 Subject: [PATCH 6/6] test case for original signal() addition --- src/unixTerminal.test.ts | 48 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/src/unixTerminal.test.ts b/src/unixTerminal.test.ts index 5a9fc759..791549db 100644 --- a/src/unixTerminal.test.ts +++ b/src/unixTerminal.test.ts @@ -104,7 +104,7 @@ if (process.platform !== 'win32') { }); }); describe('signals in parent and child', function(): void { - it('SIGINT', function(done): void { + it('SIGINT - custom in parent and child', function(done): void { // this test is cumbersome - we have to run it in a sub process to // see behavior of SIGINT handlers const data = ` @@ -145,7 +145,49 @@ if (process.platform !== 'win32') { done(); }); }); - it('SIGHUP (child only)', function(done): void { + it('SIGINT - custom in parent, default in child', function(done): void { + // 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 cp = require('child_process'); + 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)', function(done): void { const term = new UnixTerminal('node', [ '-e', ` console.log('ready'); setTimeout(()=>console.log('timeout'), 200);` @@ -164,7 +206,7 @@ if (process.platform !== 'win32') { done(); }); }); - it('SIGUSR1', function(done): void { + it('SIGUSR1 - custom in parent and child', function(done): void { let pHandlerCalled = 0; const handleSigUsr = function(h: any): any { return function(): void {