Opened 2 years ago

Last modified 5 weeks ago

#26366 needs_revision defect

Possible duplicated logic in connection_edge_finished_connecting() and connection_exit_connect()

Reported by: ahf Owned by: neel
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-hs
Cc: neel Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

There's some possible duplicated logic in connection_edge_finished_connecting() and connection_exit_connect():

When conn->state = EXIT_CONN_STATE_OPEN is set we have some duplicated code on when to begin writing. There is a comment that might be relevant around some Windows logic in the switch/case block around the result variable in connection_exit_connect().

Child Tickets

Change History (5)

comment:1 Changed 3 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:2 Changed 3 months ago by neel

Status: assignedneeds_review

comment:3 Changed 2 months ago by asn

Reviewer: catalyst

comment:4 in reply to:  2 Changed 7 weeks ago by catalyst

Status: needs_reviewneeds_information

Replying to neel:

PR: https://github.com/torproject/tor/pull/1781

Thanks for the patch!

There are a few things I would like to know more about the existing code. Maybe ahf knows?

  • There's an asymmetry between connection_edge_finished_connecting() and connection_exit_connect() in terms of how they call connection_watch_events(). connection_edge_finished_connecting() sets READ_EVENT, then separately calls connection_start_writing() if there's queued data, while connection_exit_connect() checks for queued data first, then picks READ_EVENT|WRITE_EVENT or READ_EVENT as appropriate. Maybe this difference is insignificant in practice?
  • For exit connections, is it even possible for connected_cell_format_payload() to return -1? It looks like it could if the address family is neither AF_INET nor AF_INET6, but how can this happen? Shouldn't previously-run code already have enforced? If so, why can't we use an assertion here?

I made additional suggestions on the pull request.

comment:5 Changed 5 weeks ago by ahf

Status: needs_informationneeds_revision
Note: See TracTickets for help on using tickets.