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:               |
-----------------------------+--------------------------------
Changes (by yawning):

 * status:  needs_review => needs_revision


Comment:

 (I looked at SocksSocket-009.patch.)

 Some quick thoughts after a casual review:
  * This patch will break the build in environments that do not define
 AF_UNIX.  I'm not sure we actually care or build in such places
 (src/or/connections.c and src/or/cpuworker.c also will break the build),
 but a good fraction of the code has explicit guards.
  * In or/config.c:parse_ports() the parse_unix_socket_config() call should
 probably be next to where the normal SocksPort/SocksListenAddress is
 handled, instead of in a block where the control port is handled. (Minor)
  * In or/connection.c:
    * check_location_for_socks_unix_socket() is duplicating
 check_location_for_unix_socket() just to look at a different option and
 print "SocksSocket" instead of "control socket".  This should probably be
 one routine, that takes the group writable option as an argument and an
 error message that works for both cases (or another argument).
    * check_location_for_socks_unix_socket() inherited not using
 path_is_relative() from check_location_for_unix_socket().  Both cases
 should probably be fixed for future portability.
    * connection_listener_new() has left overs from what looks like an
 attempt to handle the control port and SocksSocket with the same code.
 "if ( type == CONN_TYPE_CONTROL_LISTENER ) {") is harmless  but should be
 removed.  "if (options->SocksSocketsGroupWritable) {" is a bug (the
 control port socket will be made group writable based off
 SocksSocketGroupWritable").
    * connection_handle_listener_read() leaks memory, a tor_dup_addr() call
 needs to be moved into a branch.
 {{{
     newconn->address = tor_dup_addr(&addr); /* malloc() here */
     /* <snip> */
     if (new_type == CONN_TYPE_AP && conn->socket_family == AF_UNIX) {
       newconn->port = 0;
       newconn->address = tor_strdup(conn->address); /* leak here */
 }}}
  * Run make check-spaces and fix the whitespace issues.

 It looks mostly ok, but there's a few minor bug fixes required (and
 whitespace cleanup), setting to needs_revision.

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