From b00559a04dd6199ff24512e36f2813a965850413 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Sat, 25 Mar 2023 01:47:15 +0900 Subject: [PATCH 1/3] refactor: posix_spawn usage for macOS --- binding.gyp | 15 -- scripts/post-install.js | 1 - src/interfaces.ts | 1 - src/native.d.ts | 2 +- src/unix/comms.h | 5 - src/unix/pty.cc | 464 +++++++++++++++++++++++++-------------- src/unix/spawn-helper.cc | 70 ------ src/unixTerminal.test.ts | 91 +++----- src/unixTerminal.ts | 10 +- typings/node-pty.d.ts | 8 +- 10 files changed, 336 insertions(+), 331 deletions(-) delete mode 100644 src/unix/comms.h delete mode 100644 src/unix/spawn-helper.cc diff --git a/binding.gyp b/binding.gyp index bf1683387..feecbf6d7 100644 --- a/binding.gyp +++ b/binding.gyp @@ -69,21 +69,6 @@ ] }, { # OS!="win" 'targets': [ - { - 'target_name': 'spawn-helper', - 'type': 'executable', - 'cflags': ['-Wall'], - 'sources': [ - 'src/unix/spawn-helper.cc', - ], - 'conditions': [ - ['OS=="mac"', { - "xcode_settings": { - "MACOSX_DEPLOYMENT_TARGET":"10.7" - } - }] - ] - }, { 'target_name': 'pty', 'include_dirs' : [ diff --git a/scripts/post-install.js b/scripts/post-install.js index 745fbdbef..2c026ee5d 100644 --- a/scripts/post-install.js +++ b/scripts/post-install.js @@ -9,7 +9,6 @@ var BUILD_FILES = [ path.join(RELEASE_DIR, 'conpty_console_list.pdb'), path.join(RELEASE_DIR, 'pty.node'), path.join(RELEASE_DIR, 'pty.pdb'), - path.join(RELEASE_DIR, 'spawn-helper'), path.join(RELEASE_DIR, 'winpty-agent.exe'), path.join(RELEASE_DIR, 'winpty-agent.pdb'), path.join(RELEASE_DIR, 'winpty.dll'), diff --git a/src/interfaces.ts b/src/interfaces.ts index b4b8f7f3b..b9ef130b1 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -109,7 +109,6 @@ interface IBasePtyForkOptions { export interface IPtyForkOptions extends IBasePtyForkOptions { uid?: number; gid?: number; - closeFDs?: boolean; } export interface IWindowsPtyForkOptions extends IBasePtyForkOptions { diff --git a/src/native.d.ts b/src/native.d.ts index 4445ad49f..3212e3b77 100644 --- a/src/native.d.ts +++ b/src/native.d.ts @@ -18,7 +18,7 @@ interface IWinptyNative { } interface IUnixNative { - fork(file: string, args: string[], parsedEnv: string[], cwd: string, cols: number, rows: number, uid: number, gid: number, closeFDs: boolean, useUtf8: boolean, onExitCallback: (code: number, signal: number) => void, helperPath: string): IUnixProcess; + fork(file: string, args: string[], parsedEnv: string[], cwd: string, cols: number, rows: number, uid: number, gid: number, useUtf8: boolean, onExitCallback: (code: number, signal: number) => void): IUnixProcess; open(cols: number, rows: number): IUnixOpenProcess; process(fd: number, pty: string): string; resize(fd: number, cols: number, rows: number): void; diff --git a/src/unix/comms.h b/src/unix/comms.h deleted file mode 100644 index b95554370..000000000 --- a/src/unix/comms.h +++ /dev/null @@ -1,5 +0,0 @@ -#define COMM_PIPE_FD (STDERR_FILENO + 1) -#define COMM_ERR_EXEC 1 -#define COMM_ERR_CHDIR 2 -#define COMM_ERR_SETUID 3 -#define COMM_ERR_SETGID 4 diff --git a/src/unix/pty.cc b/src/unix/pty.cc index ac54d2582..4572c0a71 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -29,26 +29,15 @@ #include #include #include -#include - -#include "comms.h" /* forkpty */ /* http://www.gnu.org/software/gnulib/manual/html_node/forkpty.html */ -#if defined(__GLIBC__) || defined(__CYGWIN__) +#if defined(__linux__) #include -#elif defined(__APPLE__) || defined(__OpenBSD__) || defined(__NetBSD__) +#elif defined(__APPLE__) #include -#elif defined(__FreeBSD__) -#include -#elif defined(__sun) -#include /* for I_PUSH */ -#else -#include #endif -#include /* tcgetattr, tty_ioctl */ - /* Some platforms name VWERASE and VDISCARD differently */ #if !defined(VWERASE) && defined(VWERSE) #define VWERASE VWERSE @@ -62,8 +51,12 @@ #include #include #elif defined(__APPLE__) -#include #include +#include +#include +#include +#include +#include #endif /* NSIG - macro for highest signal + 1, should be defined */ @@ -71,15 +64,26 @@ #define NSIG 32 #endif -#ifdef POSIX_SPAWN_CLOEXEC_DEFAULT - #define HAVE_POSIX_SPAWN_CLOEXEC_DEFAULT 1 -#else - #define HAVE_POSIX_SPAWN_CLOEXEC_DEFAULT 0 - #define POSIX_SPAWN_CLOEXEC_DEFAULT 0 +/* macOS 10.14 back does not define this constant */ +#ifndef POSIX_SPAWN_SETSID + #define POSIX_SPAWN_SETSID 1024 +#endif + +/* environ for execvpe */ +/* node/src/node_child_process.cc */ +#if !defined(__APPLE__) +extern char **environ; #endif -#ifndef POSIX_SPAWN_USEVFORK - #define POSIX_SPAWN_USEVFORK 0 +#if defined(__APPLE__) +extern "C" { +// Changes the current thread's directory to a path or directory file +// descriptor. libpthread only exposes a syscall wrapper starting in +// macOS 10.12, but the system call dates back to macOS 10.5. On older OSes, +// the syscall is issued directly. +int pthread_chdir_np(const char* dir) API_AVAILABLE(macosx(10.12)); +int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12)); +} #endif /** @@ -114,11 +118,6 @@ pty_nonblock(int); static char * pty_getproc(int, char *); -static int -pty_openpty(int *, int *, char *, - const struct termios *, - const struct winsize *); - static void pty_waitpid(void *); @@ -128,16 +127,21 @@ pty_after_waitpid(uv_async_t *); static void pty_after_close(uv_handle_t *); -static void throw_for_errno(const char* message, int _errno) { - Nan::ThrowError(( - message + std::string(strerror(_errno)) - ).c_str()); -} +#if defined(__APPLE__) || defined(__OpenBSD__) +static void +pty_posix_spawn(char** argv, char** env, + char* cwd, + const struct termios *termp, + const struct winsize *winp, + int* master, + pid_t* pid, + int* err); +#endif NAN_METHOD(PtyFork) { Nan::HandleScope scope; - if (info.Length() != 12 || + if (info.Length() != 10 || !info[0]->IsString() || !info[1]->IsArray() || !info[2]->IsArray() || @@ -147,11 +151,9 @@ NAN_METHOD(PtyFork) { !info[6]->IsNumber() || !info[7]->IsNumber() || !info[8]->IsBoolean() || - !info[9]->IsBoolean() || - !info[10]->IsFunction() || - !info[11]->IsString()) { + !info[9]->IsFunction()) { return Nan::ThrowError( - "Usage: pty.fork(file, args, env, cwd, cols, rows, uid, gid, utf8, closeFDs, onexit, helperPath)"); + "Usage: pty.fork(file, args, env, cwd, cols, rows, uid, gid, utf8, onexit)"); } // file @@ -172,6 +174,7 @@ NAN_METHOD(PtyFork) { // cwd Nan::Utf8String cwd_(info[3]); + char* cwd = strdup(*cwd_); // size struct winsize winp; @@ -180,9 +183,11 @@ NAN_METHOD(PtyFork) { winp.ws_xpixel = 0; winp.ws_ypixel = 0; +#if !defined(__APPLE__) // uid / gid int uid = info[6]->IntegerValue(Nan::GetCurrentContext()).FromJust(); int gid = info[7]->IntegerValue(Nan::GetCurrentContext()).FromJust(); +#endif // termios struct termios t = termios(); @@ -219,36 +224,35 @@ NAN_METHOD(PtyFork) { term->c_cc[VSTATUS] = 20; #endif - // closeFDs - bool closeFDs = Nan::To(info[9]).FromJust(); - bool explicitlyCloseFDs = closeFDs && !HAVE_POSIX_SPAWN_CLOEXEC_DEFAULT; - - // helperPath - Nan::Utf8String helper_path_(info[11]); - char *helper_path = strdup(*helper_path_); + cfsetispeed(term, B38400); + cfsetospeed(term, B38400); - const int EXTRA_ARGS = 6; int argc = argv_->Length(); - int argl = argc + EXTRA_ARGS + 1; + int argl = argc + 2; char **argv = new char*[argl]; - argv[0] = strdup(helper_path); - argv[1] = strdup(*cwd_); - argv[2] = strdup(std::to_string(uid).c_str()); - argv[3] = strdup(std::to_string(gid).c_str()); - argv[4] = strdup(explicitlyCloseFDs ? "1": "0"); - argv[5] = strdup(*file); + argv[0] = strdup(*file); argv[argl - 1] = NULL; for (int i = 0; i < argc; i++) { Nan::Utf8String arg(Nan::Get(argv_, i).ToLocalChecked()); - argv[i + EXTRA_ARGS] = strdup(*arg); + argv[i + 1] = strdup(*arg); } - cfsetispeed(term, B38400); - cfsetospeed(term, B38400); - + pid_t pid; + int master; +#if defined(__APPLE__) + int err = 0; + pty_posix_spawn(argv, env, cwd, term, &winp, &master, &pid, &err); + if (err != 0) { + Nan::ThrowError("posix_spawnp failed."); + goto done; + } + if (pty_nonblock(master) == -1) { + Nan::ThrowError("Could not set master fd to nonblocking."); + goto done; + } +#else sigset_t newmask, oldmask; - int flags = POSIX_SPAWN_USEVFORK; - + 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 @@ -256,72 +260,69 @@ NAN_METHOD(PtyFork) { sigfillset(&newmask); pthread_sigmask(SIG_SETMASK, &newmask, &oldmask); - int master, slave; - int ret = pty_openpty(&master, &slave, nullptr, term, &winp); - if (ret == -1) { - perror("openpty failed"); - Nan::ThrowError("openpty failed."); - goto done; - } + pid = forkpty(&master, nullptr, static_cast(term), static_cast(&winp)); - int comms_pipe[2]; - if (pipe(comms_pipe)) { - perror("pipe() failed"); - Nan::ThrowError("pipe() failed."); - goto done; - } + 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); + } + } else { + for (int i = 0; i < argl; i++) free(argv[i]); + delete[] argv; - posix_spawn_file_actions_t acts; - posix_spawn_file_actions_init(&acts); - posix_spawn_file_actions_adddup2(&acts, slave, STDIN_FILENO); - posix_spawn_file_actions_adddup2(&acts, slave, STDOUT_FILENO); - posix_spawn_file_actions_adddup2(&acts, slave, STDERR_FILENO); - posix_spawn_file_actions_adddup2(&acts, comms_pipe[1], COMM_PIPE_FD); - posix_spawn_file_actions_addclose(&acts, comms_pipe[1]); + for (int i = 0; i < envc; i++) free(env[i]); + delete[] env; - posix_spawnattr_t attrs; - posix_spawnattr_init(&attrs); - if (closeFDs) { - flags |= POSIX_SPAWN_CLOEXEC_DEFAULT; + free(cwd); } - posix_spawnattr_setflags(&attrs, flags); - - { // suppresses "jump bypasses variable initialization" errors - pid_t pid; - auto error = posix_spawn(&pid, argv[0], &acts, &attrs, argv, env); - - close(comms_pipe[1]); - // reenable signals - pthread_sigmask(SIG_SETMASK, &oldmask, NULL); + // reenable signals + pthread_sigmask(SIG_SETMASK, &oldmask, NULL); - if (error) { - throw_for_errno("posix_spawn failed: ", error); + switch (pid) { + case -1: + Nan::ThrowError("forkpty(3) failed."); goto done; - } + case 0: + if (strlen(cwd)) { + if (chdir(cwd) == -1) { + perror("chdir(2) failed."); + _exit(1); + } + } - int helper_error[2]; - auto bytes_read = read(comms_pipe[0], &helper_error, sizeof(helper_error)); - close(comms_pipe[0]); - - if (bytes_read == sizeof(helper_error)) { - if (helper_error[0] == COMM_ERR_EXEC) { - throw_for_errno("exec() failed: ", helper_error[1]); - } else if (helper_error[0] == COMM_ERR_CHDIR) { - throw_for_errno("chdir() failed: ", helper_error[1]); - } else if (helper_error[0] == COMM_ERR_SETUID) { - throw_for_errno("setuid() failed: ", helper_error[1]); - } else if (helper_error[0] == COMM_ERR_SETGID) { - throw_for_errno("setgid() failed: ", helper_error[1]); + if (uid != -1 && gid != -1) { + if (setgid(gid) == -1) { + perror("setgid(2) failed."); + _exit(1); + } + if (setuid(uid) == -1) { + perror("setuid(2) failed."); + _exit(1); + } } - goto done; - } - if (pty_nonblock(master) == -1) { - Nan::ThrowError("Could not set master fd to nonblocking."); - goto done; - } + { + char **old = environ; + environ = env; + execvp(argv[0], argv); + environ = old; + perror("execvp(3) failed."); + _exit(1); + } + default: + if (pty_nonblock(master) == -1) { + Nan::ThrowError("Could not set master fd to nonblocking."); + goto done; + } + } +#endif + { v8::Local obj = Nan::New(); Nan::Set(obj, Nan::New("fd").ToLocalChecked(), @@ -336,7 +337,7 @@ NAN_METHOD(PtyFork) { pty_baton *baton = new pty_baton(); baton->exit_code = 0; baton->signal_code = 0; - baton->cb.Reset(v8::Local::Cast(info[10])); + baton->cb.Reset(v8::Local::Cast(info[9])); baton->pid = pid; baton->async.data = baton; @@ -344,21 +345,20 @@ NAN_METHOD(PtyFork) { uv_thread_create(&baton->tid, pty_waitpid, static_cast(baton)); - info.GetReturnValue().Set(obj); + return info.GetReturnValue().Set(obj); } + done: - posix_spawn_file_actions_destroy(&acts); - posix_spawnattr_destroy(&attrs); +#if defined(__APPLE__) + for (int i = 0; i < argl; i++) free(argv[i]); + delete[] argv; - if (argv) { - for (int i = 0; i < argl; i++) free(argv[i]); - delete[] argv; - } - if (env) { - for (int i = 0; i < envc; i++) free(env[i]); - delete[] env; - } - free(helper_path); + for (int i = 0; i < envc; i++) free(env[i]); + delete[] env; + + free(cwd); +#endif + return info.GetReturnValue().SetUndefined(); } NAN_METHOD(PtyOpen) { @@ -379,7 +379,7 @@ NAN_METHOD(PtyOpen) { // pty int master, slave; - int ret = pty_openpty(&master, &slave, nullptr, NULL, &winp); + int ret = openpty(&master, &slave, nullptr, NULL, static_cast(&winp)); if (ret == -1) { return Nan::ThrowError("openpty(3) failed."); @@ -648,50 +648,194 @@ pty_getproc(int fd, char *tty) { #endif -/** - * openpty(3) / forkpty(3) - */ +#if defined(__APPLE__) +static void +pty_posix_spawn(char** argv, char** env, + char* cwd, + const struct termios *termp, + const struct winsize *winp, + int* master, + pid_t* pid, + int* err) { + int low_fds[3]; + size_t count = 0; + const char *p; + const char *z; + size_t l; + size_t k; + int seen_eacces; + const char *path; + char** env_iterator; + const char path_var[] = "PATH="; + + for (; count < 3; count++) { + low_fds[count] = posix_openpt(O_RDWR); + if (low_fds[count] >= STDERR_FILENO) + break; + } -static int -pty_openpty(int *amaster, - int *aslave, - char *name, - const struct termios *termp, - const struct winsize *winp) { -#if defined(__sun) - char *slave_name; + int flags = POSIX_SPAWN_CLOEXEC_DEFAULT | + POSIX_SPAWN_SETSIGDEF | + POSIX_SPAWN_SETSIGMASK | + POSIX_SPAWN_SETSID; + *master = posix_openpt(O_RDWR); + if (*master == -1) { + return; + } + + int res = grantpt(*master) || unlockpt(*master); + if (res == -1) { + return; + } + + // Use TIOCPTYGNAME instead of ptsname() to avoid threading problems. int slave; - int master = open("/dev/ptmx", O_RDWR | O_NOCTTY); - if (master == -1) return -1; - if (amaster) *amaster = master; + char slave_pty_name[128]; + res = ioctl(*master, TIOCPTYGNAME, slave_pty_name); + if (res == -1) { + return; + } - if (grantpt(master) == -1) goto err; - if (unlockpt(master) == -1) goto err; + slave = open(slave_pty_name, O_RDWR | O_NOCTTY); + if (slave == -1) { + return; + } - slave_name = ptsname(master); - if (slave_name == NULL) goto err; - if (name) strcpy(name, slave_name); + if (termp) { + res = tcsetattr(slave, TCSANOW, termp); + if (res == -1) { + return; + }; + } - slave = open(slave_name, O_RDWR | O_NOCTTY); - if (slave == -1) goto err; - if (aslave) *aslave = slave; + if (winp) { + res = ioctl(slave, TIOCSWINSZ, winp); + if (res == -1) { + return; + } + } - ioctl(slave, I_PUSH, "ptem"); - ioctl(slave, I_PUSH, "ldterm"); - ioctl(slave, I_PUSH, "ttcompat"); + posix_spawn_file_actions_t acts; + posix_spawn_file_actions_init(&acts); + posix_spawn_file_actions_adddup2(&acts, slave, STDIN_FILENO); + posix_spawn_file_actions_adddup2(&acts, slave, STDOUT_FILENO); + posix_spawn_file_actions_adddup2(&acts, slave, STDERR_FILENO); + posix_spawn_file_actions_addclose(&acts, slave); + posix_spawn_file_actions_addclose(&acts, *master); + if (strlen(cwd)) { + if (__builtin_available(macOS 10.15, *)) { + posix_spawn_file_actions_addchdir_np(&acts, cwd); + } else { + *err = pthread_chdir_np(cwd); + if (*err != 0) { + goto done; + } + } + } - if (termp) tcsetattr(slave, TCSAFLUSH, termp); - if (winp) ioctl(slave, TIOCSWINSZ, winp); + posix_spawnattr_t attrs; + posix_spawnattr_init(&attrs); + *err = posix_spawnattr_setflags(&attrs, flags); + if (*err != 0) { + goto done; + } - return 0; + sigset_t signal_set; + /* Reset all signal the child to their default behavior */ + sigfillset(&signal_set); + *err = posix_spawnattr_setsigdefault(&attrs, &signal_set); + if (*err != 0) { + goto done; + } -err: - close(master); - return -1; -#else - return openpty(amaster, aslave, name, (termios *)termp, (winsize *)winp); -#endif + /* Reset the signal mask for all signals */ + sigemptyset(&signal_set); + *err = posix_spawnattr_setsigmask(&attrs, &signal_set); + if (*err != 0) { + goto done; + } + + // path resolution is copied from + // https://github.com/libuv/libuv/blob/7b84d5b0ecb737b4cc30ce63eade690d994e00a6/src/unix/process.c#L695-L764 + if (strchr(argv[0], '/') != NULL) { + do + *err = posix_spawn(pid, argv[0], &acts, &attrs, argv, env); + while (*err == EINTR); + } else { + + for (env_iterator = env; *env_iterator != NULL; env_iterator++) { + if (strncmp(*env_iterator, path_var, sizeof(path_var) - 1) == 0) { + /* Found "PATH=" at the beginning of the string */ + path = *env_iterator + sizeof(path_var) - 1; + } + } + + if (path == NULL) + path = _PATH_DEFPATH; + + k = strnlen(argv[0], NAME_MAX + 1); + if (k > NAME_MAX) + goto done; + + l = strnlen(path, PATH_MAX - 1) + 1; + + for (p = path;; p = z) { + /* Compose the new process file from the entry in the PATH + * environment variable and the actual file name */ + char b[PATH_MAX + NAME_MAX]; + z = strchr(p, ':'); + if (!z) + z = p + strlen(p); + if ((size_t)(z - p) >= l) { + if (!*z++) + break; + + continue; + } + memcpy(b, p, z - p); + b[z - p] = '/'; + memcpy(b + (z - p) + (z > p), argv[0], k + 1); + + do + *err = posix_spawn(pid, b, &acts, &attrs, argv, env); + while (*err == EINTR); + + switch (*err) { + case EACCES: + seen_eacces = 1; + break; /* continue search */ + case ENOENT: + case ENOTDIR: + break; /* continue search */ + default: + goto done; + } + + if (!*z++) + break; + } + + if (seen_eacces) + goto done; + } + + // Restore the thread's working directory if it was changed. + if (strlen(cwd)) { + if (__builtin_available(macOS 10.15, *)) { + // __builtin_available is special and cannot be negated. + } else { + pthread_fchdir_np(-1); + } + } +done: + posix_spawn_file_actions_destroy(&acts); + posix_spawnattr_destroy(&attrs); + + for (; count > 0; count--) { + close(low_fds[count]); + } } +#endif /** * Init diff --git a/src/unix/spawn-helper.cc b/src/unix/spawn-helper.cc deleted file mode 100644 index f792437c9..000000000 --- a/src/unix/spawn-helper.cc +++ /dev/null @@ -1,70 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include "comms.h" - -void bail (int type, int code) { - int buf[2] = { type, code }; - (void)! write(COMM_PIPE_FD, &buf, sizeof(buf)); - _exit(1); -} - -int main (int argc, char** argv) { - sigset_t empty_set; - sigemptyset(&empty_set); - pthread_sigmask(SIG_SETMASK, &empty_set, nullptr); - - setsid(); - -#if defined(TIOCSCTTY) - // glibc does this - if (ioctl(STDIN_FILENO, TIOCSCTTY, NULL) == -1) { - _exit(1); - } -#else - char *slave_path = ttyname(STDIN_FILENO); - // open implicit attaches a process to a terminal device if: - // - process has no controlling terminal yet - // - O_NOCTTY is not set - close(open(slave_path, O_RDWR)); -#endif - - char *cwd = argv[1]; - int uid = std::stoi(argv[2]); - int gid = std::stoi(argv[3]); - bool closeFDs = std::stoi(argv[4]); - char *file = argv[5]; - argv = &argv[5]; - - fcntl(COMM_PIPE_FD, F_SETFD, FD_CLOEXEC); - - if (strlen(cwd) && chdir(cwd) == -1) { - bail(COMM_ERR_CHDIR, errno); - } - if (uid != -1 && setuid(uid) == -1) { - bail(COMM_ERR_SETUID, errno); - } - if (gid != -1 && setgid(gid) == -1) { - bail(COMM_ERR_SETGID, errno); - } - if (closeFDs) { - struct rlimit rlim_ofile; - getrlimit(RLIMIT_NOFILE, &rlim_ofile); - for (rlim_t fd = STDERR_FILENO + 1; fd < rlim_ofile.rlim_cur; fd++) { - if (fd != COMM_PIPE_FD) { - close(fd); - } - } - } - - execvp(file, argv); - bail(COMM_ERR_EXEC, errno); - return 1; -} diff --git a/src/unixTerminal.test.ts b/src/unixTerminal.test.ts index 48069bb48..aa0b98416 100644 --- a/src/unixTerminal.test.ts +++ b/src/unixTerminal.test.ts @@ -255,75 +255,42 @@ if (process.platform !== 'win32') { }); }); describe('spawn', () => { - it('should handle exec() errors', (done) => { - try { - new UnixTerminal('/bin/bogus.exe', []); - done(new Error('should have failed')); - } catch { - done(); - } - }); - it('should handle chdir() errors', (done) => { - try { - new UnixTerminal('/bin/echo', [], { cwd: '/nowhere' }); - done(new Error('should have failed')); - } catch (e) { - assert.strictEqual((e as any).toString(), 'Error: chdir() failed: No such file or directory'); - done(); - } - }); - it('should handle setuid() errors', (done) => { - try { - new UnixTerminal('/bin/echo', [], { uid: 999999 }); - done(new Error('should have failed')); - } catch (e) { - assert.strictEqual((e as any).toString(), 'Error: setuid() failed: Operation not permitted'); - done(); - } - }); if (process.platform === 'linux') { - it('should not close on exec when closeFDs is not defined', (done) => { - const data = ` - var pty = require('./lib/index'); - var ptyProcess = pty.spawn('node', ['-e', 'setTimeout(() => console.log("hello from terminal"), 300);']); - ptyProcess.on('data', function (data) { - console.log(data); - }); - setTimeout(() => null, 500); - console.log('ready', ptyProcess.pid); - `; - const buffer: string[] = []; - const readFd = fs.openSync(FIXTURES_PATH, 'r'); - const p = cp.spawn('node', ['-e', data], { - stdio: ['ignore', 'pipe', 'pipe', readFd] - }); - let sub = ''; - p.stdout!.on('data', (data) => { - if (!data.toString().indexOf('ready')) { - sub = data.toString().split(' ')[1].slice(0, -1); - try { - fs.statSync(`/proc/${sub}/fd/${readFd}`); - } catch (_) { - done('not reachable'); - } - 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, '')); - } + it('should handle exec() errors', (done) => { + const term = new UnixTerminal('/bin/bogus.exe', []); + term.on('exit', (code, signal) => { + assert.strictEqual(code, 1); + done(); }); - p.on('close', () => { + }); + it('should handle chdir() errors', (done) => { + const term = new UnixTerminal('/bin/echo', [], { cwd: '/nowhere' }); + term.on('exit', (code, signal) => { + assert.strictEqual(code, 1); done(); }); }); - it('should close on exec when closeFDs is true', (done) => { + } else if (process.platform === 'darwin') { + it('should handle exec() errors', (done) => { + try { + new UnixTerminal('/bin/bogus.exe', []); + done(new Error('should have failed')); + } catch { + done(); + } + }); + it('should handle chdir() errors', (done) => { + try { + new UnixTerminal('/bin/echo', [], { cwd: '/nowhere' }); + done(new Error('should have failed')); + } catch (e) { + done(); + } + }); + it('should close on exec', (done) => { const data = ` var pty = require('./lib/index'); - var ptyProcess = pty.spawn('node', ['-e', 'setTimeout(() => console.log("hello from terminal"), 300);'], { - closeFDs: true - }); + var ptyProcess = pty.spawn('node', ['-e', 'setTimeout(() => console.log("hello from terminal"), 300);']); ptyProcess.on('data', function (data) { console.log(data); }); diff --git a/src/unixTerminal.ts b/src/unixTerminal.ts index c9f72685c..1930bf20b 100644 --- a/src/unixTerminal.ts +++ b/src/unixTerminal.ts @@ -11,14 +11,11 @@ import { ArgvOrCommandLine } from './types'; import { assign } from './utils'; let pty: IUnixNative; -let helperPath: string; try { pty = require('../build/Release/pty.node'); - helperPath = '../build/Release/spawn-helper'; } catch (outerError) { try { pty = require('../build/Debug/pty.node'); - helperPath = '../build/Debug/spawn-helper'; } catch (innerError) { console.error('innerError', innerError); // Re-throw the exception from the Release require if the Debug require fails as well @@ -26,10 +23,6 @@ try { } } -helperPath = path.resolve(__dirname, helperPath); -helperPath = helperPath.replace('app.asar', 'app.asar.unpacked'); -helperPath = helperPath.replace('node_modules.asar', 'node_modules.asar.unpacked'); - const DEFAULT_FILE = 'sh'; const DEFAULT_NAME = 'xterm'; const DESTROY_SOCKET_TIMEOUT_MS = 200; @@ -69,7 +62,6 @@ export class UnixTerminal extends Terminal { this._rows = opt.rows || DEFAULT_ROWS; const uid = opt.uid ?? -1; const gid = opt.gid ?? -1; - const closeFDs = opt.closeFDs || false; const env: IProcessEnv = assign({}, opt.env); if (opt.env === process.env) { @@ -112,7 +104,7 @@ export class UnixTerminal extends Terminal { }; // fork - const term = pty.fork(file, args, parsedEnv, cwd, this._cols, this._rows, uid, gid, (encoding === 'utf8'), closeFDs, onexit, helperPath); + const term = pty.fork(file, args, parsedEnv, cwd, this._cols, this._rows, uid, gid, (encoding === 'utf8'), onexit); this._socket = new PipeSocket(term.fd); if (encoding !== null) { diff --git a/typings/node-pty.d.ts b/typings/node-pty.d.ts index 4dc542785..270afb77e 100644 --- a/typings/node-pty.d.ts +++ b/typings/node-pty.d.ts @@ -78,17 +78,11 @@ declare module 'node-pty' { export interface IPtyForkOptions extends IBasePtyForkOptions { /** - * Security warning: use this option with great caution unless `closeFDs` is also set, + * Security warning: use this option with great caution, * as opened file descriptors with higher privileges might leak to the child program. */ uid?: number; gid?: number; - - /** - * Close all open file after spawning the child process (UNIX only). - * Carries a performance penalty on Linux. - */ - closeFDs?: boolean; } export interface IWindowsPtyForkOptions extends IBasePtyForkOptions { From 6c53a8f857eebc57af466ff186737da67582d53a Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Sat, 25 Mar 2023 13:50:49 +0900 Subject: [PATCH 2/3] chore: add regression spec --- src/unixTerminal.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/unixTerminal.test.ts b/src/unixTerminal.test.ts index aa0b98416..10aa18138 100644 --- a/src/unixTerminal.test.ts +++ b/src/unixTerminal.test.ts @@ -325,6 +325,22 @@ if (process.platform !== 'win32') { }); }); } + it('should not leak child process', (done) => { + const count = cp.execSync('ps -ax | grep node | wc -l'); + const term = new UnixTerminal('node', [ '-e', ` + console.log('ready'); + setTimeout(()=>console.log('timeout'), 200);` + ]); + term.on('data', async (data) => { + if (data === 'ready\r\n') { + process.kill(term.pid, 'SIGINT'); + await setTimeout(() => null, 1000); + const newCount = cp.execSync('ps -ax | grep node | wc -l'); + assert.strictEqual(count.toString(), newCount.toString()); + done(); + } + }); + }); }); }); } From 0507164cdc87eccafd7553c9607c5a4188d3c501 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Sat, 25 Mar 2023 15:21:59 +0900 Subject: [PATCH 3/3] chore: use kqueue to listen for exit event --- src/unix/pty.cc | 51 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/src/unix/pty.cc b/src/unix/pty.cc index 4572c0a71..597ad4682 100644 --- a/src/unix/pty.cc +++ b/src/unix/pty.cc @@ -55,6 +55,7 @@ #include #include #include +#include #include #include #endif @@ -84,6 +85,16 @@ extern "C" { int pthread_chdir_np(const char* dir) API_AVAILABLE(macosx(10.12)); int pthread_fchdir_np(int fd) API_AVAILABLE(macosx(10.12)); } + +#define HANDLE_EINTR(x) ({ \ + int eintr_wrapper_counter = 0; \ + decltype(x) eintr_wrapper_result; \ + do { \ + eintr_wrapper_result = (x); \ + } while (eintr_wrapper_result == -1 && errno == EINTR && \ + eintr_wrapper_counter++ < 100); \ + eintr_wrapper_result; \ +}) #endif /** @@ -486,11 +497,44 @@ static void pty_waitpid(void *data) { int ret; int stat_loc; - pty_baton *baton = static_cast(data); - errno = 0; - +#if defined(__APPLE__) + // Based on + // https://source.chromium.org/chromium/chromium/src/+/main:base/process/kill_mac.cc;l=35-69? + int kq = HANDLE_EINTR(kqueue()); + struct kevent change = {0}; + EV_SET(&change, baton->pid, EVFILT_PROC, EV_ADD, NOTE_EXIT, 0, NULL); + ret = HANDLE_EINTR(kevent(kq, &change, 1, NULL, 0, NULL)); + if (ret == -1) { + if (errno == ESRCH) { + // At this point, one of the following has occurred: + // 1. The process has died but has not yet been reaped. + // 2. The process has died and has already been reaped. + // 3. The process is in the process of dying. It's no longer + // kqueueable, but it may not be waitable yet either. Mark calls + // this case the "zombie death race". + ret = HANDLE_EINTR(waitpid(baton->pid, &stat_loc, WNOHANG)); + if (ret == 0) { + ret = kill(baton->pid, SIGKILL); + if (ret != -1) { + HANDLE_EINTR(waitpid(baton->pid, &stat_loc, 0)); + } + } + } + } else { + struct kevent event = {0}; + ret = HANDLE_EINTR(kevent(kq, NULL, 0, &event, 1, NULL)); + if (ret == 1) { + if ((event.fflags & NOTE_EXIT) && + (event.ident == static_cast(baton->pid))) { + // The process is dead or dying. This won't block for long, if at + // all. + HANDLE_EINTR(waitpid(baton->pid, &stat_loc, 0)); + } + } + } +#else if ((ret = waitpid(baton->pid, &stat_loc, 0)) != baton->pid) { if (ret == -1 && errno == EINTR) { return pty_waitpid(baton); @@ -503,6 +547,7 @@ pty_waitpid(void *data) { assert(false); } } +#endif if (WIFEXITED(stat_loc)) { baton->exit_code = WEXITSTATUS(stat_loc); // errno?