[tor-commits] [tor/master] Fix double-closing a stdio stream

[nickm at torproject.org]

commit cfa9ee5fe708539965f46a31b5d2abe4950179af
Author: Steven Murdoch <Steven.Murdoch at cl.cam.ac.uk>
Date:   Thu Sep 1 13:09:38 2011 +0100

    Fix double-closing a stdio stream
    
    After a stream reached eof, we fclose it, but then
    test_util_spawn_background_partial_read() reads from it again, which causes
    an error and thus another fclose(). Some platforms are fine with this, others
    (e.g. debian-sid-i386) trigger a double-free() error. The actual code used by
    Tor (log_from_pipe() and tor_check_port_forwarding()) handle this case
    correctly.
---
 src/common/util.c    |   18 +++++++++++++-----
 src/common/util.h    |    3 ++-
 src/test/test_util.c |   18 ++++++++++++------
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/common/util.c b/src/common/util.c
index 7cd9dd8..db6b00f 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -3583,14 +3583,19 @@ tor_read_all_handle(HANDLE h, char *buf, size_t count,
  * <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. */
+ * 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,
-                    const process_handle_t *process)
+                    const process_handle_t *process,
+                    int *eof)
 {
   size_t numread = 0;
   char *retval;
 
+  if (eof)
+    *eof = 0;
+
   if (count > SIZE_T_CEILING || count > SSIZE_T_MAX)
     return -1;
 
@@ -3599,7 +3604,10 @@ tor_read_all_handle(FILE *h, char *buf, size_t count,
     retval = fgets(buf+numread, (int)(count-numread), h);
     if (NULL == retval) {
       if (feof(h)) {
+        log_debug(LD_GENERAL, "fgets() reached end of file");
         fclose(h);
+        if (eof)
+          *eof = 1;
         break;
       } else {
         if (EAGAIN == errno) {
@@ -3620,7 +3628,7 @@ tor_read_all_handle(FILE *h, char *buf, size_t count,
     numread += strlen(retval);
   }
 
-  log_debug(LD_GENERAL, "fgets read %d bytes from handle", (int)numread);
+  log_debug(LD_GENERAL, "fgets() read %d bytes from handle", (int)numread);
   return (ssize_t)numread;
 }
 #endif
@@ -3635,7 +3643,7 @@ tor_read_all_from_process_stdout(const process_handle_t *process_handle,
                              process_handle);
 #else
   return tor_read_all_handle(process_handle->stdout_handle, buf, count,
-                             process_handle);
+                             process_handle, NULL);
 #endif
 }
 
@@ -3649,7 +3657,7 @@ tor_read_all_from_process_stderr(const process_handle_t *process_handle,
                              process_handle);
 #else
   return tor_read_all_handle(process_handle->stderr_handle, buf, count,
-                             process_handle);
+                             process_handle, NULL);
 #endif
 }
 
diff --git a/src/common/util.h b/src/common/util.h
index 6211267..c8cce39 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -391,7 +391,8 @@ 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,
-                            const process_handle_t *process);
+                            const process_handle_t *process,
+                            int *eof);
 #endif
 ssize_t tor_read_all_from_process_stdout(
     const process_handle_t *process_handle, char *buf, size_t count);
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 06c1be3..7faa0ae 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1484,7 +1484,7 @@ test_util_spawn_background_partial_read(void *ptr)
   const int expected_status = PROCESS_STATUS_RUNNING;
 
   int retval, exit_code;
-  ssize_t pos;
+  ssize_t pos = -1;
   process_handle_t process_handle;
   char stdout_buf[100], stderr_buf[100];
 #ifdef MS_WINDOWS
@@ -1499,6 +1499,7 @@ test_util_spawn_background_partial_read(void *ptr)
                                  "DONE\n",
                                  NULL };
   const char *expected_err = "ERR\n";
+  int eof = 0;
 #endif
   int expected_out_ctr;
   (void)ptr;
@@ -1517,8 +1518,10 @@ test_util_spawn_background_partial_read(void *ptr)
     pos = tor_read_all_handle(process_handle.stdout_pipe, stdout_buf,
                               sizeof(stdout_buf) - 1, NULL);
 #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,
-                              sizeof(stdout_buf) - 1, NULL);
+                              sizeof(stdout_buf) - 1, NULL, &eof);
 #endif
     log_info(LD_GENERAL, "tor_read_all_handle() returned %d", (int)pos);
 
@@ -1526,7 +1529,7 @@ test_util_spawn_background_partial_read(void *ptr)
     if (0 == pos)
       continue;
 
-    tt_int_op(pos, >=, 0);
+    tt_int_op(pos, >, 0);
     stdout_buf[pos] = '\0';
     tt_str_op(stdout_buf, ==, expected_out[expected_out_ctr]);
     tt_int_op(pos, ==, strlen(expected_out[expected_out_ctr]));
@@ -1539,10 +1542,13 @@ test_util_spawn_background_partial_read(void *ptr)
                             sizeof(stdout_buf) - 1,
                             &process_handle);
 #else
-  pos = tor_read_all_handle(process_handle.stdout_handle, stdout_buf,
-                            sizeof(stdout_buf) - 1,
-                            &process_handle);
+  if (!eof)
+    pos = tor_read_all_handle(process_handle.stdout_handle, stdout_buf,
+                              sizeof(stdout_buf) - 1,
+                              &process_handle, &eof);
+  tt_assert(eof)
 #endif
+  tt_int_op(pos, ==, 0);
 
   /* Check it terminated correctly */
   retval = tor_get_exit_code(process_handle, 1, &exit_code);