[tor-commits] [tor/master] Correctly handle fd-drain errors on windows workqueues

[nickm at torproject.org]

commit 44514058b96440eaa1b364b915860ae372207ca6
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed Mar 15 11:53:01 2017 -0400

    Correctly handle fd-drain errors on windows workqueues
    
    Windows doesn't let you check the socket error for a socket with
    WSAGetLastError() and getsockopt(SO_ERROR).  But
    getsockopt(SO_ERROR) clears the error on the socket, so you can't
    call it more than once per error.
    
    When we introduced recv_ni to help drain alert sockets, back in
    0.2.6.3-alpha, we had the failure path for recv_ni call getsockopt()
    twice, though: once to check for EINTR and one to check for EAGAIN.
    Of course, we never got the eagain, so we treated it as an error,
    and warned about: "No error".
    
    The fix here is to have these functions return -errno on failure.
    
    Fixes bug 21540; bugfix on 0.2.6.3-alpha.
---
 changes/bug21540            |  4 +++
 src/common/compat_threads.c | 68 ++++++++++++++++++++++++++++++---------------
 src/common/workqueue.c      |  5 ++--
 3 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/changes/bug21540 b/changes/bug21540
new file mode 100644
index 0000000..0cf684b
--- /dev/null
+++ b/changes/bug21540
@@ -0,0 +1,4 @@
+  o Minor bugfixes (windows, relay):
+    - Resolve "Failure from drain_fd: No error" warnings on Windows
+      relays. Fixes bug 21540; bugfix on 0.2.6.3-alpha.
+
diff --git a/src/common/compat_threads.c b/src/common/compat_threads.c
index f480906..a25d9a7 100644
--- a/src/common/compat_threads.c
+++ b/src/common/compat_threads.c
@@ -94,51 +94,73 @@ in_main_thread(void)
 }
 
 #if defined(HAVE_EVENTFD) || defined(HAVE_PIPE)
-/* As write(), but retry on EINTR */
+/* As write(), but retry on EINTR, and return the negative error code on
+ * error. */
 static int
 write_ni(int fd, const void *buf, size_t n)
 {
   int r;
  again:
   r = (int) write(fd, buf, n);
-  if (r < 0 && errno == EINTR)
-    goto again;
+  if (r < 0) {
+    if (errno == EINTR)
+      goto again;
+    else
+      return -errno;
+  }
   return r;
 }
-/* As read(), but retry on EINTR */
+/* As read(), but retry on EINTR, and return the negative error code on error.
+ */
 static int
 read_ni(int fd, void *buf, size_t n)
 {
   int r;
  again:
   r = (int) read(fd, buf, n);
-  if (r < 0 && errno == EINTR)
-    goto again;
+  if (r < 0) {
+    if (errno == EINTR)
+      goto again;
+    else
+      return -errno;
+  }
   return r;
 }
 #endif
 
-/** As send(), but retry on EINTR. */
+/** As send(), but retry on EINTR, and return the negative error code on
+ * error. */
 static int
 send_ni(int fd, const void *buf, size_t n, int flags)
 {
   int r;
  again:
   r = (int) send(fd, buf, n, flags);
-  if (r < 0 && ERRNO_IS_EINTR(tor_socket_errno(fd)))
-    goto again;
+  if (r < 0) {
+    int error = tor_socket_errno(fd);
+    if (ERRNO_IS_EINTR(error))
+      goto again;
+    else
+      return -error;
+  }
   return r;
 }
 
-/** As recv(), but retry on EINTR. */
+/** As recv(), but retry on EINTR, and return the negative error code on
+ * error. */
 static int
 recv_ni(int fd, void *buf, size_t n, int flags)
 {
   int r;
  again:
   r = (int) recv(fd, buf, n, flags);
-  if (r < 0 && ERRNO_IS_EINTR(tor_socket_errno(fd)))
-    goto again;
+  if (r < 0) {
+    int error = tor_socket_errno(fd);
+    if (ERRNO_IS_EINTR(error))
+      goto again;
+    else
+      return -error;
+  }
   return r;
 }
 
@@ -149,7 +171,7 @@ eventfd_alert(int fd)
 {
   uint64_t u = 1;
   int r = write_ni(fd, (void*)&u, sizeof(u));
-  if (r < 0 && errno != EAGAIN)
+  if (r < 0 && -r != EAGAIN)
     return -1;
   return 0;
 }
@@ -160,8 +182,8 @@ eventfd_drain(int fd)
 {
   uint64_t u = 0;
   int r = read_ni(fd, (void*)&u, sizeof(u));
-  if (r < 0 && errno != EAGAIN)
-    return -1;
+  if (r < 0 && -r != EAGAIN)
+    return r;
   return 0;
 }
 #endif
@@ -172,8 +194,8 @@ static int
 pipe_alert(int fd)
 {
   ssize_t r = write_ni(fd, "x", 1);
-  if (r < 0 && errno != EAGAIN)
-    return -1;
+  if (r < 0 && -r != EAGAIN)
+    return r;
   return 0;
 }
 
@@ -188,7 +210,7 @@ pipe_drain(int fd)
     r = read_ni(fd, buf, sizeof(buf));
   } while (r > 0);
   if (r < 0 && errno != EAGAIN)
-    return -1;
+    return -errno;
   /* A value of r = 0 means EOF on the fd so successfully drained. */
   return 0;
 }
@@ -200,13 +222,13 @@ static int
 sock_alert(tor_socket_t fd)
 {
   ssize_t r = send_ni(fd, "x", 1, 0);
-  if (r < 0 && !ERRNO_IS_EAGAIN(tor_socket_errno(fd)))
-    return -1;
+  if (r < 0 && !ERRNO_IS_EAGAIN(-r))
+    return r;
   return 0;
 }
 
 /** Drain all the input from a socket <b>fd</b>, and ignore it.  Return 0 on
- * success, -1 on error. */
+ * success, -errno on error. */
 static int
 sock_drain(tor_socket_t fd)
 {
@@ -215,8 +237,8 @@ sock_drain(tor_socket_t fd)
   do {
     r = recv_ni(fd, buf, sizeof(buf), 0);
   } while (r > 0);
-  if (r < 0 && !ERRNO_IS_EAGAIN(tor_socket_errno(fd)))
-    return -1;
+  if (r < 0 && !ERRNO_IS_EAGAIN(-r))
+    return r;
   /* A value of r = 0 means EOF on the fd so successfully drained. */
   return 0;
 }
diff --git a/src/common/workqueue.c b/src/common/workqueue.c
index e1fb663..ec6e257 100644
--- a/src/common/workqueue.c
+++ b/src/common/workqueue.c
@@ -510,12 +510,13 @@ replyqueue_get_socket(replyqueue_t *rq)
 void
 replyqueue_process(replyqueue_t *queue)
 {
-  if (queue->alert.drain_fn(queue->alert.read_fd) < 0) {
+  int r = queue->alert.drain_fn(queue->alert.read_fd);
+  if (r < 0) {
     //LCOV_EXCL_START
     static ratelim_t warn_limit = RATELIM_INIT(7200);
     log_fn_ratelim(&warn_limit, LOG_WARN, LD_GENERAL,
                  "Failure from drain_fd: %s",
-                 tor_socket_strerror(tor_socket_errno(queue->alert.read_fd)));
+                   tor_socket_strerror(-r));
     //LCOV_EXCL_STOP
   }