[tor-bugs] #9321 [Tor]: Load balance right when we have higher guard rotation periods

[Tor Bug Tracker & Wiki]

#9321: Load balance right when we have higher guard rotation periods
-------------------------+-------------------------------------------------
     Reporter:  arma     |      Owner:
         Type:  project  |     Status:  needs_review
     Priority:  normal   |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor      |    Version:
   Resolution:           |   Keywords:  needs-proposal, tor-auth, tor-
Actual Points:           |  client, 026-triaged-1
       Points:           |  Parent ID:  #11480
-------------------------+-------------------------------------------------

Comment (by teor):

 Review of `git diff 390ec9154c58d6c7acaecd5e575adf575ab2f1fe^
 asn/bug9321_draft` by reading through the modified code:

 '''Impacts on other changes'''

 If `dirserv_read_guardfraction_file` can handle reading an empty (zero-
 length) file, and we merge #13111 in 0.2.6 as well, this code:
 {{{
 +      if (file_status(options->GuardfractionFile) != FN_FILE) {
 +        REJECT("GuardfractionFile set but not a file? Failing");
 +      }
 +
 +      dirserv_read_guardfraction_file(options->GuardfractionFile, NULL);
 }}}

 will need to be updated to:
 {{{
 +      if (file_status(options->GuardfractionFile) != FN_FILE &&
 file_status(options->GuardfractionFile) != FN_EMPTY) {
 +        REJECT("GuardfractionFile set but not a file? Failing");
 +      }
 +
 +      dirserv_read_guardfraction_file(options->GuardfractionFile, NULL);
 }}}

 Or maybe we should just fail if the guardfraction file is truncated to
 zero-length?


 '''Minor Picky Things'''

 Log Severities:

 I'd downgrade `log_warn(LD_GENERAL, "%s: Guardfraction weight %f instead
 of %f (%s)",` to a `log_debug`, given it may be called several thousand
 times.

 Integer ranges:
 I think this code could cause an integer overflow on insane inputs when
 `SIZEOF_LONG == SIZEOF_INT`:
 {{{
 version =
 +        (unsigned int) tor_parse_long(line->value, 10, 0, UINT_MAX,
 &num_ok, NU
 LL);
 }}}
 `tor_parse_long` function prototype is:
 {{{
 long
 tor_parse_long(const char *s, int base, long min, long max,
                int *ok, char **next)
 }}}
 I suggest we use `tor_parse_long(line->value, 10, 0, INT_MAX, ...`.

 Capitalisation:
 * `UseGuardFraction` vs `GuardfractionFile` vs `Guardfraction:` in log
 messages
  * I suggest `GuardFractionFile` and `GuardFraction:` (all Fs
 capitalised), but I don't care much either way about `Guard[Ff]raction:`

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9321#comment:32>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online