September 2014 - tor-commits - lists.torproject.org

[tor/master] Stop spawn test failures due to a race condition with SIGCHLD on process exit
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit b827a08284230bb1a08ddd7eff258d2b690d6793 Author: teor <teor2345(a)gmail.com> Date: Mon Sep 29 21:50:17 2014 +1000 Stop spawn test failures due to a race condition with SIGCHLD on process exit When a spawned process forks, fails, then exits very quickly, (this typically occurs when exec fails), there is a race condition between the SIGCHLD handler updating the process_handle's fields, and checking the process status in those fields. The update can occur … [View More]before or after the spawn tests check the process status. We check whether the process is running or not running (rather than just checking if it is running) to avoid this issue. --- changes/bug13291-spawn-test-race-condition | 4 ++ src/test/test_util.c | 62 +++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/changes/bug13291-spawn-test-race-condition b/changes/bug13291-spawn-test-race-condition new file mode 100644 index 0000000..c14de8a --- /dev/null +++ b/changes/bug13291-spawn-test-race-condition @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Stop spawn test failures due to a race condition between the SIGCHLD + handler updating the process status, and the test reading it. + Fixes bug 13291. diff --git a/src/test/test_util.c b/src/test/test_util.c index 9d9b393..7acda6b 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -2850,6 +2850,30 @@ test_util_fgets_eagain(void *ptr) #define EOL "\n" #endif +#ifdef _WIN32 +/* I've assumed Windows doesn't have the gap between fork and exec + * that causes the race condition on unix-like platforms */ +#define MATCH_PROCESS_STATUS(s1,s2) (s1 == s2) + +#else +/* work around a race condition of the timing of SIGCHLD handler updates + * to the process_handle's fields, and checks of those fields + * + * TODO: Once we can signal failure to exec, change PROCESS_STATUS_RUNNING to + * PROCESS_STATUS_ERROR (and similarly with *_OR_NOTRUNNING) */ +#define PROCESS_STATUS_RUNNING_OR_NOTRUNNING (PROCESS_STATUS_RUNNING+1) +#define IS_RUNNING_OR_NOTRUNNING(s) \ + (s == PROCESS_STATUS_RUNNING || s == PROCESS_STATUS_NOTRUNNING) +/* well, this is ugly */ +#define MATCH_PROCESS_STATUS(s1,s2) \ + ( s1 == s2 \ + ||(s1 == PROCESS_STATUS_RUNNING_OR_NOTRUNNING \ + && IS_RUNNING_OR_NOTRUNNING(s2)) \ + ||(s2 == PROCESS_STATUS_RUNNING_OR_NOTRUNNING \ + && IS_RUNNING_OR_NOTRUNNING(s1))) + +#endif // _WIN32 + /** Helper function for testing tor_spawn_background */ static void run_util_spawn_background(const char *argv[], const char *expected_out, @@ -2871,18 +2895,39 @@ run_util_spawn_background(const char *argv[], const char *expected_out, notify_pending_waitpid_callbacks(); - tt_int_op(expected_status,==, status); + /* the race condition doesn't affect status, + * because status isn't updated by the SIGCHLD handler, + * but we still need to handle PROCESS_STATUS_RUNNING_OR_NOTRUNNING */ + tt_assert(MATCH_PROCESS_STATUS(expected_status, status)); if (status == PROCESS_STATUS_ERROR) { tt_ptr_op(process_handle, ==, NULL); return; } tt_assert(process_handle != NULL); - tt_int_op(expected_status,==, process_handle->status); + + /* When a spawned process forks, fails, then exits very quickly, + * (this typically occurs when exec fails) + * there is a race condition between the SIGCHLD handler + * updating the process_handle's fields, and this test + * checking the process status in those fields. + * The SIGCHLD update can occur before or after the code below executes. + * This causes intermittent failures in spawn_background_fail(), + * typically when the machine is under load. + * We use PROCESS_STATUS_RUNNING_OR_NOTRUNNING to avoid this issue. */ + + /* the race condition affects the change in + * process_handle->status from RUNNING to NOTRUNNING */ + tt_assert(MATCH_PROCESS_STATUS(expected_status, process_handle->status)); #ifndef _WIN32 notify_pending_waitpid_callbacks(); - tt_ptr_op(process_handle->waitpid_cb, !=, NULL); + /* the race condition affects the change in + * process_handle->waitpid_cb to NULL, + * so we skip the check if expected_status is ambiguous, + * that is, PROCESS_STATUS_RUNNING_OR_NOTRUNNING */ + tt_assert(process_handle->waitpid_cb != NULL + || expected_status == PROCESS_STATUS_RUNNING_OR_NOTRUNNING); #endif #ifdef _WIN32 @@ -2955,8 +3000,8 @@ test_util_spawn_background_fail(void *ptr) const int expected_status = PROCESS_STATUS_ERROR; #else /* TODO: Once we can signal failure to exec, set this to be - * PROCESS_STATUS_ERROR */ - const int expected_status = PROCESS_STATUS_RUNNING; + * PROCESS_STATUS_RUNNING_OR_ERROR */ + const int expected_status = PROCESS_STATUS_RUNNING_OR_NOTRUNNING; #endif memset(expected_out, 0xf0, sizeof(expected_out)); @@ -3149,6 +3194,13 @@ test_util_spawn_background_waitpid_notify(void *arg) #undef TEST_CHILD #undef EOL +#undef MATCH_PROCESS_STATUS + +#ifndef _WIN32 +#undef PROCESS_STATUS_RUNNING_OR_NOTRUNNING +#undef IS_RUNNING_OR_NOTRUNNING +#endif + /** * Test for format_hex_number_sigsafe() */ [View Less]

1 0

[tor/master] Parenthesize macro arguments for 13291 fix
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit 0a985af072812354742159777bea87777577889c Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Sep 29 09:38:50 2014 -0400 Parenthesize macro arguments for 13291 fix --- src/test/test_util.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/test_util.c b/src/test/test_util.c index 7acda6b..693cc06 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -2853,7 +2853,7 @@ test_util_fgets_eagain(void *ptr) #ifdef _WIN32 … [View More]

1 0

[tor/master] Whitespace fixes on 13291 fix
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit 15b0bf0aad53655a22430dbdf7d23f1f273f0877 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Sep 29 09:39:21 2014 -0400 Whitespace fixes on 13291 fix --- src/test/test_util.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/test_util.c b/src/test/test_util.c index 693cc06..c67aa71 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -2856,7 +2856,7 @@ test_util_fgets_eagain(void *ptr) #define MATCH_PROCESS_STATUS(s1,… [View More]

1 0

[tor/master] Note when 13291 was introduced.
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit ba7b6246b7dc6d75ccf28329c41ea1d688123127 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Sep 29 09:40:33 2014 -0400 Note when 13291 was introduced. --- changes/bug13291-spawn-test-race-condition | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/bug13291-spawn-test-race-condition b/changes/bug13291-spawn-test-race-condition index c14de8a..bedd799 100644 --- a/changes/bug13291-spawn-test-race-condition +++ b/changes/bug13291-spawn-test-… [View More]

1 0

[tor/master] Add an "AccountingRule" feature to permit limiting bw usage by read+write
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit 8527a2996675d9502551ccdd4224036a45aae47b Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Sep 23 08:34:22 2014 -0400 Add an "AccountingRule" feature to permit limiting bw usage by read+write Patch from "chobe". Closes ticket 961. --- doc/tor.1.txt | 38 ++++++++++++++-------- src/or/config.c | 5 +++ src/or/hibernate.c | 50 ++++++++++++++++++++--------- src/or/hibernate.h | 1 + src/or/or.h … [View More]| 4 +++ src/or/router.c | 7 +++- src/or/status.c | 7 +++- src/test/include.am | 1 + src/test/test.c | 2 ++ src/test/test_accounting.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ 10 files changed, 161 insertions(+), 30 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index d6b1432..c9172b8 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1635,19 +1635,31 @@ is non-zero): to 0 will disable the heartbeat. (Default: 6 hours) [[AccountingMax]] **AccountingMax** __N__ **bytes**|**KBytes**|**MBytes**|**GBytes**|**KBits**|**MBits**|**GBits**|**TBytes**:: - Never send more than the specified number of bytes in a given accounting - period, or receive more than that number in the period. For example, with - AccountingMax set to 1 GByte, a server could send 900 MBytes and - receive 800 MBytes and continue running. It will only hibernate once - one of the two reaches 1 GByte. When the number of bytes gets low, - Tor will stop accepting new connections and circuits. When the - number of bytes is exhausted, Tor will hibernate until some - time in the next accounting period. To prevent all servers from waking at - the same time, Tor will also wait until a random point in each period - before waking up. If you have bandwidth cost issues, enabling hibernation - is preferable to setting a low bandwidth, since it provides users with a - collection of fast servers that are up some of the time, which is more - useful than a set of slow servers that are always "available". + Limits the max number of bytes sent and received within a set time period + using a given calculation rule (see: AccountingStart, AccountingRule). + Useful if you need to stay under a specific bandwidth. By default, the + number used for calculation is the max of either the bytes sent or + received. For example, with AccountingMax set to 1 GByte, a server + could send 900 MBytes and receive 800 MBytes and continue running. + It will only hibernate once one of the two reaches 1 GByte. This can + be changed to use the sum of the both bytes received and sent by setting + the AccountingRule option to "sum" (total bandwidth in/out). When the + number of bytes remaining gets low, Tor will stop accepting new connections + and circuits. When the number of bytes is exhausted, Tor will hibernate + until some time in the next accounting period. To prevent all servers + from waking at the same time, Tor will also wait until a random point + in each period before waking up. If you have bandwidth cost issues, + enabling hibernation is preferable to setting a low bandwidth, since + it provides users with a collection of fast servers that are up some + of the time, which is more useful than a set of slow servers that are + always "available". + +[[AccountingRule]] **AccountingRule** **sum**|**max**:: + How we determine when our AccountingMax has been reached (when we + should hibernate) during a time interval. Set to "max" to calculate + using the higher of either the sent or received bytes (this is the + default functionality). Set to "sum" to calculate using the sent + plus received bytes. [[AccountingStart]] **AccountingStart** **day**|**week**|**month** [__day__] __HH:MM__:: Specify how long accounting periods last. If **month** is given, each diff --git a/src/or/config.c b/src/or/config.c index 16acec7..c7c6154 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -126,6 +126,7 @@ static config_abbrev_t option_abbrevs_[] = { */ static config_var_t option_vars_[] = { V(AccountingMax, MEMUNIT, "0 bytes"), + V(AccountingRule, STRING, "max"), V(AccountingStart, STRING, NULL), V(Address, STRING, NULL), V(AllowDotExit, BOOL, "0"), @@ -3108,6 +3109,10 @@ options_validate(or_options_t *old_options, or_options_t *options, "risky: they will all turn off at the same time, which may " "alert observers that they are being run by the same party."); } + if (options->AccountingRule && + strcmp(options->AccountingRule, "sum") != 0 && + strcmp(options->AccountingRule, "max") != 0) + REJECT("AccountingRule must be 'sum' or 'max'"); } if (options->HTTPProxy) { /* parse it now */ diff --git a/src/or/hibernate.c b/src/or/hibernate.c index c433ac1..6978f47 100644 --- a/src/or/hibernate.c +++ b/src/or/hibernate.c @@ -410,6 +410,16 @@ configure_accounting(time_t now) accounting_set_wakeup_time(); } +/** Return the relevant number of bytes sent/received this interval + * based on the set AccountingRule */ +static uint64_t +get_accounting_bytes(void) +{ + if (strcmp(get_options()->AccountingRule, "sum") == 0) + return n_bytes_read_in_interval+n_bytes_written_in_interval; + return MAX(n_bytes_read_in_interval, n_bytes_written_in_interval); +} + /** Set expected_bandwidth_usage based on how much we sent/received * per minute last interval (if we were up for at least 30 minutes), * or based on our declared bandwidth otherwise. */ @@ -421,6 +431,11 @@ update_expected_bandwidth(void) uint64_t max_configured = (options->RelayBandwidthRate > 0 ? options->RelayBandwidthRate : options->BandwidthRate) * 60; + /* max_configured is the larger of bytes read and bytes written + * If we are accounting based on sum, worst case is both are + * at max, doubling the expected sum of bandwidth */ + if (strcmp(get_options()->AccountingRule, "sum") == 0) + max_configured *= 2; #define MIN_TIME_FOR_MEASUREMENT (1800) @@ -439,8 +454,7 @@ update_expected_bandwidth(void) * doesn't know to store soft-limit info. Just take rate at which * we were reading/writing in the last interval as our expected rate. */ - uint64_t used = MAX(n_bytes_written_in_interval, - n_bytes_read_in_interval); + uint64_t used = get_accounting_bytes(); expected = used / (n_seconds_active_in_interval / 60); } else { /* If we haven't gotten enough data last interval, set 'expected' @@ -715,8 +729,7 @@ hibernate_hard_limit_reached(void) uint64_t hard_limit = get_options()->AccountingMax; if (!hard_limit) return 0; - return n_bytes_read_in_interval >= hard_limit - || n_bytes_written_in_interval >= hard_limit; + return get_accounting_bytes() >= hard_limit; } /** Return true iff we have sent/received almost all the bytes we are willing @@ -747,8 +760,7 @@ hibernate_soft_limit_reached(void) if (!soft_limit) return 0; - return n_bytes_read_in_interval >= soft_limit - || n_bytes_written_in_interval >= soft_limit; + return get_accounting_bytes() >= soft_limit; } /** Called when we get a SIGINT, or when bandwidth soft limit is @@ -772,8 +784,7 @@ hibernate_begin(hibernate_state_t new_state, time_t now) hibernate_state == HIBERNATE_STATE_LIVE) { soft_limit_hit_at = now; n_seconds_to_hit_soft_limit = n_seconds_active_in_interval; - n_bytes_at_soft_limit = MAX(n_bytes_read_in_interval, - n_bytes_written_in_interval); + n_bytes_at_soft_limit = get_accounting_bytes(); } /* close listeners. leave control listener(s). */ @@ -1003,13 +1014,22 @@ getinfo_helper_accounting(control_connection_t *conn, U64_PRINTF_ARG(n_bytes_written_in_interval)); } else if (!strcmp(question, "accounting/bytes-left")) { uint64_t limit = get_options()->AccountingMax; - uint64_t read_left = 0, write_left = 0; - if (n_bytes_read_in_interval < limit) - read_left = limit - n_bytes_read_in_interval; - if (n_bytes_written_in_interval < limit) - write_left = limit - n_bytes_written_in_interval; - tor_asprintf(answer, U64_FORMAT" "U64_FORMAT, - U64_PRINTF_ARG(read_left), U64_PRINTF_ARG(write_left)); + if (strcmp(get_options()->AccountingRule, "sum") == 0) { + uint64_t total_left = 0; + uint64_t total_bytes = get_accounting_bytes(); + if (total_bytes < limit) + total_left = limit - total_bytes; + tor_asprintf(answer, U64_FORMAT" "U64_FORMAT, + U64_PRINTF_ARG(total_left), U64_PRINTF_ARG(total_left)); + } else { + uint64_t read_left = 0, write_left = 0; + if (n_bytes_read_in_interval < limit) + read_left = limit - n_bytes_read_in_interval; + if (n_bytes_written_in_interval < limit) + write_left = limit - n_bytes_written_in_interval; + tor_asprintf(answer, U64_FORMAT" "U64_FORMAT, + U64_PRINTF_ARG(read_left), U64_PRINTF_ARG(write_left)); + } } else if (!strcmp(question, "accounting/interval-start")) { *answer = tor_malloc(ISO_TIME_LEN+1); format_iso_time(*answer, interval_start_time); diff --git a/src/or/hibernate.h b/src/or/hibernate.h index 38ecb75..799b582 100644 --- a/src/or/hibernate.h +++ b/src/or/hibernate.h @@ -28,6 +28,7 @@ void consider_hibernation(time_t now); int getinfo_helper_accounting(control_connection_t *conn, const char *question, char **answer, const char **errmsg); +uint64_t get_accounting_max_total(void); #ifdef HIBERNATE_PRIVATE /** Possible values of hibernate_state */ diff --git a/src/or/or.h b/src/or/or.h index b2b0d5f..20cfa5b 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3775,6 +3775,10 @@ typedef struct { uint64_t AccountingMax; /**< How many bytes do we allow per accounting * interval before hibernation? 0 for "never * hibernate." */ + char *AccountingRule; /**< How do we determine when our AccountingMax + * has been reached? + * "max" for when in or out reaches AccountingMax + * "sum for when in plus out reaches AccountingMax */ /** Base64-encoded hash of accepted passwords for the control system. */ config_line_t *HashedControlPassword; diff --git a/src/or/router.c b/src/or/router.c index 4af8d26..96d16bb 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1080,6 +1080,7 @@ decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port) * they're confused or to get statistics. */ int interval_length = accounting_get_interval_length(); uint32_t effective_bw = get_effective_bwrate(options); + uint64_t acc_bytes; if (!interval_length) { log_warn(LD_BUG, "An accounting interval is not allowed to be zero " "seconds long. Raising to 1."); @@ -1090,8 +1091,12 @@ decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port) "accounting interval length %d", effective_bw, U64_PRINTF_ARG(options->AccountingMax), interval_length); + + acc_bytes = options->AccountingMax; + if (strcmp(options->AccountingRule, "sum") == 0) + acc_bytes /= 2; if (effective_bw >= - options->AccountingMax / interval_length) { + acc_bytes / interval_length) { new_choice = 0; reason = "AccountingMax enabled"; } diff --git a/src/or/status.c b/src/or/status.c index c4156d0..4158df5 100644 --- a/src/or/status.c +++ b/src/or/status.c @@ -145,10 +145,15 @@ log_accounting(const time_t now, const or_options_t *options) or_state_t *state = get_or_state(); char *acc_rcvd = bytes_to_usage(state->AccountingBytesReadInInterval); char *acc_sent = bytes_to_usage(state->AccountingBytesWrittenInInterval); - char *acc_max = bytes_to_usage(options->AccountingMax); + const char *acc_rule = options->AccountingRule; + uint64_t acc_bytes = options->AccountingMax; + char *acc_max; time_t interval_end = accounting_get_end_time(); char end_buf[ISO_TIME_LEN + 1]; char *remaining = NULL; + if (strcmp(acc_rule, "sum") == 0) + acc_bytes *= 2; + acc_max = bytes_to_usage(acc_bytes); format_local_iso_time(end_buf, interval_end); remaining = secs_to_uptime(interval_end - now); diff --git a/src/test/include.am b/src/test/include.am index 77c92f1..03755a4 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -34,6 +34,7 @@ src_test_test_SOURCES = \ src/test/test_logging.c \ src/test/test_microdesc.c \ src/test/test_oom.c \ + src/test/test_accounting.c \ src/test/test_options.c \ src/test/test_pt.c \ src/test/test_relaycell.c \ diff --git a/src/test/test.c b/src/test/test.c index cfbe203..d2813ed 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -1305,6 +1305,7 @@ extern struct testcase_t hs_tests[]; extern struct testcase_t nodelist_tests[]; extern struct testcase_t routerkeys_tests[]; extern struct testcase_t oom_tests[]; +extern struct testcase_t accounting_tests[]; extern struct testcase_t policy_tests[]; extern struct testcase_t status_tests[]; extern struct testcase_t routerset_tests[]; @@ -1337,6 +1338,7 @@ static struct testgroup_t testgroups[] = { { "nodelist/", nodelist_tests }, { "routerkeys/", routerkeys_tests }, { "oom/", oom_tests }, + { "accounting/", accounting_tests }, { "policy/" , policy_tests }, { "status/" , status_tests }, { "routerset/" , routerset_tests }, diff --git a/src/test/test_accounting.c b/src/test/test_accounting.c new file mode 100644 index 0000000..a1a2ec6 --- /dev/null +++ b/src/test/test_accounting.c @@ -0,0 +1,76 @@ +#include "or.h" +#include "test.h" +#define HIBERNATE_PRIVATE +#include "hibernate.h" +#include "config.h" +#define STATEFILE_PRIVATE +#include "statefile.h" + +#define NS_MODULE accounting + +#define NS_SUBMODULE limits + +/* + * Test to make sure accounting triggers hibernation + * correctly with both sum or max rules set + */ + +static or_state_t *or_state; +NS_DECL(or_state_t *, get_or_state, (void)); +static or_state_t * +NS(get_or_state)(void) +{ + return or_state; +} + +static void +test_accounting_limits(void *arg) +{ + or_options_t *options = get_options_mutable(); + time_t fake_time = time(NULL); + (void) arg; + + NS_MOCK(get_or_state); + or_state = or_state_new(); + + options->AccountingMax = 100; + options->AccountingRule = tor_strdup("max"); + + tor_assert(accounting_is_enabled(options)); + configure_accounting(fake_time); + + accounting_add_bytes(10, 0, 1); + fake_time += 1; + consider_hibernation(fake_time); + tor_assert(we_are_hibernating() == 0); + + accounting_add_bytes(90, 0, 1); + fake_time += 1; + consider_hibernation(fake_time); + tor_assert(we_are_hibernating() == 1); + + options->AccountingMax = 200; + options->AccountingRule = tor_strdup("sum"); + + accounting_add_bytes(0, 10, 1); + fake_time += 1; + consider_hibernation(fake_time); + tor_assert(we_are_hibernating() == 0); + + accounting_add_bytes(0, 90, 1); + fake_time += 1; + consider_hibernation(fake_time); + tor_assert(we_are_hibernating() == 1); + goto done; + done: + NS_UNMOCK(get_or_state); + or_state_free(or_state); +} + +#undef NS_SUBMODULE + +struct testcase_t accounting_tests[] = { + { "bwlimits", test_accounting_limits, TT_FORK, NULL, NULL }, + END_OF_TESTCASES +}; + [View Less]

1 0

[tor/master] Changes file for ticket 961
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit f15e5aedbc6298f4fad4ef6cff755591a44a7109 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Sep 23 08:57:22 2014 -0400 Changes file for ticket 961 --- changes/ticket961 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/changes/ticket961 b/changes/ticket961 new file mode 100644 index 0000000..018f265 --- /dev/null +++ b/changes/ticket961 @@ -0,0 +1,5 @@ + o Minor features: + - A new AccountingRule option lets you set whether you'd like the + … [View More]

1 0

[tor/master] Avoid frequent strcmp() calls for AccountingRule
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit 4903ab1caaf0b2631e6091b58b31eaff0c9f8424 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Sep 23 08:46:35 2014 -0400 Avoid frequent strcmp() calls for AccountingRule Generally, we don't like to parse the same thing over and over; it's best IMO to do it once at the start of the code. --- src/or/config.c | 14 ++++++++++---- src/or/hibernate.c | 9 +++++---- src/or/or.h | 9 +++++---- src/or/router.c | … [View More] 2 +- src/or/status.c | 3 +-- src/test/test_accounting.c | 4 ++-- 6 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index c7c6154..8035afb 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -126,7 +126,7 @@ static config_abbrev_t option_abbrevs_[] = { */ static config_var_t option_vars_[] = { V(AccountingMax, MEMUNIT, "0 bytes"), - V(AccountingRule, STRING, "max"), + VAR("AccountingRule", STRING, AccountingRule_option, "max"), V(AccountingStart, STRING, NULL), V(Address, STRING, NULL), V(AllowDotExit, BOOL, "0"), @@ -3109,9 +3109,15 @@ options_validate(or_options_t *old_options, or_options_t *options, "risky: they will all turn off at the same time, which may " "alert observers that they are being run by the same party."); } - if (options->AccountingRule && - strcmp(options->AccountingRule, "sum") != 0 && - strcmp(options->AccountingRule, "max") != 0) + } + + options->AccountingRule = ACCT_MAX; + if (options->AccountingRule_option) { + if (!strcmp(options->AccountingRule_option, "sum")) + options->AccountingRule = ACCT_SUM; + else if (!strcmp(options->AccountingRule_option, "max")) + options->AccountingRule = ACCT_MAX; + else REJECT("AccountingRule must be 'sum' or 'max'"); } diff --git a/src/or/hibernate.c b/src/or/hibernate.c index 6978f47..b3761cf 100644 --- a/src/or/hibernate.c +++ b/src/or/hibernate.c @@ -415,9 +415,10 @@ configure_accounting(time_t now) static uint64_t get_accounting_bytes(void) { - if (strcmp(get_options()->AccountingRule, "sum") == 0) + if (get_options()->AccountingRule == ACCT_SUM) return n_bytes_read_in_interval+n_bytes_written_in_interval; - return MAX(n_bytes_read_in_interval, n_bytes_written_in_interval); + else + return MAX(n_bytes_read_in_interval, n_bytes_written_in_interval); } /** Set expected_bandwidth_usage based on how much we sent/received @@ -434,7 +435,7 @@ update_expected_bandwidth(void) /* max_configured is the larger of bytes read and bytes written * If we are accounting based on sum, worst case is both are * at max, doubling the expected sum of bandwidth */ - if (strcmp(get_options()->AccountingRule, "sum") == 0) + if (get_options()->AccountingRule == ACCT_SUM) max_configured *= 2; #define MIN_TIME_FOR_MEASUREMENT (1800) @@ -1014,7 +1015,7 @@ getinfo_helper_accounting(control_connection_t *conn, U64_PRINTF_ARG(n_bytes_written_in_interval)); } else if (!strcmp(question, "accounting/bytes-left")) { uint64_t limit = get_options()->AccountingMax; - if (strcmp(get_options()->AccountingRule, "sum") == 0) { + if (get_options()->AccountingRule == ACCT_SUM) { uint64_t total_left = 0; uint64_t total_bytes = get_accounting_bytes(); if (total_bytes < limit) diff --git a/src/or/or.h b/src/or/or.h index 20cfa5b..54cee46 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3775,10 +3775,11 @@ typedef struct { uint64_t AccountingMax; /**< How many bytes do we allow per accounting * interval before hibernation? 0 for "never * hibernate." */ - char *AccountingRule; /**< How do we determine when our AccountingMax - * has been reached? - * "max" for when in or out reaches AccountingMax - * "sum for when in plus out reaches AccountingMax */ + /** How do we determine when our AccountingMax has been reached? + * "max" for when in or out reaches AccountingMax + * "sum for when in plus out reaches AccountingMax */ + char *AccountingRule_option; + enum { ACCT_MAX, ACCT_SUM } AccountingRule; /** Base64-encoded hash of accepted passwords for the control system. */ config_line_t *HashedControlPassword; diff --git a/src/or/router.c b/src/or/router.c index 96d16bb..dbe985a 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1093,7 +1093,7 @@ decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port) interval_length); acc_bytes = options->AccountingMax; - if (strcmp(options->AccountingRule, "sum") == 0) + if (get_options()->AccountingRule == ACCT_SUM) acc_bytes /= 2; if (effective_bw >= acc_bytes / interval_length) { diff --git a/src/or/status.c b/src/or/status.c index 4158df5..daae1d7 100644 --- a/src/or/status.c +++ b/src/or/status.c @@ -145,13 +145,12 @@ log_accounting(const time_t now, const or_options_t *options) or_state_t *state = get_or_state(); char *acc_rcvd = bytes_to_usage(state->AccountingBytesReadInInterval); char *acc_sent = bytes_to_usage(state->AccountingBytesWrittenInInterval); - const char *acc_rule = options->AccountingRule; uint64_t acc_bytes = options->AccountingMax; char *acc_max; time_t interval_end = accounting_get_end_time(); char end_buf[ISO_TIME_LEN + 1]; char *remaining = NULL; - if (strcmp(acc_rule, "sum") == 0) + if (options->AccountingRule == ACCT_SUM) acc_bytes *= 2; acc_max = bytes_to_usage(acc_bytes); format_local_iso_time(end_buf, interval_end); diff --git a/src/test/test_accounting.c b/src/test/test_accounting.c index a1a2ec6..25908e9 100644 --- a/src/test/test_accounting.c +++ b/src/test/test_accounting.c @@ -34,7 +34,7 @@ test_accounting_limits(void *arg) or_state = or_state_new(); options->AccountingMax = 100; - options->AccountingRule = tor_strdup("max"); + options->AccountingRule = ACCT_MAX; tor_assert(accounting_is_enabled(options)); configure_accounting(fake_time); @@ -50,7 +50,7 @@ test_accounting_limits(void *arg) tor_assert(we_are_hibernating() == 1); options->AccountingMax = 200; - options->AccountingRule = tor_strdup("sum"); + options->AccountingRule = ACCT_SUM; accounting_add_bytes(0, 10, 1); fake_time += 1; [View Less]

1 0

[tor/master] Merge branch 'ticket961_squashed'
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit 5e8cc766e6a4d297c66ff1afd6a5f8024607ef9c Merge: dc019b0 f15e5ae Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Sep 29 09:05:18 2014 -0400 Merge branch 'ticket961_squashed' changes/ticket961 | 5 +++ doc/tor.1.txt | 38 ++++++++++++++-------- src/or/config.c | 11 +++++++ src/or/hibernate.c | 51 ++++++++++++++++++++--------- src/or/hibernate.h | 1 + src/or/or.h | 5 +++ src/or/router.… [View More]

1 0

[tor/master] Respond with 'Command not supported' SOCKS5 reply message upon reception of unsupported request.
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit c5ad890904b68596d1081ce92307f57e67a5e3e4 Author: rl1987 <rl1987(a)sdf.lonestar.org> Date: Sun Sep 21 19:17:44 2014 +0300 Respond with 'Command not supported' SOCKS5 reply message upon reception of unsupported request. --- changes/bug12971 | 5 +++++ src/or/buffers.c | 20 ++++++++++++++++++++ src/test/test_socks.c | 18 ++++++++++++++---- 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/changes/bug12971 b/changes/bug12971 new file mode 100644 … [View More]index 0000000..e548bbf --- /dev/null +++ b/changes/bug12971 @@ -0,0 +1,5 @@ + o Bugfixes: + - Handle unsupported SOCKS5 requests properly by responding with + 'Command not supported' reply message before closing a TCP connection + to the user. Fixes bug 12971. + diff --git a/src/or/buffers.c b/src/or/buffers.c index 033f862..edb64ec 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -55,6 +55,9 @@ * forever. */ +static void send_socks5_error(socks_request_t *req, + socks5_reply_status_t reason); + static int parse_socks(const char *data, size_t datalen, socks_request_t *req, int log_sockstype, int safe_socks, ssize_t *drain_out, size_t *want_length_out); @@ -1831,6 +1834,21 @@ fetch_ext_or_command_from_evbuffer(struct evbuffer *buf, ext_or_cmd_t **out) } #endif +/** Create a SOCKS5 reply message with reason in its REP field and + * have Tor send it as error response to req. + */ +static void +send_socks5_error(socks_request_t *req, + socks5_reply_status_t reason) +{ + req->replylen = 10; + memset(req->reply,0,10); + + req->reply[0] = 0x05; // VER field. + req->reply[1] = reason; // REP field. + req->reply[3] = 0x01; // ATYP field. +} + /** Implementation helper to implement fetch_from_*_socks. Instead of looking * at a buffer's contents, we look at the datalen bytes of data in * data. Instead of removing data from the buffer, we set @@ -1966,6 +1984,8 @@ parse_socks(const char *data, size_t datalen, socks_request_t *req, req->command != SOCKS_COMMAND_RESOLVE && req->command != SOCKS_COMMAND_RESOLVE_PTR) { /* not a connect or resolve or a resolve_ptr? we don't support it. */ + send_socks5_error(req,SOCKS5_COMMAND_NOT_SUPPORTED); + log_warn(LD_APP,"socks5: command %d not recognized. Rejecting.", req->command); return -1; diff --git a/src/test/test_socks.c b/src/test/test_socks.c index 20f58ca..2b8f824 100644 --- a/src/test/test_socks.c +++ b/src/test/test_socks.c @@ -143,23 +143,33 @@ test_socks_5_unsupported_commands(void *ptr) ADD_DATA(buf, "\x05\x02\x00\x01\x02\x02\x02\x01\x01\x01"); tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks),==, -1); - /* XXX: shouldn't tor reply 'command not supported' [07]? */ + + tt_int_op(5,==,socks->socks_version); + tt_int_op(10,==,socks->replylen); + tt_int_op(5,==,socks->reply[0]); + tt_int_op(SOCKS5_COMMAND_NOT_SUPPORTED,==,socks->reply[1]); + tt_int_op(1,==,socks->reply[3]); buf_clear(buf); socks_request_clear(socks); /* SOCKS 5 Send unsupported UDP_ASSOCIATE [03] command */ - ADD_DATA(buf, "\x05\x03\x00\x01\x02"); + ADD_DATA(buf, "\x05\x02\x00\x01"); tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks),==, 0); tt_int_op(5,==, socks->socks_version); tt_int_op(2,==, socks->replylen); tt_int_op(5,==, socks->reply[0]); - tt_int_op(2,==, socks->reply[1]); + tt_int_op(0,==, socks->reply[1]); ADD_DATA(buf, "\x05\x03\x00\x01\x02\x02\x02\x01\x01\x01"); tt_int_op(fetch_from_buf_socks(buf, socks, get_options()->TestSocks, get_options()->SafeSocks),==, -1); - /* XXX: shouldn't tor reply 'command not supported' [07]? */ + + tt_int_op(5,==,socks->socks_version); + tt_int_op(10,==,socks->replylen); + tt_int_op(5,==,socks->reply[0]); + tt_int_op(SOCKS5_COMMAND_NOT_SUPPORTED,==,socks->reply[1]); + tt_int_op(1,==,socks->reply[3]); done: ; [View Less]

1 0

[tor/master] Rename socks5 error code setting function again
by nickm＠torproject.org 29 Sep '14

29 Sep '14

commit fcebc8da95af5ccb28d715a6897566ea1c190a03 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Sep 29 09:17:20 2014 -0400 Rename socks5 error code setting function again I'd prefer not to use the name "send" for any function that doesn't really send things. --- src/or/buffers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index edb64ec..d174f81 100644 --- a/src/or/buffers.c +++ b/src/or/… [View More]

1 0