[tor-bugs] #26366 [Core Tor/Tor]: Possible duplicated logic in connection_edge_finished_connecting() and connection_exit_connect()

[Tor Bug Tracker & Wiki]

#26366: Possible duplicated logic in connection_edge_finished_connecting() and
connection_exit_connect()
--------------------------+-----------------------------------
 Reporter:  ahf           |          Owner:  neel
     Type:  defect        |         Status:  needs_information
 Priority:  Medium        |      Milestone:  Tor: unspecified
Component:  Core Tor/Tor  |        Version:  Tor: unspecified
 Severity:  Normal        |     Resolution:
 Keywords:  tor-hs        |  Actual Points:
Parent ID:                |         Points:
 Reviewer:  catalyst      |        Sponsor:
--------------------------+-----------------------------------
Changes (by catalyst):

 * status:  needs_review => needs_information


Comment:

 Replying to [comment:2 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26366#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online