Wrong failure report when closing parallel intro points
|Reported by:||dgoulet||Owned by:|
|Severity:||Keywords:||tor-hs, 025-backport, 024-backport, 2016-bug-retrospective|
Description (last modified by dgoulet)
This is a part of the larger bug #13664.
When opening intro points, tor does that in parallel with all the possible node in the descriptor it has fetched and cached. Once an intro points successfully acknowledged, a close "all parallel pending circuit(s)" is triggered and here comes trouble. Here is the chain of actions:
rend_client_introduction_acked() |- rend_client_close_other_intros() |- circuit_mark_for_close(c, END_CIRC_REASON_TIMEOUT)
So far so good, notice the reason being a timeout. That would be "OK" because the timeout reason does not remove the intro point from the rend cache object intro list. It's still wrong but at least the intro point would have been kept.
That mark for close function is only called under certain conditions and one is that CIRCUIT_IS_ORIGIN(c) is true. Now in circuit_mark_for_close(), a check is done on exactly that condition and if true, the reason is changed from END_CIRC_REASON_TIMEOUT to END_CIRC_REASON_NONE.
Even if the intro point purpose was CIRCUIT_PURPOSE_C_INTRODUCING or CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT, in both cases since the reason was changed, a generic failure or unreachable error is thrown. Both will end up removing the intro points from the cache object (the unreachable error will try 3 times to connect before though). You can found this in rend_client_report_intro_point_failure().
What I think should happen here is to:
1) add a reason for closing a parellel intro circuit. Something like END_CIRC_REASON_IP_PARALLEL. Handling that reason to simply close the circuit without changing anything of the rend cache state.
2) Don't change the reason if it's an intro point circuit and CIRCUIT_IS_ORIGIN(c) resolved yes because the reason indicates the type of intro point failure that rend_client_report_intro_point_failure() uses for specific behaviour.
Change History (13)
comment:4 Changed 2 years ago by dgoulet
- Keywords 024-backport added
- Status changed from new to needs_review
comment:6 Changed 2 years ago by nickm
- Milestone changed from Tor: 0.2.6.x-final to Tor: 0.2.5.x-final
comment:9 Changed 22 months ago by saint
- Resolution set to fixed
- Status changed from needs_review to closed
comment:10 Changed 22 months ago by dgoulet
- Resolution fixed deleted
- Status changed from closed to reopened
comment:12 Changed 19 months ago by nickm
- Milestone changed from Tor: 0.2.5.x-final to Tor: 0.2.6.x-final
- Resolution set to fixed
- Status changed from reopened to closed