Skip to content

Commit bb3bf09

Browse files
piscisaureuspiscisaureus
committed
Fix several child process bugs on windows
1 parent 5087c62 commit bb3bf09

2 files changed

Lines changed: 107 additions & 96 deletions

File tree

‎src/node_child_process.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ class ChildProcess : ObjectWrap {
9999
static void watch(ChildProcess *child);
100100
static void CALLBACK watch_wait_callback(void *data, BOOLEAN didTimeout);
101101
static void notify_spawn_failure(ChildProcess *child);
102-
static void notify_exit(ev_async *ev, int revent);
102+
static void notify_exit(EV_P_ ev_async *ev, int revent);
103103
static int do_kill(ChildProcess *child, int sig);static void close_stdio_handles(ChildProcess *child);
104104

105105
int pid_;

‎src/node_child_process_win32.cc‎

Lines changed: 106 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -50,16 +50,18 @@ static inline WCHAR* search_path_join_test(
5050
WCHAR *result, *result_pos;
5151

5252
if (dir_len >= 1 && (dir[0] == L'/' || dir[0] == L'\\')) {
53-
// It's a full path with drive letter, don't use cwd
54-
cwd_len = 0;
55-
} else if (dir_len == 2 && dir[1] == L':') {
53+
// It's a full path without drive letter, use cwd's drive letter only
54+
cwd_len = 2;
55+
} else if (dir_len >= 2 && dir[1] == L':' &&
56+
(dir_len < 3 || (dir[2] != L'/' && dir[2] != L'\\'))) {
5657
// It's a relative path with drive letter (ext.g. D:../some/file)
57-
// Replace dir by full cwd if it points to the same drive,
58+
// Replace drive letter in dir by full cwd if it points to the same drive,
5859
// otherwise use the dir only.
5960
if (cwd_len < 2 || _wcsnicmp(cwd, dir, 2) != 0) {
6061
cwd_len = 0;
6162
} else {
62-
dir_len = 0;
63+
dir += 2;
64+
dir_len -= 2;
6365
}
6466
} else if (dir_len > 2 && dir[1] == L':') {
6567
// It's an absolute path with drive letter
@@ -327,7 +329,7 @@ void ChildProcess::close_stdio_handles(ChildProcess *child) {
327329

328330

329331
// Called from the main thread
330-
void ChildProcess::notify_exit(ev_async *ev, int revent) {
332+
void ChildProcess::notify_exit(EV_P_ ev_async *ev, int revent) {
331333
// Get the child process, then release the lock
332334
ChildProcess *child = watcher_status.child;
333335

@@ -355,15 +357,13 @@ void ChildProcess::notify_exit(ev_async *ev, int revent) {
355357
}
356358

357359
// Close and unset the process handle
358-
EnterCriticalSection(&child->info_lock_);
359360
CloseHandle(child->process_handle_);
360361
child->process_handle_ = NULL;
361362
child->pid_ = 0;
362363
}
363364

364365
LeaveCriticalSection(&child->info_lock_);
365366

366-
// The process never even started
367367
child->OnExit(exit_code);
368368
}
369369

@@ -381,7 +381,7 @@ void ChildProcess::notify_spawn_failure(ChildProcess *child) {
381381

382382
watcher_status.child = child;
383383

384-
ev_async_send(&watcher_status.async_watcher);
384+
ev_async_send(EV_DEFAULT_UC_ &watcher_status.async_watcher);
385385
}
386386

387387

@@ -401,7 +401,7 @@ void CALLBACK ChildProcess::watch_wait_callback(void *data,
401401
assert(result == WAIT_OBJECT_0);
402402

403403
watcher_status.child = child;
404-
ev_async_send(&watcher_status.async_watcher);
404+
ev_async_send(EV_DEFAULT_UC_ &watcher_status.async_watcher);
405405
}
406406

407407

@@ -415,7 +415,7 @@ inline void ChildProcess::watch(ChildProcess *child) {
415415
}
416416

417417
// We must retain the lock here because we don't want the RegisterWait
418-
// to complete before the waithandle is set to the child process.
418+
// to complete before the wait handle is set to the child process.
419419
RegisterWaitForSingleObject(&child->wait_handle_, child->process_handle_,
420420
watch_wait_callback, (void*)child, INFINITE,
421421
WT_EXECUTEINWAITTHREAD | WT_EXECUTEONLYONCE);
@@ -480,54 +480,57 @@ int ChildProcess::do_spawn(eio_req *req) {
480480
WCHAR* application_path = search_path(child->application_, child->cwd_,
481481
child->path_, child->path_ext_);
482482

483-
STARTUPINFOW startup;
484-
PROCESS_INFORMATION info;
485-
486-
startup.cb = sizeof(startup);
487-
startup.lpReserved = NULL;
488-
startup.lpDesktop = NULL;
489-
startup.lpTitle = NULL;
490-
startup.dwFlags = STARTF_USESTDHANDLES;
491-
startup.cbReserved2 = 0;
492-
startup.lpReserved2 = NULL;
493-
startup.hStdInput = child->stdio_handles_[0];
494-
startup.hStdOutput = child->stdio_handles_[1];
495-
startup.hStdError = child->stdio_handles_[2];
483+
if (application_path) {
484+
STARTUPINFOW startup;
485+
PROCESS_INFORMATION info;
486+
487+
startup.cb = sizeof(startup);
488+
startup.lpReserved = NULL;
489+
startup.lpDesktop = NULL;
490+
startup.lpTitle = NULL;
491+
startup.dwFlags = STARTF_USESTDHANDLES;
492+
startup.cbReserved2 = 0;
493+
startup.lpReserved2 = NULL;
494+
startup.hStdInput = child->stdio_handles_[0];
495+
startup.hStdOutput = child->stdio_handles_[1];
496+
startup.hStdError = child->stdio_handles_[2];
496497

497-
EnterCriticalSection(&child->info_lock_);
498+
EnterCriticalSection(&child->info_lock_);
498499

499-
if (!child->kill_me_) {
500-
// Try start the process
501-
BOOL success = CreateProcessW(
502-
application_path,
503-
child->arguments_,
504-
NULL,
505-
NULL,
506-
1,
507-
CREATE_UNICODE_ENVIRONMENT,
508-
child->env_win_,
509-
child->cwd_,
510-
&startup,
511-
&info
512-
);
513-
514-
if (success) {
515-
child->process_handle_ = info.hProcess;
516-
child->pid_ = GetProcessId(info.hProcess);
517-
child->did_start_ = true;
518-
watch(child);
519-
520-
// Not interesting
521-
CloseHandle(info.hThread);
522-
523-
return 0;
500+
if (!child->kill_me_) {
501+
// Try start the process
502+
BOOL success = CreateProcessW(
503+
application_path,
504+
child->arguments_,
505+
NULL,
506+
NULL,
507+
1,
508+
CREATE_UNICODE_ENVIRONMENT,
509+
child->env_win_,
510+
child->cwd_,
511+
&startup,
512+
&info
513+
);
514+
515+
if (success) {
516+
child->process_handle_ = info.hProcess;
517+
child->pid_ = GetProcessId(info.hProcess);
518+
child->did_start_ = true;
519+
watch(child);
520+
521+
// Not interesting
522+
CloseHandle(info.hThread);
523+
524+
LeaveCriticalSection(&child->info_lock_);
525+
return 0;
526+
}
524527
}
528+
529+
LeaveCriticalSection(&child->info_lock_);
525530
}
526531

527-
// kill_me set or process failed to start
528-
LeaveCriticalSection(&child->info_lock_);
532+
// not found, kill_me set or process failed to start
529533
notify_spawn_failure(child);
530-
531534
return 0;
532535
}
533536

@@ -560,23 +563,23 @@ int ChildProcess::after_spawn(eio_req *req) {
560563
// Called from the main thread while eio/wait threads may still be busy with
561564
// the process
562565
int ChildProcess::do_kill(ChildProcess *child, int sig) {
566+
int rv = 0;
567+
563568
EnterCriticalSection(&child->info_lock_);
564569

565570
child->exit_signal_ = sig;
566571

567572
if (child->did_start_) {
568573
// On windows killed processes normally return 1
569-
if (TerminateProcess(child->process_handle_, 1) != 0) {
570-
return 0;
571-
} else {
572-
return GetLastError();
573-
}
574+
if (!TerminateProcess(child->process_handle_, 1))
575+
rv = -1;
574576
} else {
575577
child->kill_me_ = true;
576-
return 0;
577578
}
578579

579580
LeaveCriticalSection(&child->info_lock_);
581+
582+
return rv;
580583
}
581584

582585

@@ -609,57 +612,58 @@ Handle<Value> ChildProcess::Spawn(const Arguments& args) {
609612
ChildProcess *child = ObjectWrap::Unwrap<ChildProcess>(args.Holder());
610613

611614
// Copy appplication name
612-
String::Value application(args[0]->ToString());
613-
child->application_ = _wcsdup((WCHAR*)*application);
615+
Handle<String> app_handle = args[0]->ToString();
616+
int app_len = app_handle->Length();
617+
String::Value app(app_handle);
618+
child->application_ = new WCHAR[app_len + 1];
619+
wcsncpy(child->application_, (WCHAR*)*app, app_len + 1);
614620

615621
/*
616622
* Copy second argument args[1] into a c-string called argv.
617623
* On windows command line arguments are all quoted and concatenated to
618-
* one string.
619-
* Assuming that all arguments must be wrapped in quotes,
624+
* one string. The executable name must be prepended. This is not really
625+
* required by windows but if you don't do this programs that rely on
626+
* argv[0] being the executable misbehave.
627+
* Assuming that executable plus all arguments must be wrapped in quotes,
620628
* every character needs to be quoted with a backslash,
621629
* and every argument is followed by either a space or a nul char,
622-
* the maximum required buffer size is Σ[arg1..argc](2 * length + 3).
630+
* the maximum required buffer size is Σ[exe and args](2 * length + 3).
623631
*/
624632
Local<Array> cmd_args_handle = Local<Array>::Cast(args[1]);
625633
int cmd_argc = cmd_args_handle->Length();
626634

627-
if (cmd_argc > 0) {
628-
// Compute required buffer
629-
int max_buf = cmd_argc * 3,
630-
i;
631-
for (i = 0; i < cmd_argc; i++) {
632-
Local<String> arg_handle =
633-
cmd_args_handle->Get(Integer::New(i))->ToString();
634-
max_buf += 2 * arg_handle->Length();
635-
}
636-
637-
child->arguments_ = new WCHAR[max_buf];
638-
WCHAR *pos = child->arguments_;
639-
for (i = 0; i < cmd_argc - 1; i++) {
640-
String::Value arg(cmd_args_handle->Get(Integer::New(i))->ToString());
641-
pos = quote_cmd_arg((WCHAR*)*arg, pos, L' ');
642-
}
643-
String::Value arg(cmd_args_handle->Get(Integer::New(i))->ToString());
644-
quote_cmd_arg((WCHAR*)*arg, pos, L'\0');
635+
// Compute required buffer
636+
int max_buf = (1 + cmd_argc) * 3 + app_len * 2,
637+
i;
638+
for (i = 0; i < cmd_argc; i++) {
639+
Local<String> arg_handle =
640+
cmd_args_handle->Get(Integer::New(i))->ToString();
641+
max_buf += arg_handle->Length() * 2;
642+
}
645643

646-
} else {
647-
// No arguments
648-
child->arguments_ = _wcsdup(L"\0");
644+
child->arguments_ = new WCHAR[max_buf];
645+
WCHAR *pos = child->arguments_;
646+
pos = quote_cmd_arg((WCHAR*)*app, pos, cmd_argc ? L' ' : L'\0');
647+
for (i = 0; i < cmd_argc; i++) {
648+
String::Value arg(cmd_args_handle->Get(Integer::New(i))->ToString());
649+
pos = quote_cmd_arg((WCHAR*)*arg, pos, (i < cmd_argc - 1) ? L' ' : L'\0');
649650
}
650651

651-
// Copy command-line arguments
652+
// Current working directory
652653
Local<String>cwd_handle = Local<String>::Cast(args[2]);
653-
if (cwd_handle->Length() > 0) {
654+
int cwd_len = cwd_handle->Length();
655+
if (cwd_len > 0) {
654656
// Cwd was specified
655-
String::Value cwd(args[2]);
656-
child->cwd_ = _wcsdup((WCHAR*)*cwd);
657+
String::Value cwd(cwd_handle);
658+
child->cwd_ = new WCHAR[cwd_len + 1];
659+
wcsncpy(child->cwd_, (WCHAR*)*cwd, cwd_len + 1);
657660
} else {
658661
// Cwd not specified
659662
int chars = GetCurrentDirectoryW(0, NULL);
660663
if (!chars) {
661664
winapi_perror("GetCurrentDirectoryW");
662-
child->cwd_ = _wcsdup(L"");
665+
child->cwd_ = new WCHAR[0];
666+
child->cwd_[0] = '\0';
663667
} else {
664668
child->cwd_ = new WCHAR[chars];
665669
GetCurrentDirectoryW(chars, child->cwd_);
@@ -765,10 +769,11 @@ Handle<Value> ChildProcess::Spawn(const Arguments& args) {
765769
// Use this custom fd
766770
HANDLE custom_handle = (HANDLE)_get_osfhandle(custom_fd);
767771

768-
// Make handle inheritable
769-
if (!SetHandleInformation(child_handles[i], HANDLE_FLAG_INHERIT,
770-
HANDLE_FLAG_INHERIT))
771-
winapi_perror("SetHandleInformation");
772+
// Make handle inheritable, don't care it it fails
773+
// It may fail for certain types of handles - but always try to
774+
// spawn; it'll still work for e.g. console handles
775+
SetHandleInformation(custom_handle, HANDLE_FLAG_INHERIT,
776+
HANDLE_FLAG_INHERIT);
772777

773778
has_custom_fds[i] = true;
774779
child_handles[i] = custom_handle;
@@ -785,6 +790,9 @@ Handle<Value> ChildProcess::Spawn(const Arguments& args) {
785790
assert(parent_fds[2] >= 0);
786791
result->Set(2, Integer::New(parent_fds[2]));
787792

793+
// Grab a reference so it doesn't get GC'ed
794+
child->Ref();
795+
788796
eio_custom(do_spawn, EIO_PRI_DEFAULT, after_spawn, (void*)child);
789797

790798
return scope.Close(result);
@@ -819,6 +827,9 @@ Handle<Value> ChildProcess::Kill(const Arguments& args) {
819827
void ChildProcess::OnExit(int status) {
820828
HandleScope scope;
821829

830+
// Unref() the child, as it's no longer used by threads
831+
Unref();
832+
822833
handle_->Set(pid_symbol, Null());
823834

824835
Local<Value> onexit_v = handle_->Get(onexit_symbol);
@@ -859,7 +870,7 @@ void ChildProcess::Initialize(Handle<Object> target) {
859870

860871
target->Set(String::NewSymbol("ChildProcess"), t->GetFunction());
861872

862-
ev_async_init(&watcher_status.async_watcher, notify_exit);
873+
ev_async_init(EV_DEFAULT_UC_ &watcher_status.async_watcher, notify_exit);
863874
watcher_status.lock = CreateSemaphore(NULL, 1, 1, NULL);
864875
}
865876

0 commit comments

Comments
 (0)