From 4f61ab2058c9baffa01d9c865a376ed8d3c65820 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Thu, 16 May 2013 21:29:40 +0200 Subject: [PATCH 1/3] windows: make uv_spawn not fail under job control * Fix a potential issue introduced with 415f4d3, namely that uv_spawn can fail when the current process is under job control. This would happen on Windows versions that don't support nested jobs (versions prior to Windows 8 / Server 2012). * Change the `uv__init_global_job_handle` function signature to match what `uv_once` expects. * Add a bunch of comments that clarify how we're using job control, and how we're dealing with job control that might be established by our parent process. --- src/win/process.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/src/win/process.c b/src/win/process.c index 8ef420e6..f98767a4 100644 --- a/src/win/process.c +++ b/src/win/process.c @@ -49,7 +49,22 @@ static HANDLE uv_global_job_handle_; static uv_once_t uv_global_job_handle_init_guard_ = UV_ONCE_INIT; -static void uv__init_global_job_handle() { +static void uv__init_global_job_handle(void) { + /* Create a job object and set it up to kill all contained processes when + * it's closed. Since this handle is made non-inheritable and we're not + * giving it to anyone, we're the only process holding a reference to it. + * That means that if this process exits it is closed and all the processes + * it contains are killed. All processes created with uv_spawn that are not + * spawned with the UV_PROCESS_DETACHED flag are assigned to this job. + * + * We're setting the JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK flag so only the + * processes that we explicitly add are affected, and *their* subprocesses + * are not. This ensures that our child processes are not limited in their + * ability to use job control on Windows versions that don't deal with + * nested jobs (prior to Windows 8 / Server 2012). It also lets our child + * processes created detached processes without explicitly breaking away + * from job control (which uv_spawn doesn't, either). + */ SECURITY_ATTRIBUTES attr; JOBOBJECT_EXTENDED_LIMIT_INFORMATION info; @@ -920,7 +935,18 @@ int uv_spawn(uv_loop_t* loop, uv_process_t* process, } process_flags = CREATE_UNICODE_ENVIRONMENT; + if (options.flags & UV_PROCESS_DETACHED) { + /* Note that we're not setting the CREATE_BREAKAWAY_FROM_JOB flag. That + * means that libuv might not let you create a fully deamonized process + * when run under job control. However the type of job control that libuv + * itself creates doesn't trickle down to subprocesses so they can still + * daemonize. + * + * A reason to not do this is that CREATE_BREAKAWAY_FROM_JOB makes the + * CreateProcess call fail if we're under job control that doesn't allow + * breakaway. + */ process_flags |= DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP; } @@ -943,8 +969,21 @@ int uv_spawn(uv_loop_t* loop, uv_process_t* process, if (!(options.flags & UV_PROCESS_DETACHED)) { uv_once(&uv_global_job_handle_init_guard_, uv__init_global_job_handle); - if (!AssignProcessToJobObject(uv_global_job_handle_, info.hProcess)) - uv_fatal_error(GetLastError(), "AssignProcessToJobObject"); + if (!AssignProcessToJobObject(uv_global_job_handle_, info.hProcess)) { + /* AssignProcessToJobObject might fail if this process is under job + * control and the job doesn't have the + * JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK flag set, on a Windows version + * that doesn't support nested jobs. + * + * When that happens we just swallow the error and continue without + * establishing a kill-child-on-parent-exit relationship, otherwise + * there would be no way for libuv applications run under job control + * to spawn processes at all. + */ + DWORD err = GetLastError(); + if (err != ERROR_ACCESS_DENIED) + uv_fatal_error(err, "AssignProcessToJobObject"); + } } /* Set IPC pid to all IPC pipes. */ From d5fa633ef22bd40c81af85dd2ee3882cce3c91c4 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Fri, 17 May 2013 20:31:39 +0400 Subject: [PATCH 2/3] darwin: assume CFRunLoopStop() isn't thread-safe Use signaling mechanism for loop termination, because CFRunLoopStop() is most likely not a thread-safe function and invoking it from other thread may sometimes result in a "dead-lock". fix #799 --- src/unix/darwin.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/unix/darwin.c b/src/unix/darwin.c index cfbfed23..54351395 100644 --- a/src/unix/darwin.c +++ b/src/unix/darwin.c @@ -84,9 +84,8 @@ void uv__platform_loop_delete(uv_loop_t* loop) { uv__cf_loop_signal_t* s; assert(loop->cf_loop != NULL); - CFRunLoopStop(loop->cf_loop); + uv__cf_loop_signal(loop, NULL, NULL); uv_thread_join(&loop->cf_thread); - loop->cf_loop = NULL; uv_sem_destroy(&loop->cf_sem); uv_mutex_destroy(&loop->cf_mutex); @@ -145,7 +144,12 @@ void uv__cf_loop_cb(void* arg) { item = ngx_queue_head(&split_head); s = ngx_queue_data(item, uv__cf_loop_signal_t, member); - s->cb(s->arg); + + /* This was a termination signal */ + if (s->cb == NULL) + CFRunLoopStop(loop->cf_loop); + else + s->cb(s->arg); ngx_queue_remove(item); free(s); From db7dc6899d9badcfb99016ba87da2a66eae86dad Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Sat, 18 May 2013 20:45:36 +0200 Subject: [PATCH 3/3] win: fix UV_EALREADY incorrectly set UV_EALREADY itself is already a libuv error, it should be set with uv__set_artifical_error and not with uv__set_sys_error. Closes #802 --- src/win/stream.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/win/stream.c b/src/win/stream.c index 097f3497..edc5407c 100644 --- a/src/win/stream.c +++ b/src/win/stream.c @@ -56,7 +56,7 @@ int uv_accept(uv_stream_t* server, uv_stream_t* client) { int uv_read_start(uv_stream_t* handle, uv_alloc_cb alloc_cb, uv_read_cb read_cb) { if (handle->flags & UV_HANDLE_READING) { - uv__set_sys_error(handle->loop, UV_EALREADY); + uv__set_artificial_error(handle->loop, UV_EALREADY); return -1; } @@ -82,7 +82,7 @@ int uv_read_start(uv_stream_t* handle, uv_alloc_cb alloc_cb, int uv_read2_start(uv_stream_t* handle, uv_alloc_cb alloc_cb, uv_read2_cb read_cb) { if (handle->flags & UV_HANDLE_READING) { - uv__set_sys_error(handle->loop, UV_EALREADY); + uv__set_artificial_error(handle->loop, UV_EALREADY); return -1; }