From 9915b8f0bc3e6a1b6c8413fbb87c41137ea89104 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Thu, 17 Oct 2019 16:39:05 +0200 Subject: [PATCH 1/3] Handle errors from execve() in the Unix process backend more gracefully. This patch removes a call to tor_assert_unreached() after execve() failed. This assertion leads to the child process emitting a stack trace on its standard output, which makes the error harder for the user to demystify, since they think it is an internal error in Tor instead of "just" being a "no such file or directory" error. The process will now instead output "Error from child process: X" where X is the stringified version of the errno value. See: https://bugs.torproject.org/31810 --- changes/bug31810 | 4 ++++ src/lib/process/process_unix.c | 9 +-------- 2 files changed, 5 insertions(+), 8 deletions(-) create mode 100644 changes/bug31810 diff --git a/changes/bug31810 b/changes/bug31810 new file mode 100644 index 0000000000..628d12f09b --- /dev/null +++ b/changes/bug31810 @@ -0,0 +1,4 @@ + o Minor bugfixes (process management): + - Remove assertion in the Unix process backend. This assertion would trigger + when a new process is spawned where the executable is not found leading to + a stack trace from the child process. Fixes bug 31810; bugfix on 0.4.0.1-alpha. diff --git a/src/lib/process/process_unix.c b/src/lib/process/process_unix.c index 790ab897e9..b102afff44 100644 --- a/src/lib/process/process_unix.c +++ b/src/lib/process/process_unix.c @@ -253,22 +253,15 @@ process_unix_exec(process_t *process) process_environment_t *env = process_get_environment(process); /* Call the requested program. */ - retval = execve(argv[0], argv, env->unixoid_environment_block); + execve(argv[0], argv, env->unixoid_environment_block); /* If we made it here it is because execve failed :-( */ - if (-1 == retval) - fprintf(stderr, "Call to execve() failed: %s", strerror(errno)); - tor_free(argv); process_environment_free(env); - tor_assert_unreached(); - error: - /* LCOV_EXCL_START */ fprintf(stderr, "Error from child process: %s", strerror(errno)); _exit(1); - /* LCOV_EXCL_STOP */ } /* We are in the parent process. */ From cbed6ff8430ee7f7d1002965ebf784628735c234 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Thu, 17 Oct 2019 16:47:04 +0200 Subject: [PATCH 2/3] Ensure that the exit callback is called if CreateProcessA() fails on Windows. This patch fixes an issue where the exit handler is not called for the given process_t in case CreateProcessA() fails. This could, for example, happen if the user tries to execute a binary that does not exist. See: https://bugs.torproject.org/31810 --- src/lib/process/process_win32.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/lib/process/process_win32.c b/src/lib/process/process_win32.c index ddbe76bfd9..1b1de6ad55 100644 --- a/src/lib/process/process_win32.c +++ b/src/lib/process/process_win32.c @@ -234,6 +234,24 @@ process_win32_exec(process_t *process) CloseHandle(stdin_pipe_read); CloseHandle(stdin_pipe_write); + /* In the Unix backend, we do not get an error in the Tor process when a + * child process fails to spawn its target executable since we need to + * first do the fork() call in the Tor process and then the child process + * is responsible for doing the call to execve(). + * + * This means that the user of the process_exec() API must check for + * whether it returns PROCESS_STATUS_ERROR, which will rarely happen on + * Unix, but will happen for error cases on Windows where it does not + * happen on Unix. For example: when the target executable does not exist + * on the file system. + * + * To have somewhat feature compatibility between the Unix and the Windows + * backend, we here notify the process_t owner that the process have exited + * (even though it never managed to run) to ensure that the exit callback + * is executed. + */ + process_notify_event_exit(process, 0); + return PROCESS_STATUS_ERROR; } From ee8db8a2eb7a5d345d16d02bade593fa6db2877e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Thu, 17 Oct 2019 16:56:21 +0200 Subject: [PATCH 3/3] Add test to check if the exit callback is called in process_t upon process_exec() failures. This patch adds a test to check for whether the exit callback is always called when process_exec() fails, both on Windows and Unix. See: https://bugs.torproject.org/31810 --- src/test/test_process_slow.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/test/test_process_slow.c b/src/test/test_process_slow.c index 1322d7b833..d269a068ec 100644 --- a/src/test/test_process_slow.c +++ b/src/test/test_process_slow.c @@ -328,8 +328,38 @@ test_callbacks_terminate(void *arg) process_free(process); } +static void +test_nonexistent_executable(void *arg) +{ + (void)arg; + + /* Process callback data. */ + process_data_t *process_data = process_data_new(); + + /* Setup our process. */ + process_t *process = process_new("binary-does-not-exist"); + process_set_data(process, process_data); + process_set_exit_callback(process, process_exit_callback); + + /* Run our process. */ + process_exec(process); + + /* Start our main loop. */ + run_main_loop(process_data); + + /* Ensure that the exit callback was actually called even though the binary + * did not exist. + */ + tt_assert(process_data->did_exit); + + done: + process_data_free(process_data); + process_free(process); +} + struct testcase_t slow_process_tests[] = { { "callbacks", test_callbacks, 0, NULL, NULL }, { "callbacks_terminate", test_callbacks_terminate, 0, NULL, NULL }, + { "nonexistent_executable", test_nonexistent_executable, 0, NULL, NULL }, END_OF_TESTCASES };