Opened 6 years ago

Last modified 22 months ago

#7678 new defect

Check if a stream could send a cell but the circuit hasn't asked it to

Reported by: arma Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.3.25
Severity: Normal Keywords: tor-relay needs-insight bugproofing protocol
Cc: Actual Points:
Parent ID: Points: 4
Reviewer: Sponsor:

Description

Our #snackbug friend may have encountered a situation where somehow a circuit decides to never check again whether a stream had something to say, so when the circuit could send more, it doesn't, until the next time it gets told to check if any streams are ready to send.

For example, the exit relay waits with its 'connected' relay cell until a new stream is created, and then it suddenly notices that it had one it wanted to send.

Seems like we should sort out exactly what the invariant is supposed to be ("if the package window is at least this, and the cell queue is less than this water mark, then it should never be the case that a stream on that circuit has something ready to send but hasn't sent it"), and check it in some smart places and log if it's violated.

Child Tickets

Attachments (2)

paste_752dcf90 (12 bytes) - added by cypherpunks 6 years ago.
No more stall
paste_82785ae8 (12 bytes) - added by cypherpunks 6 years ago.
v2

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by cypherpunks

Priority: normalmajor

Some dude asked to attach this patch. Dude said: "ibid or related. No more stall", and asked to raise priority.

comment:2 Changed 6 years ago by cypherpunks

Milestone: Tor: unspecifiedTor: 0.2.3.x-final
Status: newneeds_review
Version: Tor: 0.2.3.25

comment:3 Changed 6 years ago by cypherpunks

Type: enhancementdefect

comment:4 Changed 6 years ago by cypherpunks

v2 attached. better version. probably.

Changed 6 years ago by cypherpunks

Attachment: paste_752dcf90 added

No more stall

Changed 6 years ago by cypherpunks

Attachment: paste_82785ae8 added

v2

comment:5 Changed 6 years ago by cypherpunks

Milestone: Tor: 0.2.3.x-finalTor: unspecified
Priority: majornormal
Status: needs_reviewnew
Type: defectenhancement
Version: Tor: 0.2.3.25

comment:6 Changed 6 years ago by aagbsn

Milestone: Tor: unspecifiedTor: 0.2.3.x-final
Priority: normalmajor
Status: newneeds_review
Type: enhancementdefect
Version: Tor: 0.2.3.25

comment:7 Changed 6 years ago by rransom

Status: needs_reviewnew

He/she/it replaced the patches with “nobody care”, so there is no longer any code to review on this ticket. (I hope someone has copies of them somewhere.)

comment:8 Changed 6 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:9 Changed 6 years ago by nickm

Nope, I don't have the original patches here.

My first guess of the right rule is: an output buffer should never be empty when the circuitmux targeting that connection has any cells to send on that connection. (That's because when an output buffer becomes empty, we are supposed to check for cells on the cmux, and when the cmux goes from empty to nonempty, we are supposed to prime a cell onto the output buffer immediately if it was empty.)

comment:10 Changed 6 years ago by nickm

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

comment:11 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:12 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:13 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:14 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:15 Changed 22 months ago by nickm

Keywords: needs-insight bugproofing protocol added
Points: 4
Severity: Normal
Note: See TracTickets for help on using tickets.