[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #12585 [Tor]: Implement new option SocksSocket

#12585: Implement new option SocksSocket
-----------------------------+--------------------------------
     Reporter:  ioerror      |      Owner:
         Type:  enhancement  |     Status:  needs_revision
     Priority:  normal       |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor          |    Version:  Tor: unspecified
   Resolution:               |   Keywords:  026-triaged-1
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+--------------------------------

Comment (by andrea):

 Code review of second draft (SocksSocket-009.patch) of ioerror's
 SOCKS-over-AF_UNIX patch:

  - The tor_addr_from_sock_addr() and tor_addr_make_af_unix() functions
 look okay now.

  - The change to tor_addr_to_str() looks okay to me.

  - All the config.c changes look okay to me.

  - Changes to entry_connection_new() and connection_free_() look okay.

  - I still don't like this duplicated code in
 check_location_for_socks_unix_socket() vs.
 check_location_for_unix_socket(); they should be combined and if we ever
 want the behavior to substantively differ we can split them then.

  - Since I don't see any changes to the connection_listener_new() part of
 the patch, all my previous comments, reproduced below, apply.  In
 particular, the near-identical code blocks have to go.
    * Perhaps is_tcp should be renamed, since obviously an AF_UNIX socket
 is not TCP.  Is the relevant property actually SOCK_STREAM (vs. SOCK_DGRAM
 in the CONN_TYPE_AP_DNS_LISTENER case)?
    * Why are you adding log_warn(LD_NET,"accept() fails after this...")
 when no actual error condition appears to be occurring in the
 AF_INET/AF_INET6 path?
    * The logic in the "if (listensockaddr->sa_family == AF_UNIX && type !=
 CONN_TYPE_CONTROL_LISTENER)" branch is very similar to the existing logic
 for the CONN_TYPE_CONTROL_LISTENER case.  They should be combined to avoid
 duplicated code.

  - Changes check_sock_addr() and connection_handle_listener_read() look
 okay.

  - All changes to main.c, or.h and relay.c look okay to me.

  - Changes to connection_edge.c look okay.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12585#comment:30>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs