[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