Create tor-fw-helper.c
tor-fw-helper.c should support upnp and nat-pmp; it should be written in such a way that output from tor-fw-helper.c will be machine parsed without too much trouble.
Activity
changed milestone to %Deliverable-Sep2010
My work on tor-fw-helper.c is happening in the following git branch: https://gitweb.torproject.org/ioerror/tor.git/shortlog/refs/heads/tor-fw-helper
It's likely that you'll want to download the following libraries to make use of tor-fw-helper: http://miniupnp.free.fr/files/download.php?file=libnatpmp-20100202.tar.gz http://miniupnp.free.fr/files/download.php?file=miniupnpc-1.4.20100609.tar.gz
We may also want to add support for another upnp library at a later date:
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 (moved). 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?
Things left to do from your code review:
- 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.
- 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.
- ''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?''
- ''In tor-fw-helper-natpmp.c: 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".''
I've created Ticket #1983 (moved) to deal with porting to Windows - it doesn't need to be done by tomorrow.
I believe that this is code review and merge ready. Please place all comments in either a new bug or in Ticket #1775 (moved).
Trac:
Status: needs_review to closed
Resolution: N/A to implementedmentioned in issue #1903 (moved)
mentioned in issue #1983 (moved)
moved to tpo/core/tor#1900 (closed)