[tor-bugs] #4694 [arm]: Merge request: UPnP support
Tor Bug Tracker & Wiki
torproject-admin at torproject.org
Mon Dec 12 17:41:31 UTC 2011
#4694: Merge request: UPnP support
-------------------------+--------------------------------------------------
Reporter: krkhan | Owner: atagar
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: arm | Version:
Keywords: | Parent:
Points: | Actualpoints:
-------------------------+--------------------------------------------------
Comment(by atagar):
Hi Kamran - very nice! I'm honestly very impressed that you did this
without any external dependencies. Great job!
At the moment your changes are built on previous changes in your
repository which could make merging an issue. Please cut a new branch off
of the official master and rebase your changes (c9a61f5, fac6a23, 982f8a5,
5cf57f7) on that.
Now onto the code review feedback...
======================================================================
GENERAL FEEDBACK
======================================================================
As I've nudged on irc this doesn't need to be an arm-specific feature.
With some polish and testing this would be a very nice addition to stem
which would make this available to other python controllers. Automated
testing would also benefit arm and make me more comfortable with this
change.
Stem is the future for arm's codebase. Bit by bit I've been rewriting
arm's utils, adding tests, and moving them to stem so I'd rather if we
were implementing UPnP support there first instead.
======================================================================
We should be wary of doing anything UPnP related when the user simply runs
arm to look at their relay. This is a feature that users should opt into
via an armrc option...
# uses UPnP to make our ORPort reachable
features.useUpnp false
with UPnP calls wrapped in a check for both that config option and that
the user has an ORPort (there's no point in trying to negotiate UPnP when
they aren't running a relay).
This actually has more value to users as a wizard option since that's an
"arm managed" tor instance, more similar to running tor via vidalia. I'd
suggest looking at the 'Options.PORTFORWARD' option in src/cli/wizard.py
(this would be a great fallback for when tor-fw-helper is unavailable).
======================================================================
This adds the UPnP configuration but never removes it. Maybe this will
help...
{{{
08:18 < atagar> I'm a little curious after reading this how vidalia
handles upnp - does it
somehow clear the upnp entry when it shuts down?
08:21 < atagar> chiiph: ^
08:24 < edmanm> atagar: yes, it does.
08:25 < atagar> ahh - thanks
08:26 < edmanm> (see the last few lines of UPNPControlThread::run() in
src/vidalia/config/UPNPControlThread.cpp if you want to poke further
though. :)
08:28 < atagar> got it -
https://gitweb.torproject.org/vidalia.git/blob/HEAD:/src/vidalia/config/UPNPControlThread.cpp#l56
08:28 < atagar> is that the same as an 'AddPortMapping' action for port
zero?
08:30 < edmanm> it'll end up calling miniupnpc's UPNP_DeletePortMapping.
not sure if that's
the same as AddPortMapping internally within miniupnpc though.
}}}
======================================================================
I have UPnP disabled on my home router. However, when I run your changes
it displays...
UPnP device available: Actiontec Electronics, Inc. 331 found at
192.168.0.1
This is... spooky. Does this mean that my router's lying to me about UPnP
being disabled or is this message incorrect? I'm happy to debug this with
you if you'd like.
======================================================================
CHANGE SPECIFIC FEEDBACK
======================================================================
src/util/upnp.py
Pylint spotted a few valid minor issues...
{{{
W: 26:create_soap_xml: Dangerous default value {} as argument
W: 34:UPnPError.__init__: __init__ method from base class 'Exception' is
not called
W:103:UpnpDevice.get_port_mappings: Unused variable 'e'
W:126:UpnpDevice.add_port_mapping: Unused variable 'e'
W: 8: Unused import time
W: 10: Unused import urllib
W: 7: Unused import sys
}}}
> + def __init__(self, ip, response):
The self.response is never used, and the response arg is only used to
determine 'location' which is in turn used to get 'fd'. This object would
be much easier to understand if its input argument was the 'location' and
you had a comment giving an example of what one looks like.
> + sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM,
socket.IPPROTO_UDP)
> + sock.connect(('18.0.0.1', 9))
> + self.ifip, _ = sock.getsockname()
I'm guessing that you're trying to figure out our internal ip address? If
so then this method can be shortened a tiny bit...
{{{
>>> s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
>>> s.connect(('18.0.0.1', 0))
>>> s.getsockname()[0]
'192.168.0.2'
>>> s.close()
}}}
It looks like tor does something like this...
https://gitweb.torproject.org/tor.git/blob/HEAD:/src/common/address.c#l1089
however, it confuses users so they're trying to find an alternative...
https://trac.torproject.org/projects/tor/ticket/1827
I'm a little frustrated that there doesn't seem to be a less hacky method
for doing this with python. In practice this seems to match the local ip
from...
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/cli/connections/connPanel.py#l427
However, using that would be pretty hacky too and won't work if tor isn't
running or connection resolution is disabled (ie, if the getConnections()
results are empty). If we could figure out the system's dns resolvers and
connect to that instead it would be the least alarming to users though I'm
not spotting a nice way of doing that... :/
Please move this to a "getMyIpAddress()" function in the connection util.
> + match = re.findall(r"LOCATION: (.*)", response)
> + if not match:
> + return
> + location = match[0].strip()
Do we have a validly constructed UpnpDevice object if we prematurely
return here? This'll be moot if we get rid of the response arg as
mentioned earlier.
> + fd = None
Not needed.
> + if 'url' in self.control:
> + self.get_port_mappings()
> + self.add_port_mapping()
It seems inappropriate for the constructor to actually turn on UPnP.
> + def get_port_mappings(self):
> + action = 'GetGenericPortMappingEntry'
> +
> + for i in range(3):
> + args = { 'NewPortMappingIndex' : i }
> + try:
> + retvals = self.get_soap_response(action, args)
> + except UPnPError, e:
> + pass
> + else:
> + log.log(log.NOTICE, 'Port mapping found: ' +
retvals['NewPortMappingDescription'])
This function doesn't set or return anything. Is its only purpose to log
the 'NewPortMappingDescription'? Why is this done three times?
> + def add_port_mapping(self):
Is this idempotent? How long does this UPnP entry last? Until tor is shut
down or does it remain until the router's restarted? We should be careful
not to mess with the user's router configuration without reverting it
afterward.
> + 'SOAPAction': '"' + self.control['serviceType'] + '#' + action +
'"',
The serviceType mapping comes from parse_desc which may not have happened
if we got an urllib2.URLError when we called "fd =
urllib2.urlopen(location)".
======================================================================
src/util/connections.py
Please move these changes into 'upnp.py' (the UpnpResolver is upnp
specific).
Pylint spotted some more minor issues...
{{{
W:663:AppResolver._queryApplications: Dangerous default value [] as
argument
W: 23: Unused import struct
W: 20: Unused import fcntl
}}}
> +import fcntl
> import os
> +import select
> +import struct
> +import socket
Minor convention note but I order imports by the length of the module's
name.
> +class UpnpResolver(threading.Thread):
You never actually use this as a thread. Instead resolve spawns
subthreads.
> + """
> + Constructs a resolver instance.
> + """
> + threading.Thread.__init__(self)
Missing a blank line between the doc and code.
> + self._cond = threading.Condition() # used for pausing when waiting
for devices
> + self.isResolving = False # flag set if we're in the process of
making a query
> + self.failureCount = 0 # -1 if we've made a successful query
Conventions are that private variables are self._foo and public are
self.foo. I'm guessing that a few more should have underscores.
> + self.devicesLock.acquire()
> + devices = self.queryDevices
> + self.devicesLock.release()
I'm pretty sure that assignments are atomic so locking here isn't really
necessary.
> + def resolve(self):
> + """
> + This clears the last set of devices when completed.
> + """
> +
> + if self.failureCount < 3:
> + self.isResolving = True
> + t = threading.Thread(target = self._queryRootDevices)
> + t.setDaemon(True)
> + t.start()
Unfortunately this is likely gonna cause occasional problems when shutting
down. If a python process terminates with leftover threads (even daemon
threads) then it will give a nebulous 'syshook' error. You need to keep
track of all running threads and make sure that when arm quits the main
thread joins on them. If you really want to spawn resolver threads then
see the _Resolver class of hostnames.py for an example of doing this
safely (though it looks like in practice you don't want more than one).
Also, you aren't checking the isResolving flag before making a thread so
you may already have a thread. Hence if, say, you call resolve five times
you'll have five resolving threads and the first one to finish will set
isResolving back to False.
> + sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM,
socket.IPPROTO_UDP)
> + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
> + sock.setsockopt(socket.IPPROTO_IP, socket.IP_MULTICAST_TTL, 5)
> +
> + sock.bind(('', 1900))
Please add a comment saying what this is doing and the importance of 1900.
> + 'HOST: 239.255.255.250:1900\r\n'
> + sock.sendto(msg, ('239.255.255.250', 1900))
Please add a comment for what this address is for. Also, this will fail
(with a stacktrace) if the user's disconnected from their network...
File "/home/atagar/Desktop/arm/src/util/connection.py", line 843, in
_queryRootDevices
sock.sendto(msg, ('239.255.255.250', 1900))
error: [Errno 101] Network is unreachable
> + self.failureCount = self.failureCount + 1
This would read a bit better as...
self.failureCount += 1
> + self.devicesLock.acquire()
> + self.queryDevices = devices
> + self.isResolving = False
> + self.devicesLock.release()
Each time resolve() is called (every ten seconds) you're setting
queryDevices to a new upnp.UpnpDevice. Constructing upnp.UpnpDevice causes
us to add a UPnP entry for our relay - I doubt this is what you want.
======================================================================
src/cli/connections/connPanel.py
Hmmm, I see the reasoning that led you to add this to the connection panel
but this really concerns the user's ORPort, not their current connections.
Also, this takes quite a bit of room and looks a little odd since it
prevents the scroll bar from reaching the bottom of the screen.
If the user both has an ORPort and opted into the UPnP config option then
I'd suggest...
- Call the UPnP resolve method until we either (1) have a single success
or (2) fail three times.
- Upon success log the user's UPnP device name.
- If we successfully enable UPnP then note this in the header panel
besides the ORPort, maybe something like...
morrigan - 216.254.20.162:9050, UPnP Enabled, Control Port (cookie): 9051
For posterity (and since I'm kinda bugged that I spent hours code
reviewing this file before realizing that we should go with something
else) here's my feedback for the file. Feel free to ignore it. :)
> +from util import connections, enum, log, panel, torTools, uiTools
It doesn't look like you're using that new log import.
> + self._upnpDevices = [] # upnp devices found on network
Very, very minor but the comment's indent is off by a space.
> + # rate limits appResolver queries to once per update
> + self.upnpResolveSinceUpdate = False
> +
> + # rate limits appResolver queries to once per update
> + self.upnpResolveSinceUpdate = False
Paste error. ;)
> + listHeight = height - 3
That certainly does the trick though I'd rather if you had a
upnpLabelHeight attribute that's subtracted where appropriate, like the
detailPanelOffset. This will make it trivial to toggle the label to be on
or off via a config option.
> + if currentTime - self._lastUpnpResolve > 10:
The ten seconds should be a constant at the top of the file. More
importantly, though, all lenthy operations should be in run() or _update()
rather than the draw loop. There's a couple reasons for this...
- having the update in the draw() function means that it will only be
executed if the user is looking at that page
- rendering the panel should never do 'work' since any blocking call will
cause this part of the interface to seize (looking buggy to the user),
though it looks like you're accounting for this via a timeout
You're probably basing this on _resolveApps() which is a good idea, but in
this case not right. I *wanted* _resolveApps() to only run if the user was
actively looking at the connection panel since it was pointless to do
those lsof lookups when the user would never see the results.
> + msg = "UPnP device%s available: " % ("s" if
len(self._upnpDevices) > 1 else "")
> + self.addstr(listHeight + 2, xOffset, msg, curses.A_STANDOUT |
uiTools.getColor('green'))
> + xOffset = xOffset + len(msg)
> + msg = ", ".join([device.name for device in self._upnpDevices])
> + self.addstr(listHeight + 2, xOffset, msg, curses.A_STANDOUT |
uiTools.getColor('green'))
Using an xOffset like this is the pattern I use when multiple strings need
different formatting. However, in this case it's equivilant to doing a
single addstr call, no?
msg = "UPnP device%s available: " % ("s" if len(self._upnpDevices) > 1
else "")
msg += ", ".join([device.name for device in self._upnpDevices])
self.addstr(listHeight + 2, xOffset, msg, curses.A_STANDOUT |
uiTools.getColor('green'))
> + if flagQuery:
> + self.appResolveSinceUpdate = True
Paste error (setting the wrong flag).
Cheers! -Damian
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4694#comment:1>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list