New implementation of passive ports, uses random assignments of ports and has more efficient handling of wide passive port ranges. Thanks to Allen Firstenberg for the implementation! (FTPSERVER-420, FTPSERVER-419)
git-svn-id: https://svn.apache.org/repos/asf/mina/ftpserver/trunk@1137251 13f79535-47bb-0310-9956-ffa450edef68 Project: http://git-wip-us.apache.org/repos/asf/mina-ftpserver/repo Commit: http://git-wip-us.apache.org/repos/asf/mina-ftpserver/commit/4907aac4 Tree: http://git-wip-us.apache.org/repos/asf/mina-ftpserver/tree/4907aac4 Diff: http://git-wip-us.apache.org/repos/asf/mina-ftpserver/diff/4907aac4 Branch: refs/heads/trunk Commit: 4907aac4b48fdd5fd64a6044e9f574216e8ec4a8 Parents: e09a432 Author: Niklas Gustavsson <[email protected]> Authored: Sat Jun 18 21:33:32 2011 +0000 Committer: Niklas Gustavsson <[email protected]> Committed: Sat Jun 18 21:33:32 2011 +0000 ---------------------------------------------------------------------- .../DataConnectionConfigurationFactory.java | 3 +- .../org/apache/ftpserver/impl/PassivePorts.java | 134 ++++++++------- .../ftpserver/clienttests/PasvUsedPortTest.java | 5 - .../apache/ftpserver/impl/PassivePortsTest.java | 165 ++++++++----------- 4 files changed, 145 insertions(+), 162 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mina-ftpserver/blob/4907aac4/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java b/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java index 7349921..51d4ca2 100644 --- a/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java +++ b/core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java @@ -21,6 +21,7 @@ package org.apache.ftpserver; import java.net.InetAddress; import java.net.UnknownHostException; +import java.util.Collections; import org.apache.ftpserver.impl.DefaultDataConnectionConfiguration; import org.apache.ftpserver.impl.PassivePorts; @@ -48,7 +49,7 @@ public class DataConnectionConfigurationFactory { private String passiveAddress; private String passiveExternalAddress; - private PassivePorts passivePorts = new PassivePorts(new int[] { 0 }, true); + private PassivePorts passivePorts = new PassivePorts(Collections.<Integer>emptySet(), true); private boolean passiveIpCheck = false; private boolean implicitSsl; http://git-wip-us.apache.org/repos/asf/mina-ftpserver/blob/4907aac4/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java b/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java index 19585ec..4d00f12 100644 --- a/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java +++ b/core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java @@ -22,10 +22,15 @@ package org.apache.ftpserver.impl; import java.io.IOException; import java.net.ServerSocket; import java.util.ArrayList; -import java.util.Iterator; +import java.util.HashSet; import java.util.List; +import java.util.Random; +import java.util.Set; import java.util.StringTokenizer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** * <strong>Internal class, do not use directly.</strong> * @@ -36,18 +41,22 @@ import java.util.StringTokenizer; */ public class PassivePorts { + private Logger log = LoggerFactory.getLogger(PassivePorts.class); + private static final int MAX_PORT = 65535; private static final Integer MAX_PORT_INTEGER = Integer.valueOf(MAX_PORT); - private int[] passivePorts; + private List<Integer> freeList; + + private Set<Integer> usedList; - private boolean[] reservedPorts; + private Random r = new Random(); private String passivePortsString; private boolean checkIfBound; - + /** * Parse a string containing passive ports * @@ -57,13 +66,13 @@ public class PassivePorts { * 123,124,125) or ranges of ports, including open ended ranges * (e.g. 123-125, 30000-, -1023). Combinations for single ports * and ranges is also supported. - * @return An int[] array based on the parsed string + * @return A list of Integer objects, based on the parsed string * @throws IllegalArgumentException * If any of of the ports in the string is invalid (e.g. not an * integer or too large for a port number) */ - private static int[] parse(final String portsString) { - List<Integer> passivePortsList = new ArrayList<Integer>(); + private static Set<Integer> parse(final String portsString) { + Set<Integer> passivePortsList = new HashSet<Integer>(); boolean inRange = false; Integer lastPort = Integer.valueOf(1); @@ -86,7 +95,7 @@ public class PassivePorts { } else { Integer port = Integer.valueOf(token); - verifyPort(port.intValue()); + verifyPort(port); if (inRange) { // add all numbers from last int @@ -105,26 +114,14 @@ public class PassivePorts { fillRange(passivePortsList, lastPort, MAX_PORT_INTEGER); } - int[] passivePorts = new int[passivePortsList.size()]; - - Iterator<Integer> iter = passivePortsList.iterator(); - - int counter = 0; - while (iter.hasNext()) { - Integer port = iter.next(); - passivePorts[counter] = port.intValue(); - counter++; - } - - return passivePorts; + return passivePortsList; } /** * Fill a range of ports */ - private static void fillRange(final List<Integer> passivePortsList, - final Integer beginPort, final Integer endPort) { - for (int i = beginPort.intValue(); i <= endPort.intValue(); i++) { + private static void fillRange(final Set<Integer> passivePortsList, final Integer beginPort, final Integer endPort) { + for (int i = beginPort; i <= endPort; i++) { addPort(passivePortsList, Integer.valueOf(i)); } } @@ -132,11 +129,8 @@ public class PassivePorts { /** * Add a single port if not already in list */ - private static void addPort(final List<Integer> passivePortsList, - final Integer rangePort) { - if (!passivePortsList.contains(rangePort)) { - passivePortsList.add(rangePort); - } + private static void addPort(final Set<Integer> passivePortsList, final Integer port) { + passivePortsList.add(port); } /** @@ -144,8 +138,7 @@ public class PassivePorts { */ private static void verifyPort(final int port) { if (port < 0) { - throw new IllegalArgumentException("Port can not be negative: " - + port); + throw new IllegalArgumentException("Port can not be negative: " + port); } else if (port > MAX_PORT) { throw new IllegalArgumentException("Port too large: " + port); } @@ -157,14 +150,17 @@ public class PassivePorts { this.passivePortsString = passivePorts; } - public PassivePorts(final int[] passivePorts, boolean checkIfBound) { - if(passivePorts == null) { - throw new NullPointerException("passivePorts can not be null"); - } - - this.passivePorts = passivePorts.clone(); + public PassivePorts(Set<Integer> passivePorts, boolean checkIfBound) { + if (passivePorts == null) { + throw new NullPointerException("passivePorts can not be null"); + } else if(passivePorts.isEmpty()) { + passivePorts = new HashSet<Integer>(); + passivePorts.add(0); + } + + this.freeList = new ArrayList<Integer>(passivePorts); + this.usedList = new HashSet<Integer>(passivePorts.size()); - reservedPorts = new boolean[passivePorts.length]; this.checkIfBound = checkIfBound; } @@ -173,15 +169,15 @@ public class PassivePorts { */ private boolean checkPortUnbound(int port) { // is this check disabled? - if(!checkIfBound) { + if (!checkIfBound) { return true; } - + // if using 0 port, it will always be available - if(port == 0) { + if (port == 0) { return true; } - + ServerSocket ss = null; try { ss = new ServerSocket(port); @@ -191,7 +187,7 @@ public class PassivePorts { // port probably in use, check next return false; } finally { - if(ss != null) { + if (ss != null) { try { ss.close(); } catch (IOException e) { @@ -201,27 +197,49 @@ public class PassivePorts { } } } - - public int reserveNextPort() { - // search for a free port - for (int i = 0; i < passivePorts.length; i++) { - if (!reservedPorts[i] && checkPortUnbound(passivePorts[i])) { - if (passivePorts[i] != 0) { - reservedPorts[i] = true; - } - return passivePorts[i]; + + public synchronized int reserveNextPort() { + // create a copy of the free ports, so that we can keep track of the tested ports + List<Integer> freeCopy = new ArrayList<Integer>(freeList); + + // Loop until we have found a port, or exhausted all available ports + while (freeCopy.size() > 0) { + // Otherwise, pick one at random + int i = r.nextInt(freeCopy.size()); + Integer ret = freeCopy.get(i); + + if (ret == 0) { + // "Any" port should not be removed from our free list, + // nor added to the used list + return 0; + + } else if (checkPortUnbound(ret)) { + // Not used by someone else, so lets reserve it and return it + freeList.remove(i); + usedList.add(ret); + return ret; + + } else { + freeCopy.remove(i); + // log port unavailable, but left in pool + log.warn("Passive port in use by another process: " + ret); } } return -1; } - public void releasePort(final int port) { - for (int i = 0; i < passivePorts.length; i++) { - if (passivePorts[i] == port) { - reservedPorts[i] = false; - break; - } + public synchronized void releasePort(final int port) { + if (port == 0) { + // Ignore port 0 being released, + // since its not put on the used list + + } else if (usedList.remove(port)) { + freeList.add(port); + + } else { + // log attempt to release unused port + log.warn("Releasing unreserved passive port: " + port); } } @@ -233,7 +251,7 @@ public class PassivePorts { StringBuilder sb = new StringBuilder(); - for (int port : passivePorts) { + for (Integer port : freeList) { sb.append(port); sb.append(","); } http://git-wip-us.apache.org/repos/asf/mina-ftpserver/blob/4907aac4/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java b/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java index 0049242..ab3635b 100644 --- a/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java +++ b/core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java @@ -68,10 +68,5 @@ public class PasvUsedPortTest extends ClientTestTemplate { // close blocking socket ss.close(); - - client.connect("localhost", getListenerPort()); - client.login(ADMIN_USERNAME, ADMIN_PASSWORD); - client.pasv(); - assertEquals("227 Entering Passive Mode (127,0,0,1,48,156)", client.getReplyString().trim()); } } http://git-wip-us.apache.org/repos/asf/mina-ftpserver/blob/4907aac4/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java b/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java index 1167e20..3c48944 100644 --- a/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java +++ b/core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java @@ -19,6 +19,12 @@ package org.apache.ftpserver.impl; +import java.io.IOException; +import java.net.ServerSocket; +import java.util.ArrayList; +import java.util.List; + +import junit.framework.AssertionFailedError; import junit.framework.TestCase; /** @@ -69,146 +75,109 @@ public class PassivePortsTest extends TestCase { // ok } } + + private void assertContains( List<Integer> valid, Integer testVal ){ + if( !valid.remove(testVal) ){ + throw new AssertionFailedError( "Did not find "+testVal+" in valid list "+valid ); + } + } + + private void assertReserveAll(String portString, int... validPorts) { + PassivePorts ports = new PassivePorts(portString, false); + + List<Integer> valid = valid(validPorts); - public void testParseListOfValues() { - PassivePorts ports = new PassivePorts("123, 456,\t\n789", false); - - assertEquals(123, ports.reserveNextPort()); - assertEquals(456, ports.reserveNextPort()); - assertEquals(789, ports.reserveNextPort()); + int len = valid.size(); + for(int i = 0; i<len; i++) { + assertContains(valid, ports.reserveNextPort()); + } assertEquals(-1, ports.reserveNextPort()); - } + assertTrue(valid.isEmpty()); - public void testParseListOfValuesOrder() { - PassivePorts ports = new PassivePorts("123, 789, 456", false); + } + + private List<Integer> valid(int... ints) { + List<Integer> valid = new ArrayList<Integer>(); + for(int i : ints) { + valid.add(i); + } + return valid; + } - assertEquals(123, ports.reserveNextPort()); - assertEquals(789, ports.reserveNextPort()); - assertEquals(456, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + public void testParseListOfValues() { + assertReserveAll("123, 456,\t\n789", 123, 456, 789); } public void testParseListOfValuesDuplicate() { - PassivePorts ports = new PassivePorts("123, 789, 456, 789", false); - - assertEquals(123, ports.reserveNextPort()); - assertEquals(789, ports.reserveNextPort()); - assertEquals(456, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("123, 789, 456, 789", 123, 456, 789); } public void testParseSimpleRange() { - PassivePorts ports = new PassivePorts("123-125", false); - - assertEquals(123, ports.reserveNextPort()); - assertEquals(124, ports.reserveNextPort()); - assertEquals(125, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("123-125", 123, 124, 125); } public void testParseMultipleRanges() { - PassivePorts ports = new PassivePorts("123-125, 127-128, 130-132", false); - - assertEquals(123, ports.reserveNextPort()); - assertEquals(124, ports.reserveNextPort()); - assertEquals(125, ports.reserveNextPort()); - assertEquals(127, ports.reserveNextPort()); - assertEquals(128, ports.reserveNextPort()); - assertEquals(130, ports.reserveNextPort()); - assertEquals(131, ports.reserveNextPort()); - assertEquals(132, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("123-125, 127-128, 130-132", 123, 124, 125, 127, 128, 130, 131, 132); } public void testParseMixedRangeAndSingle() { - PassivePorts ports = new PassivePorts("123-125, 126, 128-129", false); - - assertEquals(123, ports.reserveNextPort()); - assertEquals(124, ports.reserveNextPort()); - assertEquals(125, ports.reserveNextPort()); - assertEquals(126, ports.reserveNextPort()); - assertEquals(128, ports.reserveNextPort()); - assertEquals(129, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("123-125, 126, 128-129", 123, 124, 125, 126, 128, 129); } public void testParseOverlapingRanges() { - PassivePorts ports = new PassivePorts("123-125, 124-126", false); - - assertEquals(123, ports.reserveNextPort()); - assertEquals(124, ports.reserveNextPort()); - assertEquals(125, ports.reserveNextPort()); - assertEquals(126, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("123-125, 124-126", 123, 124, 125, 126); } public void testParseOverlapingRangesorder() { - PassivePorts ports = new PassivePorts("124-126, 123-125", false); - - assertEquals(124, ports.reserveNextPort()); - assertEquals(125, ports.reserveNextPort()); - assertEquals(126, ports.reserveNextPort()); - assertEquals(123, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("124-126, 123-125", 123, 124, 125, 126); } public void testParseOpenLowerRange() { - PassivePorts ports = new PassivePorts("9, -3", false); - - assertEquals(9, ports.reserveNextPort()); - assertEquals(1, ports.reserveNextPort()); - assertEquals(2, ports.reserveNextPort()); - assertEquals(3, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("9, -3", 1, 2, 3, 9); } public void testParseOpenUpperRange() { - PassivePorts ports = new PassivePorts("65533-", false); - - assertEquals(65533, ports.reserveNextPort()); - assertEquals(65534, ports.reserveNextPort()); - assertEquals(65535, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("65533-", 65533, 65534, 65535); } public void testParseOpenUpperRange3() { - PassivePorts ports = new PassivePorts("65533-, 65532-", false); - - assertEquals(65533, ports.reserveNextPort()); - assertEquals(65534, ports.reserveNextPort()); - assertEquals(65535, ports.reserveNextPort()); - assertEquals(65532, ports.reserveNextPort()); - assertEquals(-1, ports.reserveNextPort()); + assertReserveAll("65533-, 65532-", 65532, 65533, 65534, 65535); } public void testParseOpenUpperRange2() { - PassivePorts ports = new PassivePorts("65533-, 1", false); + assertReserveAll("65533-, 1", 1, 65533, 65534, 65535); + } + + public void testReserveNextPortBound() throws IOException { + ServerSocket ss = new ServerSocket(0); + + PassivePorts ports = new PassivePorts(Integer.toString(ss.getLocalPort()), true); - assertEquals(65533, ports.reserveNextPort()); - assertEquals(65534, ports.reserveNextPort()); - assertEquals(65535, ports.reserveNextPort()); - assertEquals(1, ports.reserveNextPort()); assertEquals(-1, ports.reserveNextPort()); + + ss.close(); + + assertEquals(ss.getLocalPort(), ports.reserveNextPort()); } + public void testParseRelease() { PassivePorts ports = new PassivePorts("123, 456,789", false); - assertEquals(123, ports.reserveNextPort()); - assertEquals(456, ports.reserveNextPort()); - ports.releasePort(456); - assertEquals(456, ports.reserveNextPort()); + List<Integer> valid = valid(123, 456, 789); + + assertContains(valid, ports.reserveNextPort()); + + int port = ports.reserveNextPort(); + assertContains(valid, port); + ports.releasePort(port); + valid.add(port); + assertContains(valid, ports.reserveNextPort()); + + assertContains(valid, ports.reserveNextPort()); - assertEquals(789, ports.reserveNextPort()); assertEquals(-1, ports.reserveNextPort()); + assertEquals(0, valid.size()); } - public void testNullPorts() { - try { - new PassivePorts((int[])null, false); - fail("Must throw NPE"); - } catch(NullPointerException e) { - // ok - } - } -} +} \ No newline at end of file
