New implementation of passive ports, uses random assignments of ports and has more...
authorNiklas Gustavsson <ngn@apache.org>
Sat, 18 Jun 2011 21:33:32 +0000 (21:33 +0000)
committerNiklas Gustavsson <ngn@apache.org>
Sat, 18 Jun 2011 21:33:32 +0000 (21:33 +0000)
git-svn-id: https://svn.apache.org/repos/asf/mina/ftpserver/trunk@1137251 13f79535-47bb-0310-9956-ffa450edef68

core/src/main/java/org/apache/ftpserver/DataConnectionConfigurationFactory.java
core/src/main/java/org/apache/ftpserver/impl/PassivePorts.java
core/src/test/java/org/apache/ftpserver/clienttests/PasvUsedPortTest.java
core/src/test/java/org/apache/ftpserver/impl/PassivePortsTest.java

index 7349921..51d4ca2 100644 (file)
@@ -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;
 
index 19585ec..4d00f12 100644 (file)
@@ -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(",");
         }
index 0049242..ab3635b 100644 (file)
@@ -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());
     }
 }
index 1167e20..3c48944 100644 (file)
 
 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