* Transports: Clean up internal/external port confusion (ticket #873)

- Bind SSU to configured internal, not external, port at startup
   - Use only internal ports for UPnP (getRequestedPort() fixups)
   - Don't have NTCP follow frequent SSU port changes
   - Don't use external SSU port for internal NTCP port
   - Display internal SSU port on /confignet

apps/routerconsole/java/src/net/i2p/router/web/ConfigNetHelper.java

@@ -50,7 +50,12 @@ public class ConfigNetHelper extends HelperBase {
         return ua.getHost();
     }
 
+    /**
+     *  To reduce confusion caused by NATs, this is the current internal SSU port,
+     *  not the external port.
+     */
     public String getUdpPort() {
+      /****
         RouterAddress addr = _context.router().getRouterInfo().getTargetAddress("SSU");
         if (addr == null)
             return _("unknown");
@@ -58,8 +63,17 @@ public class ConfigNetHelper extends HelperBase {
         if (ua.getPort() <= 0)
             return _("unknown");
         return "" + ua.getPort();
+      ****/
+        // Since we can't get to UDPTransport.getRequestedPort() from here, just use
+        // configured port. If UDPTransport is changed such that the actual port
+        // could be different, fix this.
+        return getConfiguredUdpPort();
     }
 
+    /**
+     *  This should always be the actual internal SSU port, as UDPTransport udpates
+     *  the config when it changes.
+     */
     public String getConfiguredUdpPort() {
         return _context.getProperty(UDPTransport.PROP_INTERNAL_PORT, "unset");
     }

history.txt

@@ -1,3 +1,16 @@
+2013-05-06 zzz
+ * Transports: Clean up internal/external port confusion (ticket #873)
+   - Bind SSU to configured internal, not external, port at startup
+   - Use only internal ports for UPnP (getRequestedPort() fixups)
+   - Don't have NTCP follow frequent SSU port changes
+   - Don't use external SSU port for internal NTCP port
+   - Display internal SSU port on /confignet
+
+2013-05-01 zzz
+ * BuildRequestor: Slow down build loop if we have no exploratory tunnels
+                   (ticket #926)
+ * IRC Server tunnel: Reject bad protocols immediately
+
 2013-04-30 str4d
  * Console: Updates to readme_ar and a CSS tweak from hamada (ticket #489)
 

router/java/src/net/i2p/router/RouterVersion.java

@@ -18,7 +18,7 @@ public class RouterVersion {
     /** deprecated */
     public final static String ID = "Monotone";
     public final static String VERSION = CoreVersion.VERSION;
-    public final static long BUILD = 20;
+    public final static long BUILD = 21;
 
     /** for example "-test" */
     public final static String EXTRA = "";

router/java/src/net/i2p/router/transport/CommSystemFacadeImpl.java

@@ -272,8 +272,8 @@ public class CommSystemFacadeImpl extends CommSystemFacade {
      * This should really be moved to ntcp/NTCPTransport.java, why is it here?
      */
     @Override
-    public synchronized void notifyReplaceAddress(RouterAddress UDPAddr) {
+    public synchronized void notifyReplaceAddress(RouterAddress udpAddr) {
-        if (UDPAddr == null)
+        if (udpAddr == null)
             return;
         NTCPTransport t = (NTCPTransport) _manager.getTransport(NTCPTransport.STYLE);
         if (t == null)
@@ -296,19 +296,38 @@ public class CommSystemFacadeImpl extends CommSystemFacade {
         // Auto Port Setting
         // old behavior (<= 0.7.3): auto-port defaults to false, and true trumps explicit setting
         // new behavior (>= 0.7.4): auto-port defaults to true, but explicit setting trumps auto
+        // TODO rewrite this to operate on ints instead of strings
         String oport = newProps.getProperty(NTCPAddress.PROP_PORT);
         String nport = null;
         String cport = _context.getProperty(PROP_I2NP_NTCP_PORT);
         if (cport != null && cport.length() > 0) {
             nport = cport;
         } else if (_context.getBooleanPropertyDefaultTrue(PROP_I2NP_NTCP_AUTO_PORT)) {
-            nport = UDPAddr.getOption(UDPAddress.PROP_PORT);
+            // 0.9.6 change
+            // This wasn't quite right, as udpAddr is the EXTERNAL port and we really
+            // want NTCP to bind to the INTERNAL port the first time,
+            // because if they are different, the NAT is changing them, and
+            // it probably isn't mapping UDP and TCP the same.
+            Transport udp = _manager.getTransport(UDPTransport.STYLE);
+            if (udp != null) {
+                int udpIntPort = udp.getRequestedPort();
+                if (udpIntPort > 0)
+                    // should always be true
+                    nport = Integer.toString(udpIntPort);
+            }
+            if (nport == null)
+                // fallback
+                nport = udpAddr.getOption(UDPAddress.PROP_PORT);
         }
         if (_log.shouldLog(Log.INFO))
             _log.info("old: " + oport + " config: " + cport + " new: " + nport);
         if (nport == null || nport.length() <= 0)
             return;
-        if (oport == null || ! oport.equals(nport)) {
+        // 0.9.6 change
+        // Don't have NTCP "chase" SSU's external port,
+        // as it may change, possibly frequently.
+        //if (oport == null || ! oport.equals(nport)) {
+        if (oport == null) {
             newProps.setProperty(NTCPAddress.PROP_PORT, nport);
             changed = true;
         }
@@ -335,7 +354,7 @@ public class CommSystemFacadeImpl extends CommSystemFacade {
             _log.info("old: " + ohost + " config: " + name + " auto: " + enabled + " status: " + status);
         if (enabled.equals("always") ||
             (Boolean.parseBoolean(enabled) && status == STATUS_OK)) {
-            String nhost = UDPAddr.getOption(UDPAddress.PROP_HOST);
+            String nhost = udpAddr.getOption(UDPAddress.PROP_HOST);
             if (_log.shouldLog(Log.INFO))
                 _log.info("old: " + ohost + " config: " + name + " new: " + nhost);
             if (nhost == null || nhost.length() <= 0)

router/java/src/net/i2p/router/transport/TransportManager.java

@@ -345,21 +345,13 @@ public class TransportManager implements TransportEventListener {
     }
     
     /**
-     * Include the published port, or the requested port, for each transport
+     * The actual or requested INTERNAL ports, for each transport,
-     * which we will pass along to UPnP
+     * which we will pass along to UPnP to be forwarded.
      */
     private Map<String, Integer> getPorts() {
         Map<String, Integer> rv = new HashMap(_transports.size());
         for (Transport t : _transports.values()) {
             int port = t.getRequestedPort();
-            if (t.getCurrentAddress() != null) {
-                    String s = t.getCurrentAddress().getOption("port");
-                    if (s != null) {
-                        try {
-                            port = Integer.parseInt(s);
-                        } catch (NumberFormatException nfe) {}
-                    }
-            }
             // Use UDP port for NTCP too - see comment in NTCPTransport.getRequestedPort() for why this is here
             if (t.getStyle().equals(NTCPTransport.STYLE) && port <= 0 &&
                 _context.getBooleanProperty(CommSystemFacadeImpl.PROP_I2NP_NTCP_AUTO_PORT)) {

router/java/src/net/i2p/router/transport/ntcp/NTCPTransport.java

@@ -638,6 +638,7 @@ public class NTCPTransport extends TransportImpl {
 
     //private boolean bindAllInterfaces() { return true; }
 
+    /** caller must synch on this */
     private void configureLocalAddress() {
         RouterContext ctx = getContext();
         if (ctx == null) {
@@ -684,8 +685,17 @@ public class NTCPTransport extends TransportImpl {
         }
     }
 
+    /**
+     *  @return current port, else NTCP configured port, else -1 (but not UDP port if auto)
+     */
     @Override
     public int getRequestedPort() {
+        NTCPAddress addr = _myAddress;
+        if (addr != null) {
+            int port = addr.getPort();
+            if (port > 0)
+                return port;
+        }
         // would be nice to do this here but we can't easily get to the UDP transport.getRequested_Port()
         // from here, so we do it in TransportManager.
         // if (Boolean.valueOf(_context.getProperty(CommSystemFacadeImpl.PROP_I2NP_NTCP_AUTO_PORT)).booleanValue())

router/java/src/net/i2p/router/transport/udp/UDPTransport.java

@@ -83,7 +83,10 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
     
     /** summary info to distribute */
     private RouterAddress _externalAddress;
-    /** port number on which we can be reached, or -1 for error, or 0 for unset */
+    /**
+     * Port number on which we can be reached, or -1 for error, or 0 for unset
+     * Do NOT use this for current internal port - use _endpoint.getListenPort()
+     */
     private int _externalListenPort;
     /** IP address of externally reachable host, or null */
     private InetAddress _externalListenHost;
@@ -310,19 +313,18 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
         
         // Requested bind port
         // This may be -1 or may not be honored if busy,
+        // Priority: Configured internal, then already used, then configured external
         // we will check below after starting up the endpoint.
         int port;
         int oldIPort = _context.getProperty(PROP_INTERNAL_PORT, -1);
+        int oldBindPort = _endpoint != null ? _endpoint.getListenPort() : -1;
         int oldEPort = _context.getProperty(PROP_EXTERNAL_PORT, -1);
-        if (_externalListenPort <= 0) {
+        if (oldIPort > 0)
-            // no explicit external port, so lets try an internal one
+            port = oldIPort;
-            if (oldIPort > 0)
+        else if (oldBindPort > 0)
-                port = oldIPort;
+            port = oldBindPort;
-            else
+        else
-                port = oldEPort;
+            port = oldEPort;
-        } else {
-            port = _externalListenPort;
-        }
         if (bindToAddr != null && _log.shouldLog(Log.WARN))
             _log.warn("Binding only to " + bindToAddr);
         if (_log.shouldLog(Log.INFO))
@@ -436,14 +438,23 @@ public class UDPTransport extends TransportImpl implements TimedWeightedPriority
     }
 
     /**
-     *  _externalListenPort should always be set (by startup()) before this is called,
+     *  The current or configured internal port.
-     *  so the returned value should be > 0
+     *  UDPEndpoint should always be instantiated (and a random port picked if not configured)
+     *  before this is called, so the returned value should be > 0
+     *  unless the endpoint failed to bind.
      */
     @Override
     public int getRequestedPort() {
-        if (_externalListenPort > 0)
+        if (_endpoint != null) {
-            return _externalListenPort;
+            int rv = _endpoint.getListenPort();
-        return _context.getProperty(PROP_INTERNAL_PORT, -1);
+            if (rv > 0)
+                return rv;
+        }
+        // fallbacks
+        int rv = _context.getProperty(PROP_INTERNAL_PORT, -1);
+        if (rv > 0)
+            return rv;
+        return _context.getProperty(PROP_EXTERNAL_PORT, -1);
     }
 
     /**