Remove buffered I/O stream usage in process_handle_t.

This patch removes the buffered I/O stream usage in process_handle_t and
its related utility functions. This simplifies the code and avoids racy
code where we used buffered I/O on non-blocking file descriptors.

See: https://bugs.torproject.org/21654

1 files changed, 33 insertions, 67 deletions

diff --git a/src/common/util.c b/src/common/util.c
index e2ad5ffa48..1fd3b16d72 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -4175,10 +4175,10 @@ tor_process_get_stdout_pipe(process_handle_t *process_handle)
 }
 #else
 /* DOCDOC tor_process_get_stdout_pipe */
-FILE *
+int
 tor_process_get_stdout_pipe(process_handle_t *process_handle)
 {
-  return process_handle->stdout_handle;
+  return process_handle->stdout_pipe;
 }
 #endif
 
@@ -4609,10 +4609,6 @@ tor_spawn_background(const char *const filename, const char **argv,
     log_warn(LD_GENERAL, "Failed to set stderror/stdout/stdin pipes "
              "nonblocking in parent process: %s", strerror(errno));
   }
-  /* Open the buffered IO streams */
-  process_handle->stdout_handle = fdopen(process_handle->stdout_pipe, "r");
-  process_handle->stderr_handle = fdopen(process_handle->stderr_pipe, "r");
-  process_handle->stdin_handle = fdopen(process_handle->stdin_pipe, "r");
 
   *process_handle_out = process_handle;
   return process_handle->status;
@@ -4659,14 +4655,9 @@ tor_process_handle_destroy,(process_handle_t *process_handle,
   if (process_handle->stdin_pipe)
     CloseHandle(process_handle->stdin_pipe);
 #else
-  if (process_handle->stdout_handle)
-    fclose(process_handle->stdout_handle);
-
-  if (process_handle->stderr_handle)
-    fclose(process_handle->stderr_handle);
-
-  if (process_handle->stdin_handle)
-    fclose(process_handle->stdin_handle);
+  close(process_handle->stdout_pipe);
+  close(process_handle->stderr_pipe);
+  close(process_handle->stdin_pipe);
 
   clear_waitpid_callback(process_handle->waitpid_cb);
 #endif
@@ -4998,14 +4989,14 @@ tor_read_all_handle(HANDLE h, char *buf, size_t count,
   return (ssize_t)numread;
 }
 #else
-/** Read from a handle <b>h</b> into <b>buf</b>, up to <b>count</b> bytes.  If
+/** Read from a handle <b>fd</b> into <b>buf</b>, up to <b>count</b> bytes.  If
  * <b>process</b> is NULL, the function will return immediately if there is
  * nothing more to read. Otherwise data will be read until end of file, or
  * <b>count</b> bytes are read.  Returns the number of bytes read, or -1 on
  * error. Sets <b>eof</b> to true if <b>eof</b> is not NULL and the end of the
  * file has been reached. */
 ssize_t
-tor_read_all_handle(FILE *h, char *buf, size_t count,
+tor_read_all_handle(int fd, char *buf, size_t count,
                     const process_handle_t *process,
                     int *eof)
 {
@@ -5019,7 +5010,7 @@ tor_read_all_handle(FILE *h, char *buf, size_t count,
     return -1;
 
   while (numread != count) {
-    result = read(fileno(h), buf+numread, count-numread);
+    result = read(fd, buf+numread, count-numread);
 
     if (result == 0) {
       log_debug(LD_GENERAL, "read() reached end of file");
@@ -5053,7 +5044,7 @@ tor_read_all_from_process_stdout(const process_handle_t *process_handle,
   return tor_read_all_handle(process_handle->stdout_pipe, buf, count,
                              process_handle);
 #else
-  return tor_read_all_handle(process_handle->stdout_handle, buf, count,
+  return tor_read_all_handle(process_handle->stdout_pipe, buf, count,
                              process_handle, NULL);
 #endif
 }
@@ -5067,7 +5058,7 @@ tor_read_all_from_process_stderr(const process_handle_t *process_handle,
   return tor_read_all_handle(process_handle->stderr_pipe, buf, count,
                              process_handle);
 #else
-  return tor_read_all_handle(process_handle->stderr_handle, buf, count,
+  return tor_read_all_handle(process_handle->stderr_pipe, buf, count,
                              process_handle, NULL);
 #endif
 }
@@ -5261,11 +5252,10 @@ log_from_handle(HANDLE *pipe, int severity)
 #else
 
 /** Return a smartlist containing lines outputted from
- *  <b>handle</b>. Return NULL on error, and set
+ *  <b>fd</b>. Return NULL on error, and set
  *  <b>stream_status_out</b> appropriately. */
 MOCK_IMPL(smartlist_t *,
-tor_get_lines_from_handle, (FILE *handle,
-                            enum stream_status *stream_status_out))
+tor_get_lines_from_handle, (int fd, enum stream_status *stream_status_out))
 {
   enum stream_status stream_status;
   char stdout_buf[400];
@@ -5274,7 +5264,7 @@ tor_get_lines_from_handle, (FILE *handle,
   while (1) {
     memset(stdout_buf, 0, sizeof(stdout_buf));
 
-    stream_status = get_string_from_pipe(handle,
+    stream_status = get_string_from_pipe(fd,
                                          stdout_buf, sizeof(stdout_buf) - 1);
     if (stream_status != IO_STREAM_OKAY)
       goto done;
@@ -5288,20 +5278,20 @@ tor_get_lines_from_handle, (FILE *handle,
   return lines;
 }
 
-/** Read from stream, and send lines to log at the specified log level.
+/** Read from fd, and send lines to log at the specified log level.
  * Returns 1 if stream is closed normally, -1 if there is a error reading, and
  * 0 otherwise. Handles lines from tor-fw-helper and
  * tor_spawn_background() specially.
  */
 static int
-log_from_pipe(FILE *stream, int severity, const char *executable,
+log_from_pipe(int fd, int severity, const char *executable,
               int *child_status)
 {
   char buf[256];
   enum stream_status r;
 
   for (;;) {
-    r = get_string_from_pipe(stream, buf, sizeof(buf) - 1);
+    r = get_string_from_pipe(fd, buf, sizeof(buf) - 1);
 
     if (r == IO_STREAM_CLOSED) {
       return 1;
@@ -5326,7 +5316,7 @@ log_from_pipe(FILE *stream, int severity, const char *executable,
 }
 #endif
 
-/** Reads from <b>stream</b> and stores input in <b>buf_out</b> making
+/** Reads from <b>fd</b> and stores input in <b>buf_out</b> making
  *  sure it's below <b>count</b> bytes.
  *  If the string has a trailing newline, we strip it off.
  *
@@ -5342,52 +5332,28 @@ log_from_pipe(FILE *stream, int severity, const char *executable,
  * IO_STREAM_OKAY: If everything went okay and we got a string
  *  in <b>buf_out</b>. */
 enum stream_status
-get_string_from_pipe(FILE *stream, char *buf_out, size_t count)
+get_string_from_pipe(int fd, char *buf_out, size_t count)
 {
-  char *retval;
-  size_t len;
+  ssize_t ret;
 
   tor_assert(count <= INT_MAX);
 
-  retval = tor_fgets(buf_out, (int)count, stream);
-
-  if (!retval) {
-    if (feof(stream)) {
-      /* Program has closed stream (probably it exited) */
-      /* TODO: check error */
-      return IO_STREAM_CLOSED;
-    } else {
-      if (EAGAIN == errno) {
-        /* Nothing more to read, try again next time */
-        return IO_STREAM_EAGAIN;
-      } else {
-        /* There was a problem, abandon this child process */
-        return IO_STREAM_TERM;
-      }
-    }
-  } else {
-    len = strlen(buf_out);
-    if (len == 0) {
-      /* this probably means we got a NUL at the start of the string. */
-      return IO_STREAM_EAGAIN;
-    }
+  ret = read(fd, buf_out, count);
 
-    if (buf_out[len - 1] == '\n') {
-      /* Remove the trailing newline */
-      buf_out[len - 1] = '\0';
-    } else {
-      /* No newline; check whether we overflowed the buffer */
-      if (!feof(stream))
-        log_info(LD_GENERAL,
-                 "Line from stream was truncated: %s", buf_out);
-      /* TODO: What to do with this error? */
-    }
+  if (ret == 0)
+    return IO_STREAM_CLOSED;
+  else if (ret < 0 && errno == EAGAIN)
+    return IO_STREAM_EAGAIN;
+  else if (ret < 0)
+    return IO_STREAM_TERM;
 
-    return IO_STREAM_OKAY;
-  }
+  if (buf_out[ret - 1] == '\n') {
+    /* Remove the trailing newline */
+    buf_out[ret - 1] = '\0';
+  } else
+    buf_out[ret] = '\0';
 
-  /* We should never get here */
-  return IO_STREAM_TERM;
+  return IO_STREAM_OKAY;
 }
 
 /** Parse a <b>line</b> from tor-fw-helper and issue an appropriate
@@ -5624,7 +5590,7 @@ tor_check_port_forwarding(const char *filename,
 #ifdef _WIN32
     stderr_status = log_from_handle(child_handle->stderr_pipe, LOG_INFO);
 #else
-    stderr_status = log_from_pipe(child_handle->stderr_handle,
+    stderr_status = log_from_pipe(child_handle->stderr_pipe,
                                   LOG_INFO, filename, &retval);
 #endif
     if (handle_fw_helper_output(filename, child_handle) < 0) {