circuit: Cleanup an HS circuit when it is being re-purposed
Mike found out that when an IP/RP circuit fails to build in the right amount of time (for instance through circuit_expire_building()), it is re-purposed to become a measurement circuit.
The issue is that those HS circuits are set in the HS circuitmap and have an hs_ident or rend_data set to them that should really not linger in the circuit object if the circuit is not an HS one anymore.
Offenders: circuit_build_times_mark_circ_as_measurement_only() and pathbias_send_usable_probe().
Solution:
circuit_change_purpose() is probably the right place to make a callback within the HS subsystem specific to cleaning up a circuit for a purpose change. I think we need a new function that specifically does that and not use hs_circ_cleanup() since it won't remove the ident.
Lingering circuits in the HS circuitmap is bad and this bug could probably explain some of the issues we had with clients unable to establish connections because the IP auth key wouldn't match the one in the circuit ident.
I strongly believe this should be backported up to 0.3.5 at the very least.
Activity
changed milestone to %Tor: 0.3.5.x-final
added 035-backport 040-backport actualpoints::3 component::core tor/tor milestone::Tor: 0.3.5.x-final network-team-roadmap-july owner::mikeperry points::3 postfreeze-ok priority::high resolution::fixed reviewer::asn severity::normal sponsor::27-must status::closed tor-hs-reachability type::defect version::tor 0.3.2.1-alpha labels
Trac:
Parent: N/A to #29995 (moved)
Ok I did the simplest narrow fix, based on what dgoulet said in the description: https://github.com/torproject/tor/pull/931
Someone who is deeply familiar with both v2 and v3 state should review this. It's simple, and I think it's ok, but I don't know both v2 and v3 code bases well enough to really be sure.
Trac:
Status: assigned to needs_review
LGTM!
I have a slight revision in https://github.com/torproject/tor/pull/969, because I opted for not calling the new functions
hs_circ_free()since*_free()functions have a very specific meaning in Tor (they free and NULL their input). Instead I hijackedhs_circ_cleanup()which seamed a better target.If you like, let's merge_ready.
Trac:
Status: needs_review to needs_revision-
Ok, dgoulet said it should be a different function, but I figure you guys can hash that out. I'm fine with asn's https://github.com/torproject/tor/pull/969.
Trac:
Status: needs_revision to merge_ready -
dgoulet: what do you think about what I did earlier in that case? (https://github.com/torproject/tor/pull/931/commits/bee7979e87d51735d2d5f746d69761d89d7dd928) mikeperry: yeah something like that but it needs to call hs_circ_cleanup() for v3 only and the v2 stuff needs to ideally not be done there... so I think the place it needs to go is hs_common.c and then call the v2 or v3 specialized calls
Hrmm, I am not sure how to differentiate the v2 vs v3 calls+data. What is the convention for that?
-
This is needed for Design a first useful padding machine (hiding c... (#28634 - moved)#28780 (moved) for 0.4.1.
Trac:
Parent: #29995 (moved) to #28634 (moved) 
Replying to mikeperry:
Hrmm, I am not sure how to differentiate the v2 vs v3 calls+data. What is the convention for that?
Within the circuit, v2 has a
rend_dataand v3 hashs_ident. One circuit can not have both. If none exist also, it is not an HS circuit.-
Taking over as part of s27 since we believe #28970 (moved) and #27471 (moved) will get fixed with this.
Trac:
Owner: mikeperry to dgoulet
Status: needs_revision to assigned -
dgoulet - So far we have not experienced any obvious reliability issues due to this bug while testing #28634 (moved) (which uses #28780 (moved), which changes client-side INTRO and REND circ purposes to PADDING circs).
I think this means that the hs_circuitmap reliability issues do not apply to client-side intros?
My guess is it is because clients only seem to use this map for the 128bit rend cookie, and not any permanent mappings that could be invalid later. In which case, this is "just" a client-side memory leak of this state with #28780 (moved), and not a reliability problem?
-
New version. It is the same from the initial branches but with a v2/v3 separation:
Branch:
ticket29034_041_01PR: https://github.com/torproject/tor/pull/1048Trac:
Status: assigned to needs_review -
Should this get backported due to its significance to bugs #28970 (moved) and #27471 (moved) ? This way we learn faster if those bugs get fixed?
-
Ah yes I think so. Here is an 035 branch:
ticket29034_035_01https://github.com/torproject/tor/pull/1053 -
OK let's get this merged. We still don't know if it helps against #28970 (moved) and #27471 (moved) but the unittests show that it will help with the circpad issues.
Trac:
Status: needs_review to merge_ready 
Trac:
Parent: #28634 (moved) to N/A-
We should not backport this; instead we should backport c525135dac354892a45ad3d2f6de9450d393f09f combined with the changes file for this branch.
(Edited to add: See #30773 (moved) for rationale)
-
Here is the new backport branch for 0.3.5+: https://github.com/torproject/tor/pull/1077
Trac:
Status: needs_revision to needs_review
