commit 63b67577d6df1080e0bca89d66a2e1550da6265d Author: Nick Mathewson nickm@torproject.org Date: Mon Mar 11 20:58:28 2013 -0400
Check return values from fcntl and setsockopt
(Based on a patch from flupzor; bug #8206) --- changes/bug6206 | 6 +++++ src/common/compat.c | 54 ++++++++++++++++++++++++++++++++++++++++++-------- src/common/compat.h | 2 +- src/ext/eventdns.c | 8 ++++++- src/or/connection.c | 23 +++++++++++++++++---- src/or/cpuworker.c | 7 ++++- 6 files changed, 82 insertions(+), 18 deletions(-)
diff --git a/changes/bug6206 b/changes/bug6206 new file mode 100644 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 d7ce894..25df9a9 100644 --- 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 f9eb4ba..f0a34aa 100644 --- 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 3ee9f72..0dd7629 100644 --- 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 c659e65..622eadc 100644 --- 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 38c6613..61f9faa 100644 --- 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 */