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

changes/bug21654

@@ -0,0 +1,4 @@
+  o Code simplifications and refactoring
+    - Use unbuffered I/O for utility functions around the process_handle_t
+      type. This fixes unit test failures reported on OpenBSD and FreeBSD.
+      Fixes bug 21654.

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) {

src/common/util.h

@@ -322,7 +322,7 @@ enum stream_status {
 
 const char *stream_status_to_string(enum stream_status stream_status);
 
-enum stream_status get_string_from_pipe(FILE *stream, char *buf, size_t count);
+enum stream_status get_string_from_pipe(int fd, char *buf, size_t count);
 
 MOCK_DECL(int,tor_unlink,(const char *pathname));
 
@@ -463,9 +463,6 @@ struct process_handle_t {
   int stdin_pipe;
   int stdout_pipe;
   int stderr_pipe;
-  FILE *stdin_handle;
-  FILE *stdout_handle;
-  FILE *stderr_handle;
   pid_t pid;
   /** If the process has not given us a SIGCHLD yet, this has the
    * waitpid_callback_t that gets invoked once it has. Otherwise this
@@ -488,7 +485,7 @@ int tor_split_lines(struct smartlist_t *sl, char *buf, int len);
 ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count,
                             const process_handle_t *process);
 #else
-ssize_t tor_read_all_handle(FILE *h, char *buf, size_t count,
+ssize_t tor_read_all_handle(int fd, char *buf, size_t count,
                             const process_handle_t *process,
                             int *eof);
 #endif
@@ -502,7 +499,7 @@ int tor_process_get_pid(process_handle_t *process_handle);
 #ifdef _WIN32
 HANDLE tor_process_get_stdout_pipe(process_handle_t *process_handle);
 #else
-FILE *tor_process_get_stdout_pipe(process_handle_t *process_handle);
+int tor_process_get_stdout_pipe(process_handle_t *process_handle);
 #endif
 
 #ifdef _WIN32
@@ -511,7 +508,7 @@ tor_get_lines_from_handle,(HANDLE *handle,
                            enum stream_status *stream_status));
 #else
 MOCK_DECL(struct smartlist_t *,
-tor_get_lines_from_handle,(FILE *handle,
+tor_get_lines_from_handle,(int fd,
                            enum stream_status *stream_status));
 #endif
 

src/test/test_pt.c

@@ -284,13 +284,13 @@ test_pt_get_extrainfo_string(void *arg)
 }
 
 #ifdef _WIN32
-#define STDIN_HANDLE HANDLE
+#define STDIN_HANDLE HANDLE*
 #else
-#define STDIN_HANDLE FILE
+#define STDIN_HANDLE int
 #endif
 
 static smartlist_t *
-tor_get_lines_from_handle_replacement(STDIN_HANDLE *handle,
+tor_get_lines_from_handle_replacement(STDIN_HANDLE handle,
                                       enum stream_status *stream_status_out)
 {
   static int times_called = 0;

src/test/test_util_slow.c

@@ -242,7 +242,7 @@ test_util_spawn_background_partial_read_impl(int exit_early)
 #else
     /* Check that we didn't read the end of file last time */
     tt_assert(!eof);
-    pos = tor_read_all_handle(process_handle->stdout_handle, stdout_buf,
+    pos = tor_read_all_handle(process_handle->stdout_pipe, stdout_buf,
                               sizeof(stdout_buf) - 1, NULL, &eof);
 #endif
     log_info(LD_GENERAL, "tor_read_all_handle() returned %d", (int)pos);
@@ -273,7 +273,7 @@ test_util_spawn_background_partial_read_impl(int exit_early)
 #else
   if (!eof) {
     /* We should have got all the data, but maybe not the EOF flag */
-    pos = tor_read_all_handle(process_handle->stdout_handle, stdout_buf,
+    pos = tor_read_all_handle(process_handle->stdout_pipe, stdout_buf,
                               sizeof(stdout_buf) - 1,
                               process_handle, &eof);
     tt_int_op(0,OP_EQ, pos);