[tor-commits] [tor/master] Avoid breaking the event loop prematurely.

[nickm at torproject.org]

commit ca7a2ecc512145589964577f2f2ff6133ab146f5
Author: Alexander Færøy <ahf at torproject.org>
Date:   Tue Dec 18 15:18:22 2018 +0100

    Avoid breaking the event loop prematurely.
    
    This patch makes sure that we terminate the event loop from the event
    loop timer instead of directly in the process' exit handler. This allows
    us to run the event loop an additional time to ensure that the SleepEx()
    call on Windows is called and the data from stdout/stderr is delivered
    to us.
    
    Additionally we ensure that we don't try to read or write data from a
    Unix process that have been terminated in the main loop, since its file
    descriptors are closed at that time.
    
    See: https://bugs.torproject.org/28179
---
 src/lib/process/process_unix.c | 21 +++++++++++----------
 src/test/test_process_slow.c   | 42 ++++++++++++++++++++++++++----------------
 2 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/src/lib/process/process_unix.c b/src/lib/process/process_unix.c
index 4fc1b7a51..57ca69a76 100644
--- a/src/lib/process/process_unix.c
+++ b/src/lib/process/process_unix.c
@@ -112,16 +112,6 @@ process_unix_free_(process_unix_t *unix_process)
   /* FIXME(ahf): Refactor waitpid code? */
   unix_process->waitpid = NULL;
 
-  /* Cleanup our events. */
-  if (! unix_process->stdout_handle.reached_eof)
-    process_unix_stop_reading(&unix_process->stdout_handle);
-
-  if (! unix_process->stderr_handle.reached_eof)
-    process_unix_stop_reading(&unix_process->stderr_handle);
-
-  if (unix_process->stdin_handle.is_writing)
-    process_unix_stop_writing(&unix_process->stdin_handle);
-
   /* Close all our file descriptors. */
   process_unix_close_file_descriptors(unix_process);
 
@@ -668,6 +658,17 @@ process_unix_close_file_descriptors(process_unix_t *unix_process)
   int ret;
   bool success = true;
 
+  /* Stop reading and writing before we close() our
+   * file descriptors. */
+  if (! unix_process->stdout_handle.reached_eof)
+    process_unix_stop_reading(&unix_process->stdout_handle);
+
+  if (! unix_process->stderr_handle.reached_eof)
+    process_unix_stop_reading(&unix_process->stderr_handle);
+
+  if (unix_process->stdin_handle.is_writing)
+    process_unix_stop_writing(&unix_process->stdin_handle);
+
   if (unix_process->stdin_handle.fd != -1) {
     ret = close(unix_process->stdin_handle.fd);
     if (ret == -1) {
diff --git a/src/test/test_process_slow.c b/src/test/test_process_slow.c
index cc7e8c5a9..a1f99bff0 100644
--- a/src/test/test_process_slow.c
+++ b/src/test/test_process_slow.c
@@ -36,6 +36,7 @@ struct process_data_t {
   smartlist_t *stderr_data;
   smartlist_t *stdin_data;
   process_exit_code_t exit_code;
+  bool did_exit;
 };
 
 typedef struct process_data_t process_data_t;
@@ -97,13 +98,17 @@ process_stderr_callback(process_t *process, const char *data, size_t size)
 static bool
 process_exit_callback(process_t *process, process_exit_code_t exit_code)
 {
+  process_status_t status;
+
   tt_ptr_op(process, OP_NE, NULL);
 
   process_data_t *process_data = process_get_data(process);
   process_data->exit_code = exit_code;
+  process_data->did_exit = true;
 
-  /* Our process died. Let's check the values it returned. */
-  tor_shutdown_event_loop_and_exit(0);
+  /* Check if our process is still running? */
+  status = process_get_status(process);
+  tt_int_op(status, OP_EQ, PROCESS_STATUS_NOT_RUNNING);
 
  done:
   /* Do not free up our process_t. */
@@ -137,9 +142,16 @@ main_loop_timeout_cb(periodic_timer_t *timer, void *data)
 {
   /* Sanity check. */
   tt_ptr_op(timer, OP_EQ, main_loop_timeout_timer);
-  tt_ptr_op(data, OP_EQ, NULL);
+  tt_ptr_op(data, OP_NE, NULL);
+
+  /* Our process data. */
+  process_data_t *process_data = data;
+
+  /* Our process did exit. */
+  if (process_data->did_exit)
+    tor_shutdown_event_loop_and_exit(0);
 
-  /* Have we been called 10 times we exit. */
+  /* Have we been called 10 times we exit the main loop. */
   timer_tick_count++;
 
   tt_int_op(timer_tick_count, OP_LT, 10);
@@ -156,7 +168,7 @@ main_loop_timeout_cb(periodic_timer_t *timer, void *data)
 }
 
 static void
-run_main_loop(void)
+run_main_loop(process_data_t *process_data)
 {
   int ret;
 
@@ -167,7 +179,7 @@ run_main_loop(void)
   main_loop_timeout_timer = periodic_timer_new(tor_libevent_get_base(),
                                                &interval,
                                                main_loop_timeout_cb,
-                                               NULL);
+                                               process_data);
 
   /* Run our main loop. */
   ret = run_main_loop_until_done();
@@ -225,11 +237,7 @@ test_callbacks(void *arg)
   process_printf(process, " lines?\r\n");
 
   /* Start our main loop. */
-  run_main_loop();
-
-  /* Check if our process is still running? */
-  status = process_get_status(process);
-  tt_int_op(status, OP_EQ, PROCESS_STATUS_NOT_RUNNING);
+  run_main_loop(process_data);
 
   /* We returned. Let's see what our event loop said. */
   tt_int_op(smartlist_len(process_data->stdout_data), OP_EQ, 12);
@@ -310,14 +318,16 @@ test_callbacks_terminate(void *arg)
   tt_int_op(status, OP_EQ, PROCESS_STATUS_RUNNING);
 
   /* Zap our process. */
-  process_terminate(process);
+  bool success;
+
+  success = process_terminate(process);
+  tt_assert(success);
 
   /* Start our main loop. */
-  run_main_loop();
+  run_main_loop(process_data);
 
-  /* Check if our process is still running? */
-  status = process_get_status(process);
-  tt_int_op(status, OP_EQ, PROCESS_STATUS_NOT_RUNNING);
+  /* Check if we did exit. */
+  tt_assert(process_data->did_exit);
 
  done:
   process_data_free(process_data);