From c36efb0c451b3cc55d6ebf634af451f9d0b542b9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 12 May 2009 16:17:32 -0400 Subject: [PATCH 1/5] Use a mutex to protect the count of open sockets. This matters because a cpuworker can close its socket when it finishes. Cpuworker typically runs in another thread, so without a lock here, we can have a race condition and get confused about how many sockets are open. Possible fix for bug 939. --- ChangeLog | 3 +++ src/common/compat.c | 52 ++++++++++++++++++++++++++++++++++++--------- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9359915c1e..c4608212ed 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,6 +5,9 @@ Changes in version 0.2.1.15??? - ????-??-?? Bugfix on 0.2.0.9-alpha. - Provide a more useful log message if bug 977 (related to buffer freelists) ever reappears, and do not crash right away. + - Protect the count of open sockets with a mutex, so we can't + corrupt it when two threads are closing or opening sockets at once. + Fix for bug 939. Bugfix on 0.2.0.1-alpha. Changes in version 0.2.1.14-rc - 2009-04-12 diff --git a/src/common/compat.c b/src/common/compat.c index 82957722c9..51794c762c 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -676,6 +676,23 @@ static int max_socket = -1; * eventdns and libevent.) */ static int n_sockets_open = 0; +/** Mutex to protect open_sockets, max_socket, and n_sockets_open. */ +static tor_mutex_t *socket_accounting_mutex = NULL; + +static INLINE void +socket_accounting_lock(void) +{ + if (PREDICT_UNLIKELY(!socket_accounting_mutex)) + socket_accounting_mutex = tor_mutex_new(); + tor_mutex_acquire(socket_accounting_mutex); +} + +static INLINE void +socket_accounting_unlock(void) +{ + tor_mutex_release(socket_accounting_mutex); +} + /** As close(), but guaranteed to work for sockets across platforms (including * Windows, where close()ing a socket doesn't work. Returns 0 on success, -1 * on failure. */ @@ -683,15 +700,7 @@ int tor_close_socket(int s) { int r = 0; -#ifdef DEBUG_SOCKET_COUNTING - if (s > max_socket || ! bitarray_is_set(open_sockets, s)) { - log_warn(LD_BUG, "Closing a socket (%d) that wasn't returned by tor_open_" - "socket(), or that was already closed or something.", s); - } else { - tor_assert(open_sockets && s <= max_socket); - bitarray_clear(open_sockets, s); - } -#endif + /* On Windows, you have to call close() on fds returned by open(), * and closesocket() on fds returned by socket(). On Unix, everything * gets close()'d. We abstract this difference by always using @@ -703,6 +712,17 @@ tor_close_socket(int s) #else r = close(s); #endif + + socket_accounting_lock(); +#ifdef DEBUG_SOCKET_COUNTING + if (s > max_socket || ! bitarray_is_set(open_sockets, s)) { + log_warn(LD_BUG, "Closing a socket (%d) that wasn't returned by tor_open_" + "socket(), or that was already closed or something.", s); + } else { + tor_assert(open_sockets && s <= max_socket); + bitarray_clear(open_sockets, s); + } +#endif if (r == 0) { --n_sockets_open; } else { @@ -717,9 +737,11 @@ tor_close_socket(int s) #endif r = -1; } + if (n_sockets_open < 0) log_warn(LD_BUG, "Our socket count is below zero: %d. Please submit a " "bug report.", n_sockets_open); + socket_accounting_unlock(); return r; } @@ -754,8 +776,10 @@ tor_open_socket(int domain, int type, int protocol) { int s = socket(domain, type, protocol); if (s >= 0) { + socket_accounting_lock(); ++n_sockets_open; mark_socket_open(s); + socket_accounting_unlock(); } return s; } @@ -766,8 +790,10 @@ tor_accept_socket(int sockfd, struct sockaddr *addr, socklen_t *len) { int s = accept(sockfd, addr, len); if (s >= 0) { + socket_accounting_lock(); ++n_sockets_open; mark_socket_open(s); + socket_accounting_unlock(); } return s; } @@ -776,7 +802,11 @@ tor_accept_socket(int sockfd, struct sockaddr *addr, socklen_t *len) int get_n_open_sockets(void) { - return n_sockets_open; + int n; + socket_accounting_lock(); + n = n_sockets_open; + socket_accounting_unlock(); + return n; } /** Turn socket into a nonblocking socket. @@ -817,6 +847,7 @@ tor_socketpair(int family, int type, int protocol, int fd[2]) int r; r = socketpair(family, type, protocol, fd); if (r == 0) { + socket_accounting_lock(); if (fd[0] >= 0) { ++n_sockets_open; mark_socket_open(fd[0]); @@ -825,6 +856,7 @@ tor_socketpair(int family, int type, int protocol, int fd[2]) ++n_sockets_open; mark_socket_open(fd[1]); } + socket_accounting_unlock(); } return r < 0 ? -errno : r; #else From fc091e8a96f2e2db9516f4046bec63f1e17a8f71 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 12 May 2009 15:35:53 -0400 Subject: [PATCH 2/5] When our bridge status changes, forget old geoip data. This fixes bug 932, where all of our usage totals for users when we were a regular server would get included in the extrainfo document. --- src/or/config.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/or/config.c b/src/or/config.c index 71dd391664..3d757c1bbb 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1329,6 +1329,11 @@ options_act(or_options_t *old_options) circuit_expire_all_dirty_circs(); } + if (! bool_eq(options->BridgeRelay, old_options->BridgeRelay)) { + log_info(LD_GENERAL, "Bridge status changed. Forgetting GeoIP stats."); + geoip_remove_old_clients(time(NULL)+3600); + } + if (options_transition_affects_workers(old_options, options)) { log_info(LD_GENERAL, "Worker-related options changed. Rotating workers."); From 9e97067b2fc4032bce658d73cf21ee54573186bb Mon Sep 17 00:00:00 2001 From: Karsten Loesing Date: Wed, 13 May 2009 18:14:29 +0200 Subject: [PATCH 3/5] Prevent bridges from publishing router descriptors. Bridges are not supposed to publish router descriptors to the directory authorities. It defeats the point of bridges when they are included in the public relay directory. This patch puts out a warning and exits when the node is configured as a bridge and to publish v1, v2, or v3 descriptors at the same time. Also fixes part of bug 932. --- src/or/config.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/or/config.c b/src/or/config.c index 3d757c1bbb..7c1b60dcf4 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -3238,6 +3238,15 @@ options_validate(or_options_t *old_options, or_options_t *options, return -1; } + if ((options->BridgeRelay + || options->_PublishServerDescriptor & BRIDGE_AUTHORITY) + && options->_PublishServerDescriptor + & (V1_AUTHORITY + V2_AUTHORITY + V3_AUTHORITY)) { + REJECT("Bridges are not supposed to publish router descriptors to the " + "directory authorities. Please correct your " + "PublishServerDescriptor line."); + } + if (options->MinUptimeHidServDirectoryV2 < 0) { log_warn(LD_CONFIG, "MinUptimeHidServDirectoryV2 option must be at " "least 0 seconds. Changing to 0."); From a38ed1a235ef377993d5487fa2aa432abb2d2f2f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 13 May 2009 16:43:51 -0400 Subject: [PATCH 4/5] Use | with flags, not +. --- src/or/config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 7c1b60dcf4..b696b23a4c 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -3240,8 +3240,8 @@ options_validate(or_options_t *old_options, or_options_t *options, if ((options->BridgeRelay || options->_PublishServerDescriptor & BRIDGE_AUTHORITY) - && options->_PublishServerDescriptor - & (V1_AUTHORITY + V2_AUTHORITY + V3_AUTHORITY)) { + && (options->_PublishServerDescriptor + & (V1_AUTHORITY|V2_AUTHORITY|V3_AUTHORITY))) { REJECT("Bridges are not supposed to publish router descriptors to the " "directory authorities. Please correct your " "PublishServerDescriptor line."); From 45171cd9e9cb492bd64a1251dcd7758cab686f4b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 13 May 2009 16:44:10 -0400 Subject: [PATCH 5/5] Changelog entries for bug932 fixes --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index c4608212ed..4918dd2b71 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,6 +8,10 @@ Changes in version 0.2.1.15??? - ????-??-?? - Protect the count of open sockets with a mutex, so we can't corrupt it when two threads are closing or opening sockets at once. Fix for bug 939. Bugfix on 0.2.0.1-alpha. + - Don't allow a bridge to publish its router descriptor to a non-bridge + directory authority. Fixes part of bug 932. + - When we change to or from being a bridge, reset our counts of + client usage by country. Fixes bug 932. Changes in version 0.2.1.14-rc - 2009-04-12