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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#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 ioerror):

 Replying to [comment:13 andrea]:
 > Initial code review of ioerror's SOCKS-over-AF_UNIX patch:

 Thank you for the review! I really appreciate it!

 >
 >  - In tor_addr_from_sockaddr(), you're calling a helper function
 >    tor_addr_make_af_unix(), and then returning -1.  The existing
 convention
 >    for that function (which is not documented in its comment, but should
 be),
 >    is to return -1 in the error case of an unknown address family, and 0
 >    in the success case.  The line after tor_addr_make_af_unix(a); should
 >    be return 0, I think.

 Sure, I made it return 0;

 >
 >  - The tor_addr_make_af_unix() helper itself looks okay to me, but this
 >    function is only called in one place, from within address.c.  It
 should
 >    be declared static at the top of address.c rather than in the global
 >    namespace in address.h.


 I changed '''void tor_addr_make_af_unix()''' to '''static
 tor_addr_make_af_unix()''' and removed the reference to it in
 '''address.h'''.

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

 OK.

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

 OK.

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

 OK.

 >  - As far as I can tell, the new check_location_for_socks_unix_socket()
 >    function differs from the existing check_location_for_unix_socket()
 >    only in the text of the warning message emitted.  These should be
 >    refactored to avoid duplicated logic.
 >

 I figured that we will want to change the logic internally as well later -
 for example - this is where we might check the user/group restrictions.
 That was part of the grounding for it in later revisions - does it make
 sense to keep it around with this in mind?

 >  - In connection_listener_new():
 >     - 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)?

 I think it is a TCP stream that we attach but I see that it isn't really
 TCP, of course.

 >
 >     - 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?

 I have removed that - it was a mistake to include it. Good catch!

 >
 >     - 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.
 >

 I didn't combine them because I expect that this branch of code may change
 later and I wanted it to be treated totally separate.

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

 OK.

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

 OK.

 >  - Changes to connection_edge.c look okay.

 OK.

 >
 > I will now proceed to attempt to apply the patch against current master
 and test.

 OK.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12585#comment:21>
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