[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