[tor-bugs] #4567 [Tor Bridge]: Extend Tor's UPnP support for pluggable transport ports

Tor Bug Tracker & Wiki torproject-admin at torproject.org
Fri Jun 29 16:35:28 UTC 2012


#4567: Extend Tor's UPnP support for pluggable transport ports
------------------------+---------------------------------------------------
 Reporter:  karsten     |          Owner:  asn                     
     Type:  project     |         Status:  needs_revision          
 Priority:  normal      |      Milestone:  Sponsor G: June 30, 2012
Component:  Tor Bridge  |        Version:                          
 Keywords:              |         Parent:                          
   Points:              |   Actualpoints:                          
------------------------+---------------------------------------------------

Comment(by asn):

 Replying to [comment:7 nickm]:
 > okay, so fwict all the commits before eff60fc5fc are refactoring, code
 moving, etc that don't actually have much to do with this ticket.  Worth
 merging anyway, though, of course : I'll be especially glad to say goodbye
 to the old behavior of mp->transports (where sometimes it contains strings
 and sometimes it contains transports).
 >

 Correct. Commits before `eff60fc5fc` are cherry-picked from #3589. They
 were needed for the same reason they were needed in #3589, so that server
 managed proxies keep their `transport_t`s around even after registering
 them. This is necessary because we need to know the address:port of server
 transports to port forward them (in the past, when server managed proxies
 registered their transports, they replaced them with a string of their
 name ;)).

 This means that we will need to fix the issues you found during your
 review of #3589, before we merge this feature.

 > Making ports generic in eff60fc5fc is also a win IMO: "There are only
 three numbers in computer science: zero, one, and infinity."  I think the
 tor_parse_long calls in that commit are wrong, though: they have a maximum
 of 6556, when the maximum port number is 65535.
 >

 True. Fixed. (Also, #6218...)

 > d180c576da516f is the big Tor change.  BTW, there is no such thing as
 "Refactor Tor to support the new tor-fw-helper protocol."  Refactoring
 means to improve the code without changing its functionality or behavior.
 I try not to get pedantic about stuff like that, but when I see a commit
 which claims to be refactoring but isn't, I get all twitchy.
 >

 Gotcha.

 > The right way to stick things together for a log message is esc_for_log
 and its kin.
 >

 Hm, where is the actual place you want me to use this function in this
 case?

 > The documentation for any function taking a smartlist_t really needs to
 explain what the smartlist contains.
 >

 Updated the documentation of `tor_check_port_forwarding()`.

 > In the loop that builds argv, I'd like more assurance that argv_index
 can never overflow.
 >

 Hm, I initially had some checks, but I removed them because I considered
 them outside of our threat model (and they made the code look ugly). To
 overflow `argv_index` or the allocation of `argv`, a `HUGE`
 `ports_to_forward` must be given; which means that the config file is
 humongous, or the transport proxy is trying to screw us.

 In any case, I added the checks back.

 > The manpage doc/tor-fw-helper.1.txt needs to change.
 >

 Done.

 > When I tried to build this, I got an error because
 get_list_of_ports_to_forward doesn't have a prototype in config.h.  Have
 you been building this with --enable-gcc-warnings?  Please do, if not.
 >

 Done.

 > Needs a changes file.
 >

 Done.

 > Breaks make check-spaces.
 >

 Done.

 > How much have you been able to test this?

 I've been testing this with both NAT-PMP and UPnP. I would '''surely'''
 like someone else to test this too.

 BTW, another thing I don't like about this code is that
 `get_list_of_ports_to_forward()` will be called every
 `PORT_FORWARDING_CHECK_INTERVAL`, even if it's not needed inside
 `tor_check_port_forwarding()`. I didn't find a nice way to fix this
 without radically redesigning the `tor_check_port_forwarding()` approach

 Finally, there is no nice way to say to `tor_check_port_forwarding()`:
 "OK, I understood that you failed to forward port X, stop sending log
 lines". That would also need refactoring of the
 `tor_check_port_forwarding()` logic. This issue also existed before my
 changes.

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


More information about the tor-bugs mailing list