Opened 3 years ago

Closed 2 years ago

Last modified 18 months ago

#13698 closed defect (fixed)

Wrong failure report when closing parallel intro points

Reported by: dgoulet Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs, 025-backport, 024-backport, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR

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.

Thoughts?

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by dgoulet

Description: modified (diff)

comment:2 Changed 3 years ago by nickm

Milestone: Tor: 0.2.6.x-final

comment:3 Changed 3 years ago by nickm

Keywords: 025-backport added

From IRC discussion:

re 1) Make it a negative reason, and call it something like END_CIRC_REASON_IP_NOW_REDUNDANT.

re 2) Instead of that, have the stuff that currently checks reason when it's about to call rend_client_report_intro_point_failure check orig_reason instead. (I worry that removing the code that changes reason could have unintended side-effects.)

comment:4 Changed 3 years ago by dgoulet

Keywords: 024-backport added
Status: newneeds_review

Personal repo: https://git.torproject.org/user/dgoulet/tor.git

You can find the patch for this in bug13698_024_v1 which apply on 0.2.4 and can be backport up to master.

comment:5 Changed 3 years ago by nickm

lgtm. There needs to be a bug number referenced in the changes file, but I can do that.

comment:6 Changed 3 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.5.x-final

I've added an extra commit in branch "bug13698_024_v1" in my own public repository to edit the changes file, and merged into master. If that works out, we can maybe backport.

comment:7 Changed 3 years ago by dgoulet

I think that might be a typo in the Changefile:

bugfix on 0.0.6rc2.

Do you want me to merge your fix in my branch in one single commit and you could simply merge the _v2 ?

comment:8 Changed 3 years ago by dgoulet

Keywords: SponsorR added

comment:9 Changed 2 years ago by saint

Resolution: fixed
Status: needs_reviewclosed

The fix for this has been merged. Closing.

comment:10 Changed 2 years ago by dgoulet

Resolution: fixed
Status: closedreopened

Still flag for 025-backport. Keeping it open for nickm to notice at some point :).

comment:11 Changed 2 years ago by nickm

Keywords: SponsorR removed
Sponsor: SponsorR

Bulk-replace SponsorR keyword with SponsorR sponsor field in Tor component.

comment:12 Changed 2 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Resolution: fixed
Status: reopenedclosed

This is fixed in 0.2.6; not going to backport to 0.2.5. Please upgrade to 0.2.6 or later if you need a fix for this.

comment:13 Changed 18 months ago by nickm

Keywords: 2016-bug-retrospective added

Mark more tickets for bug retrospective based on hand-review of changelogs from 0.2.5 onwards.

Note: See TracTickets for help on using tickets.