[SSHD-839] Use correct bound address to unbind local tunnel endpoint
Project: http://git-wip-us.apache.org/repos/asf/mina-sshd/repo Commit: http://git-wip-us.apache.org/repos/asf/mina-sshd/commit/463658a9 Tree: http://git-wip-us.apache.org/repos/asf/mina-sshd/tree/463658a9 Diff: http://git-wip-us.apache.org/repos/asf/mina-sshd/diff/463658a9 Branch: refs/heads/master Commit: 463658a945a75251d455da35374fbde0402877c8 Parents: aa7547b Author: Goldstein Lyor <[email protected]> Authored: Wed Aug 22 14:10:25 2018 +0300 Committer: Goldstein Lyor <[email protected]> Committed: Sun Aug 26 08:50:42 2018 +0300 ---------------------------------------------------------------------- .../common/forward/DefaultForwardingFilter.java | 60 +++++++++++++------- .../sshd/common/util/net/SshdSocketAddress.java | 9 +++ 2 files changed, 47 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/463658a9/sshd-core/src/main/java/org/apache/sshd/common/forward/DefaultForwardingFilter.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/common/forward/DefaultForwardingFilter.java b/sshd-core/src/main/java/org/apache/sshd/common/forward/DefaultForwardingFilter.java index 3e87278..42fdce0 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/forward/DefaultForwardingFilter.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/forward/DefaultForwardingFilter.java @@ -95,7 +95,10 @@ public class DefaultForwardingFilter private final ConnectionService service; private final IoHandlerFactory socksProxyIoHandlerFactory = () -> new SocksProxy(getConnectionService()); private final Session sessionInstance; + private final Object localLock = new Object(); private final Map<Integer, SshdSocketAddress> localToRemote = new TreeMap<>(Comparator.naturalOrder()); + private final Map<Integer, InetSocketAddress> boundLocals = new TreeMap<>(Comparator.naturalOrder()); + private final Map<Integer, SshdSocketAddress> remoteToLocal = new TreeMap<>(Comparator.naturalOrder()); private final Map<Integer, SocksProxy> dynamicLocal = new TreeMap<>(Comparator.naturalOrder()); private final Set<LocalForwardingEntry> localForwards = new HashSet<>(); @@ -191,23 +194,29 @@ public class DefaultForwardingFilter throw new IllegalStateException("TcpipForwarder is closing"); } - InetSocketAddress bound; + InetSocketAddress bound = null; int port; signalEstablishingExplicitTunnel(local, remote, true); try { bound = doBind(local, staticIoHandlerFactory); port = bound.getPort(); - SshdSocketAddress prev; - synchronized (localToRemote) { - prev = localToRemote.put(port, remote); - } + synchronized (localLock) { + SshdSocketAddress prevRemote = localToRemote.get(port); + if (prevRemote != null) { + throw new IOException("Multiple local port forwarding addressing on port=" + port + ": current=" + remote + ", previous=" + prevRemote); + } - if (prev != null) { - throw new IOException("Multiple local port forwarding bindings on port=" + port + ": current=" + remote + ", previous=" + prev); + InetSocketAddress prevBound = boundLocals.get(port); + if (prevBound != null) { + throw new IOException("Multiple local port forwarding bindings on port=" + port + ": current=" + bound + ", previous=" + prevBound); + } + + localToRemote.put(port, remote); + boundLocals.put(port, bound); } } catch (IOException | RuntimeException e) { try { - stopLocalPortForwarding(local); + unbindLocalForwarding(local, remote, bound); } catch (IOException | RuntimeException err) { e.addSuppressed(err); } @@ -232,29 +241,36 @@ public class DefaultForwardingFilter public synchronized void stopLocalPortForwarding(SshdSocketAddress local) throws IOException { Objects.requireNonNull(local, "Local address is null"); - SshdSocketAddress bound; + SshdSocketAddress remote; + InetSocketAddress bound; int port = local.getPort(); - synchronized (localToRemote) { - bound = localToRemote.remove(port); + synchronized (localLock) { + remote = localToRemote.remove(port); + bound = boundLocals.remove(port); } + unbindLocalForwarding(local, remote, bound); + } + + protected void unbindLocalForwarding(SshdSocketAddress local, SshdSocketAddress remote, InetSocketAddress bound) throws IOException { if ((bound != null) && (acceptor != null)) { if (log.isDebugEnabled()) { - log.debug("stopLocalPortForwarding(" + local + ") unbind " + bound); + log.debug("unbindLocalForwarding(" + local + " => " + remote + ") unbind " + bound); } - signalTearingDownExplicitTunnel(bound, true); + SshdSocketAddress boundAddress = new SshdSocketAddress(bound); + signalTearingDownExplicitTunnel(boundAddress, true); try { - acceptor.unbind(bound.toInetSocketAddress()); + acceptor.unbind(bound); } catch (RuntimeException e) { - signalTornDownExplicitTunnel(bound, true, e); + signalTornDownExplicitTunnel(boundAddress, true, e); throw e; } - signalTornDownExplicitTunnel(bound, true, null); + signalTornDownExplicitTunnel(boundAddress, true, null); } else { if (log.isDebugEnabled()) { - log.debug("stopLocalPortForwarding(" + local + ") no mapping/acceptor for " + bound); + log.debug("unbindLocalForwarding(" + local + " => " + remote + ") no mapping/acceptor for " + bound); } } } @@ -284,14 +300,14 @@ public class DefaultForwardingFilter } port = (remotePort == 0) ? result.getInt() : remote.getPort(); // TODO: Is it really safe to only store the local address after the request ? - SshdSocketAddress prev; synchronized (remoteToLocal) { - prev = remoteToLocal.put(port, local); + SshdSocketAddress prev = remoteToLocal.get(port); + if (prev != null) { + throw new IOException("Multiple remote port forwarding bindings on port=" + port + ": current=" + remote + ", previous=" + prev); + } + remoteToLocal.put(port, local); } - if (prev != null) { - throw new IOException("Multiple remote port forwarding bindings on port=" + port + ": current=" + remote + ", previous=" + prev); - } } catch (IOException | RuntimeException e) { try { stopRemotePortForwarding(remote); http://git-wip-us.apache.org/repos/asf/mina-sshd/blob/463658a9/sshd-core/src/main/java/org/apache/sshd/common/util/net/SshdSocketAddress.java ---------------------------------------------------------------------- diff --git a/sshd-core/src/main/java/org/apache/sshd/common/util/net/SshdSocketAddress.java b/sshd-core/src/main/java/org/apache/sshd/common/util/net/SshdSocketAddress.java index 3a40d20..b465d14 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/util/net/SshdSocketAddress.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/util/net/SshdSocketAddress.java @@ -144,6 +144,15 @@ public class SshdSocketAddress extends SocketAddress { this(IPV4_ANYADDR, port); } + public SshdSocketAddress(InetSocketAddress addr) { + Objects.requireNonNull(addr, "No address provided"); + + String host = addr.getHostString(); + hostName = GenericUtils.isEmpty(host) ? IPV4_ANYADDR : host; + port = addr.getPort(); + ValidateUtils.checkTrue(port >= 0, "Port must be >= 0: %d", port); + } + public SshdSocketAddress(String hostName, int port) { Objects.requireNonNull(hostName, "Host name may not be null"); this.hostName = GenericUtils.isEmpty(hostName) ? IPV4_ANYADDR : hostName;
