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

 ''I'm going to summarize the issues Nick mentioned into numbers as
 subtasks for this bug:
 ''
  1. ''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.''
  1. ''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?''
  1. ''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.''
  1. ''It still needs a Windows implementation.''
  1. ''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).''
  1. ''It would be very good to have a unit test for this.''
  1. ''In log_from_pipe(): ''
  1. ''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().''
  1. ''Prefer tor_sscanf to sscanf whenever possible.''
  1. ''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.''
  1. ''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?''
  1. ''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.''
  1. ''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.''
  1. ''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:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online