[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              
-------------------------+--------------------------------------------------

Comment(by sjmurdoch):

 Replying to [comment:1 nickm]:

 I now think I have either dealt with or created new tickets for all the
 issues you have pointed out. My changes have been put in branch bug1903 of
 git://git.torproject.org/sjm217/tor.git (8a12ce2). I think this should be
 ready for merge, with the possible exception of a unit test failing as a
 result of bug #2045. Should I disable this test until I have fixed the
 bug?

 The other issues dealt with are:

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

 Done in 5a77c64.

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

 I did that because I couldn't put the memset in there, because I was not
 sure
 it could be guarenteed to be signal handler safe. I agree with your point
 so
 have replaced it with a for loop (in 5a77c64).

 > In tor_spawn_background: stdout_read, stderr_read, and argv aren't
 > documented.

 Done in 68e576e.

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

 Noted in 708ba88 and will be dealt with in bug #2029.

 > It still needs a Windows implementation.

 This is bug #1983.

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

 Done in 4d694c7.

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

 Done in 8a12ce2.

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

 I was checking whether strstr(s1, s2) == s1, not != NULL, so I think it
 does
 check if it is at the start. In any case, I've replaced it with
 strcmpstart()
 which makes it clearer what functionality is intended (and may be
 marginally
 faster), in 23e9f36.

 > Prefer tor_sscanf to sscanf whenever possible.

 Done in 23e9f36.

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

 I thought I did do this by "if (retval == 2) {..." a couple of lines
 later.

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

 Indeed, that's a bug (fixed in 23e9f36).

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

 This will be dealt with in bug #2045 (see comment:8 above.)

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

 ioerror documented it in 48245af of
 git://git.torproject.org/ioerror/tor.git.

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

 As this seems less urgent than the other tasks, I have noted these in bug
 #2030.

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