Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#7902 closed defect (fixed)

connection_ap_process_end_not_open() does not handle END_STREAM_REASON_DONE correctly

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

< oftc_must_be_destroyed> I found some nice stucking for streams
< oftc_must_be_destroyed> onnection_ap_process_end_not_open() counts every reasons without excpetion assume it will translated to socks answer as failure code. but!
< oftc_must_be_destroyed> END_STREAM_REASON_DONE translated to SOCKS5_SUCCEEDED
< oftc_must_be_destroyed> did you got it?
< oftc_must_be_destroyed> exit closed conn but socks client assume it successly open.
< oftc_must_be_destroyed> stuck
< oftc_must_be_destroyed> connection_ap_process_end_not_open should to count END_STREAM_REASON_DONE specificaly and modify it to some reason about error.
< asn_> oftc_must_be_destroyed: IIUC connection_ap_process_end_not_open() must consider END_STREAM_REASON_DONE as an error, but it doesn't.
< asn_> oftc_must_be_destroyed: and instead of killing the SOCKS stream, it's marked as established.
< asn_> oftc_must_be_destroyed: or something.
< oftc_must_be_destroyed> well it marking for close anyway
< oftc_must_be_destroyed> but client sees success
< oftc_must_be_destroyed> socks client
< oftc_must_be_destroyed> and nexit conn closed. suddenly
< oftc_must_be_destroyed> exit answer with END_STREAM_REASON_DONE if eof or EPIPE
< oftc_must_be_destroyed> it's better to count END_STREAM_REASON_DONE as END_STREAM_REASON_MISC while process it by connection_ap_process_end_not_open

Child Tickets

Change History (7)

comment:1 Changed 7 years ago by asn

Milestone: Tor: unspecifiedTor: 0.2.4.x-final

< oftc_must_be_destroyed> and if rh->length zero then reason zero too. it need to count such case too, no need to break mood of socks client by random zeroes. outlaw it too.

comment:2 Changed 7 years ago by nickm

Status: newneeds_review

Seems like the right fix here for DONE is to change the function that converts stream-end reasons into socks reasons.

Branch "bug7902" is ready for review.

comment:3 Changed 7 years ago by nickm

Added another tweak to update control_reason to match reason.

comment:4 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged into master after review by Andrea.

comment:5 Changed 7 years ago by mikeperry

Hrmm. I have two questions here:

  1. Do we really want to retry these streams on new circuits? Changing the reason code to MISC in connection_ap_process_end_not_open() seems to cause us to do so.
  1. I think this means I should add an endreason == END_STREAM_REASON_DONE to the path bias check at the beginning of connection_ap_handshake_socks_reply(). I think we want to count such connection refused cases as valid use, as the client is *not* going to retry them on a new circuit at that point.

comment:6 Changed 7 years ago by nickm

Changing the reason code to MISC seems plausible to me, since that can only happen for streams where the end point screwed up and sent no reason at all. That shouldn't happen, and seems to be retriable at a different exit.

I think that counting "DONE" as valid use might be okay.

comment:7 Changed 7 years ago by mikeperry

Hrm.. If the only case where we can get a END_STREAM_REASON_DONE in connection_ap_handshake_socks_reply() is something that shouldn't happen, maybe I should just leave the path bias state as-is so we can probe the circuit upon teardown. I found quite a few cases where tagging could induce codepaths that shouldn't happen while looking at what sorts of reason codes come back..

Note: See TracTickets for help on using tickets.