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
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 @@
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 @@
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;
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 @@
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 @@
*/
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 boolean[] reservedPorts;
+ private Set<Integer> usedList;
+
+ private Random r = new Random();
private String passivePortsString;
private boolean checkIfBound;
-
+
/**
* Parse a string containing passive ports
*
@@ -57,13 +66,13 @@
* 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 @@
} else {
Integer port = Integer.valueOf(token);
- verifyPort(port.intValue());
+ verifyPort(port);
if (inRange) {
// add all numbers from last int
@@ -105,26 +114,14 @@
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 @@
/**
* 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 @@
*/
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 @@
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);
+ }
- reservedPorts = new boolean[passivePorts.length];
+ this.freeList = new ArrayList<Integer>(passivePorts);
+ this.usedList = new HashSet<Integer>(passivePorts.size());
+
this.checkIfBound = checkIfBound;
}
@@ -173,15 +169,15 @@
*/
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 @@
// 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 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 @@
StringBuilder sb = new StringBuilder();
- for (int port : passivePorts) {
+ for (Integer port : freeList) {
sb.append(port);
sb.append(",");
}
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 @@
// 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());
}
}
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 @@
// 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());
+
+ }
+
+ private List<Integer> valid(int... ints) {
+ List<Integer> valid = new ArrayList<Integer>();
+ for(int i : ints) {
+ valid.add(i);
+ }
+ return valid;
}
- public void testParseListOfValuesOrder() {
- PassivePorts ports = new PassivePorts("123, 789, 456", false);
-
- 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);
-
- assertEquals(65533, ports.reserveNextPort());
- assertEquals(65534, ports.reserveNextPort());
- assertEquals(65535, ports.reserveNextPort());
- assertEquals(1, ports.reserveNextPort());
- assertEquals(-1, ports.reserveNextPort());
+ 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(-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);
- assertEquals(789, ports.reserveNextPort());
+ 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(-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
