Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#1900 closed enhancement (implemented)

Create tor-fw-helper.c

Reported by: ioerror Owned by:
Priority: Medium Milestone: Deliverable-Sep2010
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: natpmp upnp
Cc: sjmurdoch Actual Points:
Parent ID: #1775 Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

TicketTypeStatusOwnerSummary
#1901enhancementclosedAdd NAT-PMP support to tor-fw-helper.c
#1902enhancementclosedAdd UPnP support to tor-fw-helper.c
#1904enhancementclosedTest NAT-PMP support in tor-fw-helper.c
#1905enhancementclosedTest UPnP support in tor-fw-helper.c
#1962defectclosedioerrorFix whitespace errors in tor-fw-helper
#1963taskclosedMake tor-fw-helper summarize success status

Change History (23)

comment:1 Changed 8 years ago by ioerror

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

comment:2 Changed 8 years ago by nickm

Status: newneeds_review

For reference, ioerror says that the branch now implements all subtickets of this ticket, and should get reviewed as such.

comment:3 Changed 8 years ago by ioerror

comment:4 Changed 8 years ago by ioerror

We may also want to add support for another upnp library at a later date:

http://pupnp.sourceforge.net/

comment:5 Changed 8 years ago 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?

comment:6 Changed 8 years ago by ioerror

git commit 34a0e2d4dc01a44414eebdfb8e0138f79f08b2d3 takes care of the autconfig && issue. Now we build if we have either library or both.

comment:7 Changed 8 years ago by ioerror

git commit e81a16822355dd2da867e5f916452d749547c5f6 takes care of the CPP flags that previously included natpmp and libminiupnpc; they're now in the tor-fw-helper makefile.am.

comment:8 Changed 8 years ago by ioerror

I'm using tor_snprintf rather than snprintf for the upnp code now.

comment:9 Changed 8 years ago by ioerror

OK - test_commandline_options renamed to log_commandline_options in 5c2c32164e1cde8c5c98ecde8950a9f716c578c1

comment:10 Changed 8 years ago by ioerror

I've cast the unsigned short to an int in tor_upnp_add_tcp_mapping() in 26f6008

comment:11 Changed 8 years ago by ioerror

It appears that UPNP_GetExternalIPAddress will not return an ipv6 address. It also appears to set externalIPAddress to \0 if it fails.

comment:12 Changed 8 years ago by ioerror

Yes, I believe that the library functions that we call are idempotent.

comment:13 Changed 8 years ago by ioerror

Things left to do from your code review:

  1. Throughout the C: This needs to get ported to windows.
  2. 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.
  3. 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.
  4. 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?

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

comment:14 Changed 8 years ago by ioerror

I've created Ticket #1983 to deal with porting to Windows - it doesn't need to be done by tomorrow.

comment:15 Changed 8 years ago by ioerror

I've taken care of the FD_SETSIZE issue - [tor-fw-helper 7117682] detect possible FD_SETSIZE errors

comment:16 Changed 8 years ago by ioerror

[tor-fw-helper 12bd664] make note about future enhancements for natpmp

comment:17 Changed 8 years ago by ioerror

I think the UPNP stuff should simply have human error codes and I've added notes about future enhancements for upnp too.

comment:18 Changed 8 years ago by ioerror

Ok - I'm down to one major blocker before we merge -

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.

I'll document these tonight.

comment:19 Changed 8 years ago by ioerror

Ok - I've documented and cleaned up tor-fw-helper/* as of 7a8f12690ec8a5d84cc6576b0046cbfcd1e62ea7

comment:20 Changed 8 years ago by ioerror

I've also documented tor-fw-helper in tor-fw-helper.1.txt; this is in commit 7b6373f7f58e5acc3f73aa49240fe2f65e798198

comment:21 Changed 8 years ago by ioerror

Resolution: implemented
Status: needs_reviewclosed

I believe that this is code review and merge ready. Please place all comments in either a new bug or in Ticket #1775.

comment:22 Changed 8 years ago by nickm

Rebased, squashed, and merged to master.

comment:23 Changed 6 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.