[tor-bugs] #1903 [Tor Client]: Teach Tor to control networks with tor-fw-helper.c

[Tor Bug Tracker & Wiki]

#1903: Teach Tor to control networks with tor-fw-helper.c
-------------------------+--------------------------------------------------
 Reporter:  ioerror      |       Owner:                     
     Type:  enhancement  |      Status:  needs_review       
 Priority:  normal       |   Milestone:  Deliverable-Sep2010
Component:  Tor Client   |     Version:  Tor: unspecified   
 Keywords:  upnp         |      Parent:  #1775              
-------------------------+--------------------------------------------------
Changes (by nickm):

  * status:  new => needs_review


Comment:

 Okay, so I'm reviewing sjmurdoch's "upnp" branch, which looks like it
 contains all of ieorror's tor-fw-helper branch.  Some of this will apply
 to this ticket and some will apply to #1900.  I'll try to put the right
 comments on the right ticket, but I might make a mistake, so you should
 probably look in both places.

 This review is based on "git diff master...sjm217/upnp"; I'm looking at
 the overall changes, not the individual patches.  The current sjm217/upnp
 head I'm looking at is b86cbcc67f1.

 In zlib.c: It looks like some old bug1526 stuff snuck into this branch.
 We should kill that when we finally rebase and squash (probably we
 shouldn't rebase and squash until we are for-sure ready to merge, though).

 In util.c: format_helper_exit_status looks safe, but I'd be a little more
 comfortable if there were more explicit checks to make sure we can't run
 off the start of the buffer.  You check in the two loops, but not before
 adding the '-' and '/'.  The Doxygen comment should require that hex_errno
 have at least HEX_ERRNO_SIZE bytes available.  Also, the format should be
 documented somewhere; if not in the doc for format_helper_exit_status,
 then somewhere mentioned in that doc.

 Also on format_helper_exit_status: it seems to me that the caller is
 responsible for filling the buffer with ' ' characters and appending a
 trailing '\n\0'.  Why not put the responsibility into
 format_helper_exit_status?

 In tor_spawn_background:  stdout_read, stderr_read, and argv aren't
 documented.  Also, it would be good to note (via an XXXX comment, or
 ideally a trac ticket) that we could remove the icky
 for(fd=STDERR_FILENO+1;fd<max_fd; fd++) close(fd) loop by setting
 FD_CLOEXEC on all of our files.

 It still needs a Windows implementation.

 log_err is for nonsurvivable errors; log_warn is more appropriate for a
 case where you can't close an fd.  (This applies to log_from_pipe too).

 It would be very good to have a unit test for this.

 In log_from_pipe():

 strstr is for telling you if a string is _in_ a buffer, but the comment
 above strstr(buf, SPAWN_ERROR_MESSAGE) implies that we want to know if
 SPAWN_ERROR_MESSAGE is at the _start_ of the buffer.  For that, use
 strcmpstart().

 Prefer tor_sscanf to sscanf whenever possible.

 I'm confused by the tor_sscanf format: first off, if you want to check for
 an exact match rather then a prefix match, you need to say 'r =
 sscanf("%d/%d%c", &i1,i2,&c)' and make sure that r==2 (not 0 or 1 or 3).
 But why is the format looking for %d?  Is it scanning the output of
 format_helper_exit_status?  That function encodes things as hexadecimal,
 not decimal.

 What does fgets do when read() returns an EAGAIN in the middle of a line?
 Does it return a partial line, or wait for a newline?

 tor_check_port_forwarding: is missing documentation.  Because of that, it
 took me a little while to figure out that you're supposed to call it over
 and over.

 It seems that the code here does two things: one is a general task "Launch
 a child process and see what it says" and another is a more specific task
 "Launch a tor-fw-helper instance and act based on its output.)"  It might
 be a good idea to disentangle these eventually, in case we ever want to
 launch anything else.

 It might be cleaner to move the static variables from
 tor_check_port_forwarding into some kind of struct, in case we ever want
 to launch two things in the future.

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