UPnP fixes part 1:

Numerous fixes for handling multiple devices and selecting the best one Fixes on device removal Deal with devices that support permanent leases only Locking fixes Sort ignored devices in UI Remove unused isDisabled field Log tweaks
2020-05-16 21:16:51 +00:00
parent a0261e8fd7
commit c6c9ba76d9
1 changed files with 222 additions and 118 deletions
--- a/router/java/src/net/i2p/router/transport/UPnP.java
+++ b/router/java/src/net/i2p/router/transport/UPnP.java
@ -3,14 +3,19 @@
 * http://www.gnu.org/ for further details of the GPL. */
 package net.i2p.router.transport;

+import java.io.Serializable;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.text.Collator;
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
@ -96,8 +101,8 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	// UDN -> friendly name
 	private final Map<String, String> _otherUDNs;
 	private final Map<String, String> _eventVars;
-	private boolean isDisabled = false; // We disable the plugin if more than one IGD is found
 	private volatile boolean _serviceLacksAPM;
+	private volatile boolean _permanentLeasesOnly;
 	private final Object lock = new Object();
 	// FIXME: detect it for real and deal with it! @see #2524
 	private volatile boolean thinksWeAreDoubleNatted = false;
@ -160,6 +165,7 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 			_router = null;
 			_service = null;
 			_serviceLacksAPM = false;
+			_permanentLeasesOnly = false;
 		}
 	}
 	
@ -173,23 +179,23 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	 */
 	public DetectedIP[] getAddress() {
 		_log.info("UP&P.getAddress() is called \\o/");
-		if(isDisabled) {
-			if (_log.shouldLog(Log.WARN))
-				_log.warn("Plugin has been disabled previously, ignoring request.");
-			return null;
-		} else if(!isNATPresent()) {
-			if (_log.shouldLog(Log.WARN))
-				_log.warn("No UP&P device found, detection of the external ip address using the plugin has failed");
-			return null;
+		Service service;
+		synchronized(lock) {
+			if (!isNATPresent()) {
+				if (_log.shouldLog(Log.WARN))
+					_log.warn("No UP&P device found, detection of the external ip address using the plugin has failed");
+				return null;
+			}
+			service = _service;
 		}
 		
-		DetectedIP result = null;
-		final String natAddress = getNATAddress();
+		final String natAddress = getNATAddress(service);
                if (natAddress == null || natAddress.length() <= 0) {
 			if (_log.shouldLog(Log.WARN))
 				_log.warn("No external address returned");
 			return null;
 		}
+		DetectedIP result = null;
 		try {
 			InetAddress detectedIP = InetAddress.getByName(natAddress);

@ -217,41 +223,28 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	 *  DeviceChangeListener
 	 */
 	public void deviceAdded(Device dev) {
+		if (!dev.hasUDN()) {
+			if (_log.shouldInfo())
+				_log.info("Bad device, no UDN");
+			return;
+		}
                String udn = dev.getUDN();
-		if (udn == null)
-			udn = "???";
                String name = dev.getFriendlyName();
 		if (name == null)
-			name = "???";
+			name = udn;
 		String type = dev.getDeviceType();
 		boolean isIGD = (ROUTER_DEVICE.equals(type) || ROUTER_DEVICE_2.equals(type)) && dev.isRootDevice();
 		name += isIGD ? " IGD" : (' ' + type);
 		String ip = getIP(dev);
 		if (ip != null)
 			name += ' ' + ip;
-		synchronized (lock) {
-			if(isDisabled) {
-				if (_log.shouldLog(Log.WARN))
-					_log.warn("Plugin has been disabled previously, ignoring " + name + " UDN: " + udn);
-				_otherUDNs.put(udn, name);
-				return;
-			}
-		}
 		if(!isIGD) {
-			if (_log.shouldLog(Log.WARN))
-				_log.warn("UP&P non-IGD device found, ignoring " + name + ' ' + dev.getDeviceType());
+			if (_log.shouldInfo())
+				_log.info("UP&P non-IGD device found, ignoring " + name + ' ' + dev.getDeviceType());
 			synchronized (lock) {
 				_otherUDNs.put(udn, name);
 			}
 			return; // ignore non-IGD devices
-		} else if(isNATPresent()) {
-                        // maybe we should see if the old one went away before ignoring the new one?
-			// TODO if old one doesn't have an IP address but new one does, switch
-			_log.logAlways(Log.WARN, "UP&P ignoring additional device " + name + " UDN: " + udn);
-			synchronized (lock) {
-				_otherUDNs.put(udn, name);
-			}
-			return;
 		}
 		
 		boolean ignore = false;
@ -261,7 +254,8 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 			for (int i = 0; i < ignores.length; i++) {
 				if (ignores[i].equals(udn)) {
 					ignore = true;
-					_log.logAlways(Log.WARN, "Ignoring by config: " + name + " UDN: " + udn);
+					if (_log.shouldWarn())
+						_log.warn("Ignoring by config: " + name + " UDN: " + udn);
 					break;
 				}
 			}
@ -269,16 +263,52 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 		Set<String> myAddresses = Addresses.getAddresses(true, false);  // yes local, no IPv6
 		if (!ignore && !ALLOW_SAME_HOST && ip != null && myAddresses.contains(ip)) {
 			ignore = true;
-			_log.logAlways(Log.WARN, "Ignoring UPnP on same host: " + name + " UDN: " + udn);
+			if (_log.shouldWarn())
+				_log.warn("Ignoring UPnP on same host: " + name + " UDN: " + udn);
 		}

 		// IP check
-		SSDPPacket pkt = dev.getSSDPPacket();
-		if (!ignore && pkt != null) {
-			String pktIP = pkt.getRemoteAddress();
-			if (!stringEquals(ip, pktIP)) {
+		if (!ignore) {
+			SSDPPacket pkt = dev.getSSDPPacket();
+			if (pkt != null) {
+				String pktIP = pkt.getRemoteAddress();
+				if (!stringEquals(ip, pktIP)) {
+					ignore = true;
+					if (_log.shouldWarn())
+						_log.warn("Ignoring UPnP with IP mismatch: " + name + " UDN: " + udn);
+				}
+			}
+		}
+
+		// Find valid service
+		Service service = null;
+		String extIP = null;
+		boolean subscriptionFailed = false;
+		if (!ignore) {
+			service = discoverService(dev);
+			if (service == null) {
 				ignore = true;
-				_log.logAlways(Log.WARN, "Ignoring UPnP with IP mismatch: " + name + " UDN: " + udn);
+			} else {
+				// does it have an external IP?
+				extIP = getNATAddress(service);
+				if (extIP == null) {
+					// this would meet all our qualifications if connected.
+					// subscribe to it, in case it becomes connected.
+					boolean ok = subscribe(service);
+					if (ok) {
+						ignore = true;
+						if (_log.shouldWarn())
+							_log.warn("UPnP subscribed but ignoring disconnected device " + name + " UDN: " + udn);
+					} else {
+						subscriptionFailed = true;
+						// we can't ignore it, as it won't tell us when it connects
+						if (_log.shouldWarn())
+							_log.warn("Failed subscription to disconnected device " + name + " UDN: " + udn);
+					}
+				} else {
+					if (_log.shouldWarn())
+						_log.warn("UPnP found device " + name + " UDN: " + udn + " with external IP: " + extIP);
+				}
 			}
 		}

@ -286,28 +316,76 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 			if (ignore) {
 				_otherUDNs.put(udn, name);
 				return;
-			} else {
-				_router = dev;
 			}
-		}
+			if (_router != null && _service != null) {
+				ignore = true;
+				String curIP = null;
+				if (extIP != null) {
+					curIP = getNATAddress(_service);
+					if (curIP == null) {
+						// new one is better
+						ignore = false;
+					} else {
+						byte[] cur = Addresses.getIP(curIP);
+						byte[] ext = Addresses.getIP(extIP);
+						if (cur != null && ext != null &&
+						    TransportUtil.isPubliclyRoutable(ext, false) &&
+						    !TransportUtil.isPubliclyRoutable(cur, false)) {
+							// new one is better
+							ignore = false;
+						}
+					}
+				}
+				if (ignore) {
+					// this meets all our qualifications, but we already have one.
+					// subscribe to it, in case ours goes away
+					if (_log.shouldWarn())
+						_log.warn("UPnP ignoring additional device " + name + " UDN: " + udn);
+					_otherUDNs.put(udn, name);
+					if (!subscriptionFailed) {
+						boolean ok = subscribe(service);
+						if (_log.shouldInfo()) {
+							if (ok)
+								_log.info("Subscribed to additional device " + name + " UDN: " + udn);
+							else
+								_log.info("Failed subscription to additional device " + name + " UDN: " + udn);
+						}
+						return;
+					}
+				} else {
+			                String oldudn = _router.getUDN();
+					String oldname = _router.getFriendlyName();
+					if (oldname == null)
+						oldname = "";
+					oldname += " IGD";
+					String oldip = getIP(_router);
+					if (oldip != null)
+						oldname += ' ' + oldip;
+					if (_log.shouldWarn())
+						_log.warn("Replacing device " + oldname + " (external IP " + curIP + ") with new device " + name + " UDN: " + udn + " external IP: " + extIP);
+					_otherUDNs.put(oldudn, oldname);
+				}
+			}

+			// We have found the device we need
+			_otherUDNs.remove(udn);
+			_router = dev;
+			_service = service;
+			_permanentLeasesOnly = false;
+		}
 		if (_log.shouldLog(Log.WARN))
 			_log.warn("UP&P IGD found : " + name + " UDN: " + udn + " lease time: " + dev.getLeaseTime());
 		
-		discoverService();
-		// We have found the device we need: stop the listener thread
-		/// No, let's stick around to get notifications
-		//stop();
-		synchronized(lock) {
-			/// we should look for the next one
-			if(_service == null) {
-				_log.error("The IGD device we got isn't suiting our needs, let's disable the plugin");
-				//isDisabled = true;
-				_router = null;
-				return;
+		if (!subscriptionFailed) {
+			boolean ok = subscribe(service);
+			if (_log.shouldInfo()) {
+				if (ok)
+					_log.info("Subscribed to our device " + name + " UDN: " + udn);
+				else
+					_log.info("Failed subscription to our device " + name + " UDN: " + udn);
 			}
-			subscribe(_service);
 		}
+
 		registerPortMappings();
 	}
 	
@ -323,10 +401,11 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis

 	/**
 	 * Traverses the structure of the router device looking for the port mapping service.
+	 *
+	 * @return the service or null
 	 */
-	private void discoverService() {
-		synchronized (lock) {
-			for (Device current : _router.getDeviceList()) {
+	private Service discoverService(Device router) {
+			for (Device current : router.getDeviceList()) {
 				String type = current.getDeviceType();
 				if (!(WAN_DEVICE.equals(type) || WAN_DEVICE_2.equals(type)))
 					continue;
@ -338,27 +417,28 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 					if (!(WANCON_DEVICE.equals(type) || WANCON_DEVICE_2.equals(type)))
 						continue;
 					
-					_service = current2.getService(WAN_IP_CONNECTION_2);
-					if (_service == null) {
-						_service = current2.getService(WAN_IP_CONNECTION);
-						if (_service == null) {
-							_service = current2.getService(WAN_PPP_CONNECTION);
-							if (_service == null) {
+					Service service = current2.getService(WAN_IP_CONNECTION_2);
+					if (service == null) {
+						service = current2.getService(WAN_IP_CONNECTION);
+						if (service == null) {
+							service = current2.getService(WAN_PPP_CONNECTION);
+							if (service == null) {
 								if (_log.shouldWarn())
 									_log.warn(_router.getFriendlyName() + " doesn't have any recognized connection type; we won't be able to use it!");
 							}
 						}
 					}
-					if (_log.shouldWarn()) {
+					if (_log.shouldInfo()) {
 						Service svc2 = current2.getService(WAN_IPV6_CONNECTION);
 						if (svc2 != null)
-							_log.warn(_router.getFriendlyName() + " supports WANIPv6Connection, but we don't");
+							_log.info(_router.getFriendlyName() + " supports WANIPv6Connection, but we don't");
 					}
-					_serviceLacksAPM = false;
-					return;
+					if (service != null)
+						return service;
 				}
 			}
-		}
+
+		return null;
 	}
 	
 	private boolean tryAddMapping(String protocol, int port, String description, ForwardPort fp) {
@ -394,16 +474,16 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	 *  DeviceChangeListener
 	 */
 	public void deviceRemoved(Device dev ){
+		if (!dev.hasUDN())
+			return;
                String udn = dev.getUDN();
 		if (_log.shouldLog(Log.WARN))
 			_log.warn("UP&P device removed : " + dev.getFriendlyName() + " UDN: " + udn);
 		ForwardPortCallback fpc = null;
 		Map<ForwardPort, ForwardPortStatus> removeMap = null;
+		boolean runSearch = false;
 		synchronized (lock) {
-			if (udn != null)
-				_otherUDNs.remove(udn);
-			else
-				_otherUDNs.remove("???");
+			_otherUDNs.remove(udn);
 			if (_router == null) return;
 			// I2P this wasn't working
 			//if(_router.equals(dev)) {
@ -414,14 +494,12 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 			   stringEquals(_router.getUDN(), udn)) {
 				if (_log.shouldLog(Log.WARN))
 					_log.warn("UP&P IGD device removed : " + dev.getFriendlyName());
-				// TODO promote an IGD from _otherUDNs ??
-				// For now, just clear the others so they can be promoted later
-				// after a rescan.
-				_otherUDNs.clear();
+				runSearch = true;
 				_router = null;
 				_service = null;
 				_eventVars.clear();
 				_serviceLacksAPM = false;
+				_permanentLeasesOnly = false;
 				if (!portsForwarded.isEmpty()) {
 					fpc = forwardCallback;
 					removeMap = new HashMap<ForwardPort, ForwardPortStatus>(portsForwarded.size());
@ -437,6 +515,8 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 		if (fpc != null) {
 			fpc.portForwardStatus(removeMap);
 		}
+		if (runSearch)
+			search();
 	}
 	
 	/**
@ -450,10 +530,12 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 			return;
 		String old = null;
 		synchronized(lock) {
-			if (_service == null || !uuid.equals(_service.getSID()))
-				return;
-			if (_eventVars.size() >= 20 && !_eventVars.containsKey(varName))
+			if (_service == null || !uuid.equals(_service.getSID()) ||
+			    (_eventVars.size() >= 20 && !_eventVars.containsKey(varName))) {
+				if (_log.shouldDebug())
+					_log.debug("Ignoring event from " + uuid + ": " + varName + " changed to " + value);
 				return;
+			}
 			old = _eventVars.put(varName, value);
 		}
 		// The following four variables are "evented":
@ -485,17 +567,12 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	}

 	/**
-	 * @return the external IPv4 address the NAT thinks we have.  Blocking.
-	 * null if we can't find it.
+	 * Blocking.
+	 *
+	 * @param service non-null
+	 * @return the external IPv4 address the NAT thinks we have.  Null if we can't find it.
 	 */
-	private String getNATAddress() {
-		Service service;
-		synchronized(lock) {
-			if(!isNATPresent())
-				return null;
-			service = _service;
-		}
-
+	private String getNATAddress(Service service) {
 		Action getIP = service.getAction("GetExternalIPAddress");
 		if(getIP == null || !getIP.postControlAction())
 			return null;
@ -774,7 +851,7 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	
 	private void listSubDev(String prefix, Device dev, StringBuilder sb){
                if (prefix == null)
-			sb.append("<p>").append(_t("Found Device")).append(": ");
+			sb.append("<p><b>").append(_t("Found Device")).append(":</b> ");
 		else
 			sb.append("<li>").append(_t("Subdevice")).append(": ");
 		sb.append(DataHelper.escapeHTML(dev.getFriendlyName()));
@ -805,11 +882,16 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 		final StringBuilder sb = new StringBuilder();
 		sb.append("<h3 id=\"upnp\">").append(_t("UPnP Status")).append("</h3><div id=\"upnpscan\">");
 		
-		synchronized(_otherUDNs) {
+		Device router;
+		Service service;
+		synchronized(lock) {
 			if (!_otherUDNs.isEmpty()) {
+				sb.append("<b>");
 				sb.append(_t("Disabled UPnP Devices"));
-				sb.append("<ul>");
-				for (Map.Entry<String, String> e : _otherUDNs.entrySet()) {
+				sb.append(":</b><ul>");
+				List<Map.Entry<String, String>> other = new ArrayList<Map.Entry<String, String>>(_otherUDNs.entrySet());
+				Collections.sort(other, new UDNComparator());
+				for (Map.Entry<String, String> e : other) {
 					String udn = e.getKey();
 					String name = e.getValue();
 					sb.append("<li>").append(DataHelper.escapeHTML(name));
@ -818,25 +900,16 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 				}
 				sb.append("</ul>");
 			}
-		}
-
-		if(isDisabled) {
-			sb.append("<p>");
-			sb.append(_t("UPnP has been disabled; Do you have more than one UPnP Internet Gateway Device on your LAN ?"));
-			return sb.toString();
-		} else if(!isNATPresent()) {
-			sb.append("<p>");
-			sb.append(_t("UPnP has not found any UPnP-aware, compatible device on your LAN."));
-			return sb.toString();
-		}
-		
-		Device router;
-		synchronized(lock) {
+			if (!isNATPresent()) {
+				sb.append("<p>");
+				sb.append(_t("UPnP has not found any UPnP-aware, compatible device on your LAN."));
+				return sb.toString();
+			}
 			router = _router;
+			service = _service;
 		}
-		if (router != null)
-			listSubDev(null, router, sb);
-		String addr = getNATAddress();
+		listSubDev(null, router, sb);
+		String addr = getNATAddress(service);
 		sb.append("<p>");
 		if (addr != null)
 		    sb.append(_t("The current external IP address reported by UPnP is {0}", DataHelper.escapeHTML(addr)));
@ -864,6 +937,16 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 		sb.append("</p></div>");
 		return sb.toString();
 	}
+
+	/**
+	 *  Compare based on name, which is the entry value
+	 *  @since 0.9.46
+	 */
+	private static class UDNComparator implements Comparator<Map.Entry<String, String>>, Serializable {
+		public int compare(Map.Entry<String, String> l, Map.Entry<String, String> r) {
+			return Collator.getInstance().compare(l.getValue(), r.getValue());
+		}
+	}
 	
 	/**
 	 *  This always requests that the external port == the internal port, for now.
@ -872,8 +955,8 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	private boolean addMapping(String protocol, int port, String description, ForwardPort fp) {
 		Service service;
 		synchronized(lock) {
-			if(isDisabled || !isNATPresent() || _router == null) {
-				_log.error("Can't addMapping: " + isDisabled + " " + isNATPresent() + " " + _router);
+			if(!isNATPresent() || _router == null) {
+				_log.error("Can't addMapping: " + isNATPresent() + " " + _router);
 				return false;
 			}
 			service = _service;
@ -911,7 +994,8 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 		add.setArgumentValue("NewEnabled","1");
                // 3 hours
                // MUST be longer than max RI republish which is 52 minutes
-		add.setArgumentValue("NewLeaseDuration", 3*60*60);
+		int leaseTime = _permanentLeasesOnly ? 0 : 3*60*60;
+		add.setArgumentValue("NewLeaseDuration", leaseTime);
 		
 		boolean rv = add.postControlAction();
 		if(rv) {
@ -948,11 +1032,24 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis

 		// TODO return error code and description for display

+		if (!rv) {
+			UPnPStatus status = add.getControlStatus();
+			if (status != null) {
+				int controlStatus = status.getCode();
+				if (controlStatus == 725) {
+					if (_log.shouldWarn())
+						_log.warn("UPnP device supports permanent leases only");
+					_permanentLeasesOnly = true;
+		                }
+	                }
+		}
+
 		return rv;
 	}

 	/**
-	*  @return IP or null
+	 * @param dev non-null
+	 * @return The local IP of the device (NOT the external IP) or null
 	 * @since 0.9.34
 	 */
 	private static String getIP(Device dev) {
@ -990,11 +1087,18 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	 *
 	 * So return the address of ours that is closest to his.
 	 *
+	 * @return our adddress or deflt on failure
 	 * @since 0.8.8
 	 */
 	private String getOurAddress(String deflt) {
 		String rv = deflt;
-		String hisIP = getIP(_router);
+		Device router;
+		synchronized(lock) {
+			router = _router;
+		}
+		if (router == null)
+			return rv;
+		String hisIP = getIP(router);
 		if (hisIP == null)
 			return rv;
 		try {
@ -1029,13 +1133,13 @@ public class UPnP extends ControlPoint implements DeviceChangeListener, EventLis
 	private boolean removeMapping(String protocol, int port, ForwardPort fp, boolean noLog) {
 		Service service;
 		synchronized(lock) {
-			if(isDisabled || !isNATPresent()) {
-				_log.error("Can't removeMapping: " + isDisabled + " " + isNATPresent() + " " + _router);
+			if(!isNATPresent()) {
+				_log.error("Can't removeMapping: " + isNATPresent() + " " + _router);
 				return false;
 			}
 			service = _service;
 		}
-		
+
 		Action remove = service.getAction("DeletePortMapping");
 		if(remove == null) {
 		    if (_log.shouldLog(Log.WARN))