Check return values from fcntl and setsockopt

(Based on a patch from flupzor; bug #8206)

diff --git a/changes/bug6206 b/changes/bug6206
new file mode 100644 (file)
index 0000000..61a16d2
--- /dev/null
+++ b/changes/bug6206
@@ -0,0 +1,6 @@
+  o Minor bugfixes:
+    - Always check the return values of functions fcntl() and
+      setsockopt(). We don't believe these are ever actually failing in
+      practice, but better safe than sorry. Also, checking these return
+      values should please some analysis tools (like Coverity). Patch
+      from 'flupzor'. Fix for bug 8206; bugfix on all versions of Tor.

diff --git a/src/common/compat.c b/src/common/compat.c
index d7ce89479a4b70104587c6b5235b7b5e327d5a4e..25df9a960c20af60a1d4237be5ff0c74b898f290 100644 (file)
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -137,8 +137,13 @@ tor_open_cloexec(const char *path, int flags, unsigned mode)
 
   fd = open(path, flags, mode);
 #ifdef FD_CLOEXEC
-  if (fd >= 0)
-        fcntl(fd, F_SETFD, FD_CLOEXEC);
+  if (fd >= 0) {
+    if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) {
+      log_warn(LD_FS,"Couldn't set FD_CLOEXEC: %s", strerror(errno));
+      close(fd);
+      return -1;
+    }
+  }
 #endif
   return fd;
 }
@@ -150,8 +155,13 @@ tor_fopen_cloexec(const char *path, const char *mode)
 {
   FILE *result = fopen(path, mode);
 #ifdef FD_CLOEXEC
-  if (result != NULL)
-    fcntl(fileno(result), F_SETFD, FD_CLOEXEC);
+  if (result != NULL) {
+    if (fcntl(fileno(result), F_SETFD, FD_CLOEXEC) == -1) {
+      log_warn(LD_FS,"Couldn't set FD_CLOEXEC: %s", strerror(errno));
+      fclose(result);
+      return NULL;
+    }
+  }
 #endif
   return result;
 }
@@ -1024,7 +1034,15 @@ tor_open_socket(int domain, int type, int protocol)
     return s;
 
 #if defined(FD_CLOEXEC)
-  fcntl(s, F_SETFD, FD_CLOEXEC);
+  if (fcntl(s, F_SETFD, FD_CLOEXEC) == -1) {
+    log_warn(LD_FS,"Couldn't set FD_CLOEXEC: %s", strerror(errno));
+#if defined(_WIN32)
+    closesocket(s);
+#else
+    close(s);
+#endif
+    return -1;
+  }
 #endif
 
   goto socket_ok; /* So that socket_ok will not be unused. */
@@ -1059,7 +1077,11 @@ tor_accept_socket(tor_socket_t sockfd, struct sockaddr *addr, socklen_t *len)
     return s;
 
 #if defined(FD_CLOEXEC)
-  fcntl(s, F_SETFD, FD_CLOEXEC);
+  if (fcntl(s, F_SETFD, FD_CLOEXEC) == -1) {
+    log_warn(LD_NET, "Couldn't set FD_CLOEXEC: %s", strerror(errno));
+    close(s);
+    return TOR_INVALID_SOCKET;
+  }
 #endif
 
   goto socket_ok; /* So that socket_ok will not be unused. */
@@ -1083,17 +1105,31 @@ get_n_open_sockets(void)
   return n;
 }
 
-/** Turn <b>socket</b> into a nonblocking socket.
+/** Turn <b>socket</b> into a nonblocking socket. Return 0 on success, -1
+ * on failure.
  */
-void
+int
 set_socket_nonblocking(tor_socket_t socket)
 {
 #if defined(_WIN32)
   unsigned long nonblocking = 1;
   ioctlsocket(socket, FIONBIO, (unsigned long*) &nonblocking);
 #else
-  fcntl(socket, F_SETFL, O_NONBLOCK);
+  int flags;
+
+  flags = fcntl(socket, F_GETFL, 0);
+  if (flags == -1) {
+    log_warn(LD_NET, "Couldn't get file status flags: %s", strerror(errno));
+    return -1;
+  }
+  flags |= O_NONBLOCK;
+  if (fcntl(socket, F_SETFL, flags) == -1) {
+    log_warn(LD_NET, "Couldn't set file status flags: %s", strerror(errno));
+    return -1;
+  }
 #endif
+
+  return 0;
 }
 
 /**

diff --git a/src/common/compat.h b/src/common/compat.h
index f9eb4ba0be0b6688094414515c1158b91534c051..f0a34aae4195423c911914572f455a78d711bcf6 100644 (file)
--- a/src/common/compat.h
+++ b/src/common/compat.h
@@ -518,7 +518,7 @@ int tor_inet_aton(const char *cp, struct in_addr *addr) ATTR_NONNULL((1,2));
 const char *tor_inet_ntop(int af, const void *src, char *dst, size_t len);
 int tor_inet_pton(int af, const char *src, void *dst);
 int tor_lookup_hostname(const char *name, uint32_t *addr) ATTR_NONNULL((1,2));
-void set_socket_nonblocking(tor_socket_t socket);
+int set_socket_nonblocking(tor_socket_t socket);
 int tor_socketpair(int family, int type, int protocol, tor_socket_t fd[2]);
 int network_init(void);
 

diff --git a/src/ext/eventdns.c b/src/ext/eventdns.c
index 3ee9f724597bc295fd2d75b95d817efd906d177b..0dd7629a19ba04f12e3a1ef92b59ae6f607bff4b 100644 (file)
--- a/src/ext/eventdns.c
+++ b/src/ext/eventdns.c
@@ -2275,6 +2275,7 @@ _evdns_nameserver_add_impl(const struct sockaddr *address,
 
        const struct nameserver *server = server_head, *const started_at = server_head;
        struct nameserver *ns;
+       int flags;
 
        int err = 0;
        if (server) {
@@ -2306,7 +2307,12 @@ _evdns_nameserver_add_impl(const struct sockaddr *address,
                ioctlsocket(ns->socket, FIONBIO, &nonblocking);
        }
 #else
-       fcntl(ns->socket, F_SETFL, O_NONBLOCK);
+       if (fcntl(ns->socket, F_SETFL, O_NONBLOCK) == -1) {
+               evdns_log(EVDNS_LOG_WARN, "Error %s (%d) while settings file status flags.",
+                                 tor_socket_strerror(errno), errno);
+               err = 2;
+               goto out2;
+       }
 #endif
 
        if (global_bind_addr_is_set &&

diff --git a/src/or/connection.c b/src/or/connection.c
index c659e65fe5b550e6b2fface4b6de2fb9c9b3ced2..622eadcff949eeb1469e94c7d2f77efc46286651 100644 (file)
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -918,8 +918,11 @@ make_socket_reuseable(tor_socket_t sock)
    * right after somebody else has let it go. But REUSEADDR on win32
    * means you can bind to the port _even when somebody else
    * already has it bound_. So, don't do that on Win32. */
-  setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, (void*) &one,
-             (socklen_t)sizeof(one));
+  if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, (void*) &one,
+             (socklen_t)sizeof(one)) == -1) {
+    log_warn(LD_NET, "Error setting SO_REUSEADDR flag: %s",
+             tor_socket_strerror(errno));
+  }
 #endif
 }
 
@@ -1102,7 +1105,10 @@ connection_listener_new(const struct sockaddr *listensockaddr,
       tor_assert(0);
   }
 
-  set_socket_nonblocking(s);
+  if (set_socket_nonblocking(s) == -1) {
+    tor_close_socket(s);
+    goto err;
+  }
 
   lis_conn = listener_connection_new(type, listensockaddr->sa_family);
   conn = TO_CONN(lis_conn);
@@ -1265,7 +1271,10 @@ connection_handle_listener_read(connection_t *conn, int new_type)
             (int)news,(int)conn->s);
 
   make_socket_reuseable(news);
-  set_socket_nonblocking(news);
+  if (set_socket_nonblocking(news) == -1) {
+    tor_close_socket(news);
+    return 0;
+  }
 
   if (options->ConstrainedSockets)
     set_constrained_socket_buffers(news, (int)options->ConstrainedSockSize);
@@ -1494,7 +1503,11 @@ connection_connect(connection_t *conn, const char *address,
     }
   }
 
-  set_socket_nonblocking(s);
+  if (set_socket_nonblocking(s) == -1) {
+    *socket_error = tor_socket_errno(s);
+    tor_close_socket(s);
+    return -1;
+  }
 
   if (options->ConstrainedSockets)
     set_constrained_socket_buffers(s, (int)options->ConstrainedSockSize);

diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c
index 38c6613f084297c7cbdeb24d9041d47645d86a56..61f9faa394d87a8951e74507a528b8e103786a1c 100644 (file)
--- a/src/or/cpuworker.c
+++ b/src/or/cpuworker.c
@@ -535,13 +535,16 @@ spawn_cpuworker(void)
 
   conn = connection_new(CONN_TYPE_CPUWORKER, AF_UNIX);
 
-  set_socket_nonblocking(fd);
-
   /* set up conn so it's got all the data we need to remember */
   conn->s = fd;
   conn->address = tor_strdup("localhost");
   tor_addr_make_unspec(&conn->addr);
 
+  if (set_socket_nonblocking(fd) == -1) {
+    connection_free(conn); /* this closes fd */
+    return -1;
+  }
+
   if (connection_add(conn) < 0) { /* no space, forget it */
     log_warn(LD_NET,"connection_add for cpuworker failed. Giving up.");
     connection_free(conn); /* this closes fd */