[tor-bugs] #7157 [Tor]: "Low circuit success rate %u/%u for guard %s=%s."

[Tor Bug Tracker & Wiki]

#7157: "Low circuit success rate %u/%u for guard %s=%s."
-----------------------------------------+----------------------------------
 Reporter:  arma                         |          Owner:                    
     Type:  enhancement                  |         Status:  needs_review      
 Priority:  normal                       |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor                          |        Version:                    
 Keywords:  tor-client, MikePerry201212  |         Parent:  #5456             
   Points:                               |   Actualpoints:  19                
-----------------------------------------+----------------------------------

Comment(by nickm):

 Since it's one branch, I'll review the branch in one place.

 This is part one of the review: I need to take a break now.  It covers up
 to but
 not including 721f7e375114abfc, with a little skipping around in in the
 diff
 that I've been reading side-by-side to see what stays and what gets fixed.

 Part one begins:

 pathbias_get_dropguards -- I think it's supposed to give a boolean, bxut
 it's
 taking its input in range 0..100 and dividing it by 100.0.  Assuming I'm
 right about it being a boolean, you need to chance its type in config.c to
 "autobool", so that setting it to "auto" can mean "use the value from the
 consensus".

 I think the comment on pathbias_get_scale_factor might be wrong; does the
 code still refrain from scaling in the event of truncation?  It doesn't
 appear to be the case.


 In entry_guards_update_state, you're using e->circ_attempts in a boolean
 context, which compares it to 0.0; that's potentially yucky.  You
 shouldn't
 do equality checks on doubles.


 Do we verify that scale_factor strictly less than mult_factor?  It looks
 like
 in ef1b830ef8d751172ebe5 we removed the requirement, making it so
 scale_factor == mult_factor is allowed.

 pathbias_count_successful_close (and others that rely on
 entry_guard_get_by_id_digest) is going to hit its LD_BUG whenever we have
 a
 node stop being a guard after we open a circuit through it.

 I'm concerned about what happens with circuits that are around when Tor
 dies.
 Do they count as successes, or failures, or not at all?  I think right now
 they're counted as never having closed sucessfully; is that so?  It seems
 kind of odd to compare "successfully closed circuits" to "circuits for
 which
 we've been the first hop."

 Don't use memcmp.  Use fast_memeq or tor_memeq or fast_memcmp as
 appropriate.

 This business with counting streams successfully sent on a circuit, using
 end
 stream reasons to say which are okay and which aren't, and so on... is it
 specified anywhere ?  I'm not seeing it in proposal 209.

 The comment on any_streams_succeeded seems wrong; it's not SOCKS streams,
 it's any client streams, right?

 Re 7f8cbe389d095f673bfc03437e1f7521abae698b: It is indeed redundant.
 Looks
 like you fixed that in c3028edba6f8474175a59ad22dd0d3ca23c60f85.  Those
 are
 two good ones to squash before merge.

 "and it shat itself" ?

 Re the XXX in connection_ap_process_end_not_open: 'digest' is hard enough
 to
 spoof that if the digest is correct, you can assume you're getting the
 right
 cell with fairly high probability.

 Gcc gives me these warnings:

 {{{
 src/or/circuitbuild.c:1020: warning: no previous prototype for
 ‘pathbias_get_warn_rate’
 src/or/circuitbuild.c: In function ‘pathbias_get_dropguards’:
 src/or/circuitbuild.c:1058: warning: implicit conversion shortens 64-bit
 value into a 32-bit value
 src/or/circuitbuild.c: In function ‘entry_guard_inc_circ_attempt_count’:
 src/or/circuitbuild.c:1669: warning: cast from function call of type
 ‘double’ to non-matching type ‘int’
 src/or/circuitbuild.c:1688: warning: cast from function call of type
 ‘double’ to non-matching type ‘int’
 src/or/circuitbuild.c:1706: warning: cast from function call of type
 ‘double’ to non-matching type ‘int’
 src/or/circuitbuild.c:1725: warning: cast from function call of type
 ‘double’ to non-matching type ‘int’
 make[1]: *** [src/or/circuitbuild.o] Error 1
 cc1: warnings being treated as errors
 src/or/connection_edge.c: In function
 ‘connection_ap_handshake_socks_reply’:
 src/or/connection_edge.c:2191: warning: format ‘%ld’ expects type ‘long
 int’, but argument 5 has type ‘uint64_t’
 make[1]: *** [src/or/connection_edge.o] Error 1
 cc1: warnings being treated as errors
 src/or/entrynodes.c: In function ‘entry_guards_update_state’:
 src/or/entrynodes.c:1197: warning: comparing floating point with == or !=
 is unsafe

 }}}

 Part one ends.  Next: review 721f7e375114abfcb1 and on.

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