Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#4655 closed defect (fixed)

Try clearing stream-isolation state on newly opened rendezvous circs

Reported by: rransom Owned by: rransom
Priority: Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When a newly created general-purpose circuit is opened, we try to attach streams to it, and if we can't, we clear the circuit's stream-isolation state and try attaching streams again. We tried to do that for rendezvous circuits, too, but at the wrong time (when a rendezvous point was established), and it broke newly created rend circs.

We should try clearing a rend circ's stream-isolation state when (a) it enters ‘C_REND_JOINED’ state and (b) we don't have any streams to attach to it.

Child Tickets

Change History (9)

comment:1 Changed 9 years ago by rransom

Status: newneeds_review

See my bug4655-v2 ( https://git.torproject.org/rransom/tor.git bug4655-v2 ) branch for a fix.

comment:2 Changed 9 years ago by nickm

Looks okay to me.

Should the "XXXX023 This is a pretty brute-force approach" comment die?

And should the connection_ap_attach_pending() calls in circuit_try_attaching_streams() get a similar comment?

(Also, because I know you sometimes share my penchant for pedantry, you might in the future consider that "return non-zero if X" is a less bullet-proof function specification than "return non-zero iff X". But it's also okay to leave it as "if", and assume that everybody who cares will assume you meant "iff".)

comment:3 in reply to:  2 Changed 9 years ago by rransom

Replying to nickm:

Looks okay to me.

Should the "XXXX023 This is a pretty brute-force approach" comment die?

Yes. The only way to avoid trying to attach all streams to circuits there is to keep a list of streams intended to connect to that circuit's hidden service; the efficiency gain there is not worth the added complexity all over. I've pushed a commit to remove the comment.

And should the connection_ap_attach_pending() calls in circuit_try_attaching_streams() get a similar comment?

No. For general-purpose circuits, we must try to attach all streams to circuits.

(Also, because I know you sometimes share my penchant for pedantry, you might in the future consider that "return non-zero if X" is a less bullet-proof function specification than "return non-zero iff X". But it's also okay to leave it as "if", and assume that everybody who cares will assume you meant "iff".)

Oops. It definitely should be “iff”. I've pushed a fixup commit for this.

comment:4 Changed 9 years ago by nickm

Strange; I went to merge it and test compilation, but I got:

circuituse.c: In function ‘circuit_try_clearing_isolation_state’:
circuituse.c:1061: error: ‘can_try_clearing_isolation’ undeclared (first use in this function)
circuituse.c:1061: error: (Each undeclared identifier is reported only once
circuituse.c:1061: error: for each function it appears in.)
circuituse.c:1061: error: ‘tried_clearing_isolation’ undeclared (first use in this function)

comment:5 Changed 9 years ago by nickm

Please review the squash! commit I added on branch bug4655-v2 in my public repo. If you like it, I'll squash and merge.

comment:6 in reply to:  5 Changed 9 years ago by rransom

Replying to nickm:

Please review the squash! commit I added on branch bug4655-v2 in my public repo. If you like it, I'll squash and merge.

Looks good. Thanks for fixing that!

comment:7 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks for the review; squashing and merging.

comment:8 Changed 8 years ago by nickm

Keywords: tor-hs added

comment:9 Changed 8 years ago by nickm

Component: Tor Hidden ServicesTor
Note: See TracTickets for help on using tickets.