February 2018 - tor-commits - lists.torproject.org

[tor/master] Merge remote-tracking branch 'dgoulet/ticket24902_029_05'
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit eafa252b26ecf83a8a48e7e19a3315e1d2983186 Merge: 9e48338a1 475218c10 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Feb 2 15:00:35 2018 -0500 Merge remote-tracking branch 'dgoulet/ticket24902_029_05' changes/ticket25122 | 4 ++ src/or/geoip.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++-- src/or/geoip.h | 2 + src/or/relay.c | 16 ++++-- src/test/test.c | 18 +++++++ 5 files changed, 180 insertions(+), 8 deletions(-) diff --cc src/or/geoip.c index 5b954979b,20dad5f15..15871f0d2 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@@ -510,12 -541,18 +542,21 @@@ HT_PROTOTYPE(clientmap, clientmap_entry HT_GENERATE2(clientmap, clientmap_entry_t, node, clientmap_entry_hash, clientmap_entries_eq, 0.6, tor_reallocarray_, tor_free_) +#define clientmap_entry_free(ent) \ + FREE_AND_NULL(clientmap_entry_t, clientmap_entry_free_, ent) + + /** Return the size of a client map entry. */ + static inline size_t + clientmap_entry_size(const clientmap_entry_t *ent) + { + tor_assert(ent); + return (sizeof(clientmap_entry_t) + + (ent->transport_name ? strlen(ent->transport_name) : 0)); + } + /** Free all storage held by ent. */ static void -clientmap_entry_free(clientmap_entry_t *ent) +clientmap_entry_free_(clientmap_entry_t *ent) { if (!ent) return; diff --cc src/or/relay.c index b1b99526d,22ce76752..506b7eccb --- a/src/or/relay.c +++ b/src/or/relay.c @@@ -2613,11 -2469,15 +2613,15 @@@ static time_t last_time_under_memory_pr STATIC int cell_queues_check_size(void) { + time_t now = time(NULL); size_t alloc = cell_queues_get_total_allocation(); alloc += buf_get_total_allocation(); - alloc += tor_zlib_get_total_allocation(); + alloc += tor_compress_get_total_allocation(); const size_t rend_cache_total = rend_cache_get_total_allocation(); alloc += rend_cache_total; + const size_t geoip_client_cache_total = + geoip_client_cache_total_allocation(); + alloc += geoip_client_cache_total; if (alloc >= get_options()->MaxMemInQueues_low_threshold) { last_time_under_memory_pressure = approx_time(); if (alloc >= get_options()->MaxMemInQueues) { @@@ -2627,7 -2487,15 +2631,13 @@@ if (rend_cache_total > get_options()->MaxMemInQueues / 5) { const size_t bytes_to_remove = rend_cache_total - (size_t)(get_options()->MaxMemInQueues / 10); - alloc -= hs_cache_handle_oom(time(NULL), bytes_to_remove); - rend_cache_clean_v2_descs_as_dir(now, bytes_to_remove); - alloc -= rend_cache_total; - alloc += rend_cache_get_total_allocation(); ++ alloc -= hs_cache_handle_oom(now, bytes_to_remove); + } + if (geoip_client_cache_total > get_options()->MaxMemInQueues / 5) { + const size_t bytes_to_remove = + geoip_client_cache_total - + (size_t)(get_options()->MaxMemInQueues / 10); + alloc -= geoip_client_cache_handle_oom(now, bytes_to_remove); } circuits_handle_oom(alloc); return 1;

1 0

[tor/master] geoip: Hook the client history cache into the OOM handler
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 51839f47650463f59bd2cc84da05d5bc535d699d Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 2 10:15:28 2018 -0500 geoip: Hook the client history cache into the OOM handler If the cache is using 20% of our maximum allowed memory, clean 10% of it. Same behavior as the HS descriptor cache. Closes #25122 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket25122 | 4 +++ src/or/geoip.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/or/geoip.h | 2 ++ src/or/relay.c | 16 ++++++++-- src/test/test.c | 18 +++++++++++ 5 files changed, 128 insertions(+), 3 deletions(-) diff --git a/changes/ticket25122 b/changes/ticket25122 new file mode 100644 index 000000000..2921811b2 --- /dev/null +++ b/changes/ticket25122 @@ -0,0 +1,4 @@ + o Minor feature (geoip cache): + - Make our OOM handler aware of the geoip client history cache so it + doesn't fill up the memory which is especially important for IPv6 and + our DoS mitigation subsystem. Closes ticket 25122. diff --git a/src/or/geoip.c b/src/or/geoip.c index 00c055bbe..c5e8cdab9 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -72,6 +72,10 @@ static smartlist_t *geoip_ipv4_entries = NULL, *geoip_ipv6_entries = NULL; static char geoip_digest[DIGEST_LEN]; static char geoip6_digest[DIGEST_LEN]; +/* Total size in bytes of the geoip client history cache. Used by the OOM + * handler. */ +static size_t geoip_client_history_cache_size; + /** Return the index of the country's entry in the GeoIP * country list if it is a valid 2-letter country code, otherwise * return -1. */ @@ -526,6 +530,15 @@ HT_PROTOTYPE(clientmap, clientmap_entry_t, node, clientmap_entry_hash, HT_GENERATE2(clientmap, clientmap_entry_t, node, clientmap_entry_hash, clientmap_entries_eq, 0.6, tor_reallocarray_, tor_free_) +/** Return the size of a client map entry. */ +static inline size_t +clientmap_entry_size(const clientmap_entry_t *ent) +{ + tor_assert(ent); + return (sizeof(clientmap_entry_t) + + (ent->transport_name ? strlen(ent->transport_name) : 0)); +} + /** Free all storage held by ent. */ static void clientmap_entry_free(clientmap_entry_t *ent) @@ -533,6 +546,8 @@ clientmap_entry_free(clientmap_entry_t *ent) if (!ent) return; + geoip_client_history_cache_size -= clientmap_entry_size(ent); + tor_free(ent->transport_name); tor_free(ent); } @@ -595,6 +610,7 @@ geoip_note_client_seen(geoip_client_action_t action, ent->transport_name = tor_strdup(transport_name); ent->action = (int)action; HT_INSERT(clientmap, &client_history, ent); + geoip_client_history_cache_size += clientmap_entry_size(ent); } if (now / 60 <= (int)MAX_LAST_SEEN_IN_MINUTES && now >= 0) ent->last_seen_in_minutes = (unsigned)(now/60); @@ -635,6 +651,81 @@ geoip_remove_old_clients(time_t cutoff) &cutoff); } +/* Cleanup client entries older than the cutoff. Used for the OOM. Return the + * number of bytes freed. If 0 is returned, nothing was freed. */ +static size_t +oom_clean_client_entries(time_t cutoff) +{ + size_t bytes = 0; + clientmap_entry_t **ent, **ent_next; + + for (ent = HT_START(clientmap, &client_history); ent; ent = ent_next) { + clientmap_entry_t *entry = *ent; + if (entry->last_seen_in_minutes < (cutoff / 60)) { + ent_next = HT_NEXT_RMV(clientmap, &client_history, ent); + bytes += clientmap_entry_size(entry); + clientmap_entry_free(entry); + } else { + ent_next = HT_NEXT(clientmap, &client_history, ent); + } + } + return bytes; +} + +/* Below this minimum lifetime, the OOM won't cleanup any entries. */ +#define GEOIP_CLIENT_CACHE_OOM_MIN_CUTOFF (4 * 60 * 60) +/* The OOM moves the cutoff by that much every run. */ +#define GEOIP_CLIENT_CACHE_OOM_STEP (15 * 50) + +/* Cleanup the geoip client history cache called from the OOM handler. Return + * the amount of bytes removed. This can return a value below or above + * min_remove_bytes but will stop as oon as the min_remove_bytes has been + * reached. */ +size_t +geoip_client_cache_handle_oom(time_t now, size_t min_remove_bytes) +{ + time_t k; + size_t bytes_removed = 0; + + /* Our OOM handler called with 0 bytes to remove is a code flow error. */ + tor_assert(min_remove_bytes != 0); + + /* Set k to the initial cutoff of an entry. We then going to move it by step + * to try to remove as much as we can. */ + k = WRITE_STATS_INTERVAL; + + do { + time_t cutoff; + + /* If k has reached the minimum lifetime, we have to stop else we might + * remove every single entries which would be pretty bad for the DoS + * mitigation subsystem if by just filling the geoip cache, it was enough + * to trigger the OOM and clean every single entries. */ + if (k <= GEOIP_CLIENT_CACHE_OOM_MIN_CUTOFF) { + break; + } + + cutoff = now - k; + bytes_removed += oom_clean_client_entries(cutoff); + k -= GEOIP_CLIENT_CACHE_OOM_STEP; + } while (bytes_removed < min_remove_bytes); + + return bytes_removed; +} + +/* Return the total size in bytes of the client history cache. */ +size_t +geoip_client_cache_total_allocation(void) +{ + size_t bytes = 0; + clientmap_entry_t **ent; + + HT_FOREACH(ent, clientmap, &client_history) { + bytes += clientmap_entry_size(*ent); + } + return bytes; +} + /** How many responses are we giving to clients requesting v3 network * statuses? */ static uint32_t ns_v3_responses[GEOIP_NS_RESPONSE_NUM]; diff --git a/src/or/geoip.h b/src/or/geoip.h index 070296dd0..42d0c1cfd 100644 --- a/src/or/geoip.h +++ b/src/or/geoip.h @@ -33,6 +33,8 @@ void geoip_note_client_seen(geoip_client_action_t action, const tor_addr_t *addr, const char *transport_name, time_t now); void geoip_remove_old_clients(time_t cutoff); +size_t geoip_client_cache_total_allocation(void); +size_t geoip_client_cache_handle_oom(time_t now, size_t min_remove_bytes); void geoip_note_ns_response(geoip_ns_response_t response); char *geoip_get_transport_history(void); diff --git a/src/or/relay.c b/src/or/relay.c index 29f34ca03..22ce76752 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2469,24 +2469,34 @@ static time_t last_time_under_memory_pressure = 0; STATIC int cell_queues_check_size(void) { + time_t now = time(NULL); size_t alloc = cell_queues_get_total_allocation(); alloc += buf_get_total_allocation(); alloc += tor_zlib_get_total_allocation(); const size_t rend_cache_total = rend_cache_get_total_allocation(); alloc += rend_cache_total; + const size_t geoip_client_cache_total = + geoip_client_cache_total_allocation(); + alloc += geoip_client_cache_total; if (alloc >= get_options()->MaxMemInQueues_low_threshold) { last_time_under_memory_pressure = approx_time(); if (alloc >= get_options()->MaxMemInQueues) { /* If we're spending over 20% of the memory limit on hidden service - * descriptors, free them until we're down to 10%. - */ + * descriptors, free them until we're down to 10%. Do the same for geoip + * client cache. */ if (rend_cache_total > get_options()->MaxMemInQueues / 5) { const size_t bytes_to_remove = rend_cache_total - (size_t)(get_options()->MaxMemInQueues / 10); - rend_cache_clean_v2_descs_as_dir(time(NULL), bytes_to_remove); + rend_cache_clean_v2_descs_as_dir(now, bytes_to_remove); alloc -= rend_cache_total; alloc += rend_cache_get_total_allocation(); } + if (geoip_client_cache_total > get_options()->MaxMemInQueues / 5) { + const size_t bytes_to_remove = + geoip_client_cache_total - + (size_t)(get_options()->MaxMemInQueues / 10); + alloc -= geoip_client_cache_handle_oom(now, bytes_to_remove); + } circuits_handle_oom(alloc); return 1; } diff --git a/src/test/test.c b/src/test/test.c index 9a41b976b..e2fbfd21b 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -912,6 +912,24 @@ test_geoip(void *arg) tt_str_op(entry_stats_2,OP_EQ, s); tor_free(s); + /* Test the OOM handler. Add a client, run the OOM. */ + geoip_entry_stats_init(now); + SET_TEST_ADDRESS(100); + geoip_note_client_seen(GEOIP_CLIENT_CONNECT, &addr, NULL, + now - (12 * 60 * 60)); + /* We've seen this 12 hours ago. Run the OOM, it should clean the entry + * because it is above the minimum cutoff of 4 hours. */ + size_t bytes_removed = geoip_client_cache_handle_oom(now, 1000); + tt_size_op(bytes_removed, OP_GT, 0); + + /* Do it again but this time with an entry with a lower cutoff. */ + geoip_entry_stats_init(now); + SET_TEST_ADDRESS(100); + geoip_note_client_seen(GEOIP_CLIENT_CONNECT, &addr, NULL, + now - (3 * 60 * 60)); + bytes_removed = geoip_client_cache_handle_oom(now, 1000); + tt_size_op(bytes_removed, OP_EQ, 0); + /* Stop collecting entry statistics. */ geoip_entry_stats_term(); get_options_mutable()->EntryStatistics = 0;

1 0

[tor/master] geoip: Increment and decrement functions for the geoip client cache
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 4d812e29b9b1ec88fe268c150a826466b23a8762 Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 2 13:14:50 2018 -0500 geoip: Increment and decrement functions for the geoip client cache These functions protect againts over and underflow. They BUG() in case we overflow the counter. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/geoip.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/src/or/geoip.c b/src/or/geoip.c index c5e8cdab9..92db9742e 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -76,6 +76,34 @@ static char geoip6_digest[DIGEST_LEN]; * handler. */ static size_t geoip_client_history_cache_size; +/* Increment the geoip client history cache size counter with the given bytes. + * This prevents an overflow and set it to its maximum in that case. */ +static inline void +geoip_increment_client_history_cache_size(size_t bytes) +{ + /* This is shockingly high, lets log it so it can be reported. */ + IF_BUG_ONCE(geoip_client_history_cache_size > (SIZE_MAX - bytes)) { + geoip_client_history_cache_size = SIZE_MAX; + return; + } + geoip_client_history_cache_size += bytes; +} + +/* Decrement the geoip client history cache size counter with the given bytes. + * This prevents an underflow and set it to 0 in that case. */ +static inline void +geoip_decrement_client_history_cache_size(size_t bytes) +{ + /* Going below 0 means that we either allocated an entry without + * incrementing the counter or we have different sizes when allocating and + * freeing. It shouldn't happened so log it. */ + IF_BUG_ONCE(geoip_client_history_cache_size < bytes) { + geoip_client_history_cache_size = 0; + return; + } + geoip_client_history_cache_size -= bytes; +} + /** Return the index of the country's entry in the GeoIP * country list if it is a valid 2-letter country code, otherwise * return -1. */ @@ -546,7 +574,7 @@ clientmap_entry_free(clientmap_entry_t *ent) if (!ent) return; - geoip_client_history_cache_size -= clientmap_entry_size(ent); + geoip_decrement_client_history_cache_size(clientmap_entry_size(ent)); tor_free(ent->transport_name); tor_free(ent); @@ -610,7 +638,7 @@ geoip_note_client_seen(geoip_client_action_t action, ent->transport_name = tor_strdup(transport_name); ent->action = (int)action; HT_INSERT(clientmap, &client_history, ent); - geoip_client_history_cache_size += clientmap_entry_size(ent); + geoip_increment_client_history_cache_size(clientmap_entry_size(ent)); } if (now / 60 <= (int)MAX_LAST_SEEN_IN_MINUTES && now >= 0) ent->last_seen_in_minutes = (unsigned)(now/60);

1 0

[tor/master] Merge branch 'ticket25122_029_02' into ticket24902_029_05
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 475218c108ad84aa302f0daec013faab9ff778f2 Merge: 33d9889a2 e758d659a Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 2 14:55:01 2018 -0500 Merge branch 'ticket25122_029_02' into ticket24902_029_05 changes/ticket25122 | 4 ++ src/or/geoip.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++-- src/or/geoip.h | 2 + src/or/relay.c | 16 ++++-- src/test/test.c | 18 +++++++ 5 files changed, 180 insertions(+), 8 deletions(-) diff --cc src/or/geoip.c index 4e4f6e639,76fca43f6..20dad5f15 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@@ -516,9 -574,7 +557,10 @@@ clientmap_entry_free(clientmap_entry_t if (!ent) return; + /* This entry is about to be freed so pass it to the DoS subsystem to see if + * any actions can be taken about it. */ + dos_geoip_entry_about_to_free(ent); + geoip_decrement_client_history_cache_size(clientmap_entry_size(ent)); tor_free(ent->transport_name); tor_free(ent); @@@ -573,13 -651,13 +640,9 @@@ geoip_note_client_seen(geoip_client_act safe_str_client(fmt_addr((addr))), transport_name ? transport_name : "<no transport>"); - tor_addr_copy(&lookup.addr, addr); - lookup.action = (int)action; - lookup.transport_name = (char*) transport_name; - ent = HT_FIND(clientmap, &client_history, &lookup); - + ent = geoip_lookup_client(addr, transport_name, action); if (! ent) { - ent = tor_malloc_zero(sizeof(clientmap_entry_t)); - tor_addr_copy(&ent->addr, addr); - if (transport_name) - ent->transport_name = tor_strdup(transport_name); - ent->action = (int)action; + ent = clientmap_entry_new(action, addr, transport_name); HT_INSERT(clientmap, &client_history, ent); } if (now / 60 <= (int)MAX_LAST_SEEN_IN_MINUTES && now >= 0) @@@ -621,25 -699,81 +684,100 @@@ geoip_remove_old_clients(time_t cutoff &cutoff); } +/* Return a client entry object matching the given address, transport name and + * geoip action from the clientmap. NULL if not found. The transport_name can + * be NULL. */ +clientmap_entry_t * +geoip_lookup_client(const tor_addr_t *addr, const char *transport_name, + geoip_client_action_t action) +{ + clientmap_entry_t lookup; + + tor_assert(addr); + + /* We always look for a client connection with no transport. */ + tor_addr_copy(&lookup.addr, addr); + lookup.action = action; + lookup.transport_name = (char *) transport_name; + + return HT_FIND(clientmap, &client_history, &lookup); +} + + /* Cleanup client entries older than the cutoff. Used for the OOM. Return the + * number of bytes freed. If 0 is returned, nothing was freed. */ + static size_t + oom_clean_client_entries(time_t cutoff) + { + size_t bytes = 0; + clientmap_entry_t **ent, **ent_next; + + for (ent = HT_START(clientmap, &client_history); ent; ent = ent_next) { + clientmap_entry_t *entry = *ent; + if (entry->last_seen_in_minutes < (cutoff / 60)) { + ent_next = HT_NEXT_RMV(clientmap, &client_history, ent); + bytes += clientmap_entry_size(entry); + clientmap_entry_free(entry); + } else { + ent_next = HT_NEXT(clientmap, &client_history, ent); + } + } + return bytes; + } + + /* Below this minimum lifetime, the OOM won't cleanup any entries. */ + #define GEOIP_CLIENT_CACHE_OOM_MIN_CUTOFF (4 * 60 * 60) + /* The OOM moves the cutoff by that much every run. */ + #define GEOIP_CLIENT_CACHE_OOM_STEP (15 * 50) + + /* Cleanup the geoip client history cache called from the OOM handler. Return + * the amount of bytes removed. This can return a value below or above + * min_remove_bytes but will stop as oon as the min_remove_bytes has been + * reached. */ + size_t + geoip_client_cache_handle_oom(time_t now, size_t min_remove_bytes) + { + time_t k; + size_t bytes_removed = 0; + + /* Our OOM handler called with 0 bytes to remove is a code flow error. */ + tor_assert(min_remove_bytes != 0); + + /* Set k to the initial cutoff of an entry. We then going to move it by step + * to try to remove as much as we can. */ + k = WRITE_STATS_INTERVAL; + + do { + time_t cutoff; + + /* If k has reached the minimum lifetime, we have to stop else we might + * remove every single entries which would be pretty bad for the DoS + * mitigation subsystem if by just filling the geoip cache, it was enough + * to trigger the OOM and clean every single entries. */ + if (k <= GEOIP_CLIENT_CACHE_OOM_MIN_CUTOFF) { + break; + } + + cutoff = now - k; + bytes_removed += oom_clean_client_entries(cutoff); + k -= GEOIP_CLIENT_CACHE_OOM_STEP; + } while (bytes_removed < min_remove_bytes); + + return bytes_removed; + } + + /* Return the total size in bytes of the client history cache. */ + size_t + geoip_client_cache_total_allocation(void) + { + size_t bytes = 0; + clientmap_entry_t **ent; + + HT_FOREACH(ent, clientmap, &client_history) { + bytes += clientmap_entry_size(*ent); + } + return bytes; + } + /** How many responses are we giving to clients requesting v3 network * statuses? */ static uint32_t ns_v3_responses[GEOIP_NS_RESPONSE_NUM]; diff --cc src/or/geoip.h index aa0fca50f,42d0c1cfd..c8ea9f85e --- a/src/or/geoip.h +++ b/src/or/geoip.h @@@ -57,9 -33,8 +57,11 @@@ void geoip_note_client_seen(geoip_clien const tor_addr_t *addr, const char *transport_name, time_t now); void geoip_remove_old_clients(time_t cutoff); +clientmap_entry_t *geoip_lookup_client(const tor_addr_t *addr, + const char *transport_name, + geoip_client_action_t action); + size_t geoip_client_cache_total_allocation(void); + size_t geoip_client_cache_handle_oom(time_t now, size_t min_remove_bytes); void geoip_note_ns_response(geoip_ns_response_t response); char *geoip_get_transport_history(void);

1 0

[tor/master] geoip: Add clientmap_entry_new() function
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit e758d659a0bc8b9a0e2bd6a0126755fd1fb58e0a Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 2 13:24:37 2018 -0500 geoip: Add clientmap_entry_new() function Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/geoip.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/src/or/geoip.c b/src/or/geoip.c index 92db9742e..76fca43f6 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -580,6 +580,31 @@ clientmap_entry_free(clientmap_entry_t *ent) tor_free(ent); } +/* Return a newly allocated clientmap entry with the given action and address + * that are mandatory. The transport_name can be optional. This can't fail. */ +static clientmap_entry_t * +clientmap_entry_new(geoip_client_action_t action, const tor_addr_t *addr, + const char *transport_name) +{ + clientmap_entry_t *entry; + + tor_assert(action == GEOIP_CLIENT_CONNECT || + action == GEOIP_CLIENT_NETWORKSTATUS); + tor_assert(addr); + + entry = tor_malloc_zero(sizeof(clientmap_entry_t)); + entry->action = action; + tor_addr_copy(&entry->addr, addr); + if (transport_name) { + entry->transport_name = tor_strdup(transport_name); + } + + /* Allocated and initialized, note down its size for the OOM handler. */ + geoip_increment_client_history_cache_size(clientmap_entry_size(entry)); + + return entry; +} + /** Clear history of connecting clients used by entry and bridge stats. */ static void client_history_clear(void) @@ -632,13 +657,8 @@ geoip_note_client_seen(geoip_client_action_t action, ent = HT_FIND(clientmap, &client_history, &lookup); if (! ent) { - ent = tor_malloc_zero(sizeof(clientmap_entry_t)); - tor_addr_copy(&ent->addr, addr); - if (transport_name) - ent->transport_name = tor_strdup(transport_name); - ent->action = (int)action; + ent = clientmap_entry_new(action, addr, transport_name); HT_INSERT(clientmap, &client_history, ent); - geoip_increment_client_history_cache_size(clientmap_entry_size(ent)); } if (now / 60 <= (int)MAX_LAST_SEEN_IN_MINUTES && now >= 0) ent->last_seen_in_minutes = (unsigned)(now/60);

1 0

[tor/maint-0.3.2] Merge branch 'bug25125_032_01_squashed' into maint-0.3.2
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 4ceae7c6b9742ce6b14f8bed34725e44522e52f2 Merge: 77634795b 005e228f8 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Feb 2 12:03:48 2018 -0500 Merge branch 'bug25125_032_01_squashed' into maint-0.3.2 src/or/scheduler.c | 26 +++++++++++++++----------- src/test/test_scheduler.c | 12 ++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-)

1 0

[tor/master] Merge branch 'bug25125_032_01_squashed' into maint-0.3.2
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 4ceae7c6b9742ce6b14f8bed34725e44522e52f2 Merge: 77634795b 005e228f8 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Feb 2 12:03:48 2018 -0500 Merge branch 'bug25125_032_01_squashed' into maint-0.3.2 src/or/scheduler.c | 26 +++++++++++++++----------- src/test/test_scheduler.c | 12 ++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-)

1 0

[tor/maint-0.3.2] sched: When releasing a channel, do not BUG() if absent from the pending list
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 005e228f80f0bd241d01c106f87e6bfe6dda6127 Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 2 08:48:34 2018 -0500 sched: When releasing a channel, do not BUG() if absent from the pending list The current code flow makes it that we can release a channel in a PENDING state but not in the pending list. This happens while the channel is being processed in the scheduler loop. Fixes #25125 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/scheduler.c | 26 +++++++++++++++----------- src/test/test_scheduler.c | 12 ++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/or/scheduler.c b/src/or/scheduler.c index 47141c538..1984084fe 100644 --- a/src/or/scheduler.c +++ b/src/or/scheduler.c @@ -628,17 +628,21 @@ scheduler_release_channel,(channel_t *chan)) return; } - if (chan->scheduler_state == SCHED_CHAN_PENDING) { - if (SCHED_BUG(smartlist_pos(channels_pending, chan) == -1, chan)) { - log_warn(LD_SCHED, "Scheduler asked to release channel %" PRIu64 " " - "but it wasn't in channels_pending", - chan->global_identifier); - } else { - smartlist_pqueue_remove(channels_pending, - scheduler_compare_channels, - offsetof(channel_t, sched_heap_idx), - chan); - } + /* Try to remove the channel from the pending list regardless of its + * scheduler state. We can release a channel in many places in the tor code + * so we can't rely on the channel state (PENDING) to remove it from the + * list. + * + * For instance, the channel can change state from OPEN to CLOSING while + * being handled in the scheduler loop leading to the channel being in + * PENDING state but not in the pending list. Furthermore, we release the + * channel when it changes state to close and a second time when we free it. + * Not ideal at all but for now that is the way it is. */ + if (chan->sched_heap_idx != -1) { + smartlist_pqueue_remove(channels_pending, + scheduler_compare_channels, + offsetof(channel_t, sched_heap_idx), + chan); } if (the_scheduler->on_channel_free) { diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index 1f014c4f6..724a6b56b 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -502,6 +502,18 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type) scheduler_touch_channel(ch1); tt_assert(scheduler_compare_channels_mock_ctr > old_count); + /* Release the ch2 and then do it another time to make sure it doesn't blow + * up and we are still in a quiescent state. */ + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Cheat a bit so make the release more confused but also will tells us if + * the release did put the channel in the right state. */ + ch2->scheduler_state = SCHED_CHAN_PENDING; + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Close */ channel_mark_for_close(ch1); tt_int_op(ch1->state, OP_EQ, CHANNEL_STATE_CLOSING);

1 0

[tor/master] sched: When releasing a channel, do not BUG() if absent from the pending list
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 005e228f80f0bd241d01c106f87e6bfe6dda6127 Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 2 08:48:34 2018 -0500 sched: When releasing a channel, do not BUG() if absent from the pending list The current code flow makes it that we can release a channel in a PENDING state but not in the pending list. This happens while the channel is being processed in the scheduler loop. Fixes #25125 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/scheduler.c | 26 +++++++++++++++----------- src/test/test_scheduler.c | 12 ++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/or/scheduler.c b/src/or/scheduler.c index 47141c538..1984084fe 100644 --- a/src/or/scheduler.c +++ b/src/or/scheduler.c @@ -628,17 +628,21 @@ scheduler_release_channel,(channel_t *chan)) return; } - if (chan->scheduler_state == SCHED_CHAN_PENDING) { - if (SCHED_BUG(smartlist_pos(channels_pending, chan) == -1, chan)) { - log_warn(LD_SCHED, "Scheduler asked to release channel %" PRIu64 " " - "but it wasn't in channels_pending", - chan->global_identifier); - } else { - smartlist_pqueue_remove(channels_pending, - scheduler_compare_channels, - offsetof(channel_t, sched_heap_idx), - chan); - } + /* Try to remove the channel from the pending list regardless of its + * scheduler state. We can release a channel in many places in the tor code + * so we can't rely on the channel state (PENDING) to remove it from the + * list. + * + * For instance, the channel can change state from OPEN to CLOSING while + * being handled in the scheduler loop leading to the channel being in + * PENDING state but not in the pending list. Furthermore, we release the + * channel when it changes state to close and a second time when we free it. + * Not ideal at all but for now that is the way it is. */ + if (chan->sched_heap_idx != -1) { + smartlist_pqueue_remove(channels_pending, + scheduler_compare_channels, + offsetof(channel_t, sched_heap_idx), + chan); } if (the_scheduler->on_channel_free) { diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index 1f014c4f6..724a6b56b 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -502,6 +502,18 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type) scheduler_touch_channel(ch1); tt_assert(scheduler_compare_channels_mock_ctr > old_count); + /* Release the ch2 and then do it another time to make sure it doesn't blow + * up and we are still in a quiescent state. */ + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Cheat a bit so make the release more confused but also will tells us if + * the release did put the channel in the right state. */ + ch2->scheduler_state = SCHED_CHAN_PENDING; + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Close */ channel_mark_for_close(ch1); tt_int_op(ch1->state, OP_EQ, CHANNEL_STATE_CLOSING);

1 0

[tor/release-0.3.2] sched: When releasing a channel, do not BUG() if absent from the pending list
by nickm＠torproject.org 02 Feb '18

02 Feb '18

commit 005e228f80f0bd241d01c106f87e6bfe6dda6127 Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Feb 2 08:48:34 2018 -0500 sched: When releasing a channel, do not BUG() if absent from the pending list The current code flow makes it that we can release a channel in a PENDING state but not in the pending list. This happens while the channel is being processed in the scheduler loop. Fixes #25125 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/scheduler.c | 26 +++++++++++++++----------- src/test/test_scheduler.c | 12 ++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/or/scheduler.c b/src/or/scheduler.c index 47141c538..1984084fe 100644 --- a/src/or/scheduler.c +++ b/src/or/scheduler.c @@ -628,17 +628,21 @@ scheduler_release_channel,(channel_t *chan)) return; } - if (chan->scheduler_state == SCHED_CHAN_PENDING) { - if (SCHED_BUG(smartlist_pos(channels_pending, chan) == -1, chan)) { - log_warn(LD_SCHED, "Scheduler asked to release channel %" PRIu64 " " - "but it wasn't in channels_pending", - chan->global_identifier); - } else { - smartlist_pqueue_remove(channels_pending, - scheduler_compare_channels, - offsetof(channel_t, sched_heap_idx), - chan); - } + /* Try to remove the channel from the pending list regardless of its + * scheduler state. We can release a channel in many places in the tor code + * so we can't rely on the channel state (PENDING) to remove it from the + * list. + * + * For instance, the channel can change state from OPEN to CLOSING while + * being handled in the scheduler loop leading to the channel being in + * PENDING state but not in the pending list. Furthermore, we release the + * channel when it changes state to close and a second time when we free it. + * Not ideal at all but for now that is the way it is. */ + if (chan->sched_heap_idx != -1) { + smartlist_pqueue_remove(channels_pending, + scheduler_compare_channels, + offsetof(channel_t, sched_heap_idx), + chan); } if (the_scheduler->on_channel_free) { diff --git a/src/test/test_scheduler.c b/src/test/test_scheduler.c index 1f014c4f6..724a6b56b 100644 --- a/src/test/test_scheduler.c +++ b/src/test/test_scheduler.c @@ -502,6 +502,18 @@ perform_channel_state_tests(int KISTSchedRunInterval, int sched_type) scheduler_touch_channel(ch1); tt_assert(scheduler_compare_channels_mock_ctr > old_count); + /* Release the ch2 and then do it another time to make sure it doesn't blow + * up and we are still in a quiescent state. */ + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Cheat a bit so make the release more confused but also will tells us if + * the release did put the channel in the right state. */ + ch2->scheduler_state = SCHED_CHAN_PENDING; + scheduler_release_channel(ch2); + tt_int_op(ch2->scheduler_state, OP_EQ, SCHED_CHAN_IDLE); + tt_int_op(smartlist_len(channels_pending), OP_EQ, 1); + /* Close */ channel_mark_for_close(ch1); tt_int_op(ch1->state, OP_EQ, CHANNEL_STATE_CLOSING);

1 0