[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #7157 [Tor]: "Low circuit success rate %u/%u for guard %s=%s."
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs