[tor-bugs] #1900 [Tor Client]: Create tor-fw-helper.c

[Tor Bug Tracker & Wiki]

#1900: Create tor-fw-helper.c
-------------------------+--------------------------------------------------
 Reporter:  ioerror      |       Owner:                     
     Type:  enhancement  |      Status:  needs_review       
 Priority:  normal       |   Milestone:  Deliverable-Sep2010
Component:  Tor Client   |     Version:  Tor: unspecified   
 Keywords:  natpmp upnp  |      Parent:  #1775              
-------------------------+--------------------------------------------------

Comment(by nickm):

 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 #1903.  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 configure.in: USE_FW_HELPER is set to true if $natpnp _and_ $upnp are
 true.  Do we mean "or"?  Also, ISTR that there are some versions of 'test'
 that don't do -a and -o properly, and that's why we use && and || rather
 than compound test calls.

 In configure.in: You're adding $TOR_CPPFLAGS_libnatpnp and
 $TOR_CPPFLAGS_libminiupnpc to CPPFLAGS.  Probably you want to make those
 only get added to tor_fw_helper's CPPFLAGS, since most of Tor won't build
 to use libnatpnp or libminiupnp at all.

 In tor-fw-helper-spec.txt: The spec isn't clear whether tor-fw-helper will
 sometimes print other messages in other formats in addition to the two
 provided or not.  It will.  You should probably do something to
 distinguish which messages are meant to be parseable and which aren't; and
 you should document which message formats are guaranteed to be stable.

 Also, the security concerns section seems to imply that no user should use
 tor-fw-helper.  Am I reading this right?

 Throughout the C: This needs to get ported to windows.

 Throughout the C: Functions, structures, and structure members should get
 documented.  Without documentation, it is hard to infer intent, and so is
 difficult to see whether behavior matches intent.

 Throughout the C: prefer tor_snprintf to snprintf.

 For both nat-pmp and upnp: Are the library functions that we call
 idempotent?  That is, if we try to add a port mapping that already exists,
 does it tell us "success" or something else?  It seems that our code is
 satisfied with nothing other than "success".

 In tor_natpmp_add_tcp_mapping, and anywhere else you use select(): unix
 fd_set uses a fixed-width bitfield internally.  If you are going to do an
 FD_SET to manipulate it, you MUST first make sure that natpmp.s is les
 than FD_SETSIZE, or you will trash the stack.

 It is possible for select() to return an error; you should check the
 return value of select.

 Probably there should be a function to make NATPMP_* errors into strings.
 You will not enjoy a future of debugging users who come up to you saying,
 "I got this log message saying E: readnatpmpresponseorretry failed -93".

 In tor-fw-helper-natpmp.h:

 Can you move the #include <natpmp.h> into tor_fw_helper-natpmp.c ?
 Minimizing visibility for external library headers is a good way to make
 sure they have a harder time interfering.

 In tor-fw-helper-upnp.c:

 All the UPNP_ERR_* defines seem to be potentially returnable to code
 outside of tor-fw-helper.c.  Does that mean they should move into the
 header so that the caller can know which error they got?

 Can UPNP_GetExternalIPAddress ever give us an IPv6 address, or leave
 externalIPAddress unchanged?  The magic number 16 on
 char[externalIPAddress] makes me a little nervous, even though
 strlen("255.255.255.255")+1 is 16.

 In tor_upnp_add_tcp_mapping:
 Passing an unsigned short to a %d format is a little iffy even if it's
 technically legal.  casting it to int first would make me happier.

 In tor-fw-helper.c:
 test_commandline_options should be renamed to log_commandline_options.

 getopt_long is a GNU extension.  What will you do on systems without it?

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