You are viewing a plain text version of this content. The canonical link for it is here.
Posted to ftpserver-users@mina.apache.org by Niklas Gustavsson <ni...@protocol7.com> on 2010/03/24 20:11:26 UTC
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fr...@gmail.com> wrote:
> we found an issue related to requestPassivePort() which may lead to an
> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced.
>
> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full description
> of the symptoms and a minimalist java client and server to reproduce it.
I haven't yet looked closer at the code you attached. But, I have seen
similar behavior myself during performance testing FtpServer. In that
case, I had a very similar behavior at around 20 threads. However, the
reason for the problem in that test was that the test case uses up
file handles (for the sockets) so fast that they will run out. Since
sockets hang around for some time also after closing, they were not
freed quickly enough and thus FtpServer could not open new ones. Could
you please verify that this is not the case here? You could look at
netstat output and look into increasing the allowed number of file
handles our the timeout time for sockets in your OS.
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by David Latorre <dv...@gmail.com>.
Please note that the patch is not even worh considering...for
instance, i am trying to close the port without any checks that we
are using passive mode!
As Sai suggests, if that's ok for you I would simply return an error
code if there are no free available ports in 1.0.x. But, since some
users (Fred) are expecting that PASV requests get enqueued I don't
know if we should change FTPServer behaviour at this moment.
2010/3/25 Sai Pullabhotla <sa...@jmethods.com>:
> I would like to make a comment on the current implementation -
>
> Do you think we want to wait indefinitely when there is no available
> passive port, or should we have a timed wait, say 60 seconds. If we
> can't get an available passive port in the timeout period, we should
> send an error back to the client. This is mostly based on the fact
> that most FTP clients would have their data connection timeout set,
> which is very small. Or even better, since the current version is not
> supposed to be used with too little passive ports than the number of
> concurrent clients, should we error out right way with 4XX message
> indicating a data connection could not be opened?
>
> Regards,
> Sai Pullabhotla
>
>
>
>
>
> On Thu, Mar 25, 2010 at 7:51 AM, Niklas Gustavsson <ni...@protocol7.com> wrote:
>> Interesting! Could you send the patch without whitespace/formatting
>> changes, would make it much easier to follow.
>>
>> /niklas
>>
>> On Thu, Mar 25, 2010 at 1:46 PM, David Latorre <dv...@gmail.com> wrote:
>>> hello Sai,
>>>
>>> I didn't have time to fully review the code but i made these nearly
>>> random changes in IODataConnectionFactory and
>>> DefaultDataConnectionConfiguration and the server is not getting
>>> blocked anymore...
>>>
>>>
>>>
>>> Index: main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java
>>> ===================================================================
>>> --- main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (revision
>>> 927354)
>>> +++ main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (working
>>> copy)
>>> @@ -129,7 +129,8 @@
>>> * port will be used.
>>> */
>>> public synchronized int requestPassivePort() {
>>> - int dataPort = -1;
>>> +
>>> + int dataPort = -1;
>>> int loopTimes = 2;
>>> Thread currThread = Thread.currentThread();
>>>
>>> @@ -164,8 +165,7 @@
>>> */
>>> public synchronized void releasePassivePort(final int port) {
>>> passivePorts.releasePort(port);
>>> -
>>> - notify();
>>> + notifyAll();
>>> }
>>>
>>> /**
>>> Index: main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java
>>> ===================================================================
>>> --- main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (revision
>>> 927354)
>>> +++ main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (working
>>> copy)
>>> @@ -44,446 +44,459 @@
>>> *
>>> * We can get the FTP data connection using this class. It uses either PORT or
>>> * PASV command.
>>> - *
>>> + *
>>> * @author <a href="http://mina.apache.org">Apache MINA Project</a>
>>> */
>>> public class IODataConnectionFactory implements ServerDataConnectionFactory {
>>>
>>> - private final Logger LOG = LoggerFactory
>>> - .getLogger(IODataConnectionFactory.class);
>>> + private final Logger LOG = LoggerFactory
>>> + .getLogger(IODataConnectionFactory.class);
>>>
>>> - private FtpServerContext serverContext;
>>> + private FtpServerContext serverContext;
>>>
>>> - private Socket dataSoc;
>>> + private Socket dataSoc;
>>>
>>> - ServerSocket servSoc;
>>> + ServerSocket servSoc;
>>>
>>> - InetAddress address;
>>> + InetAddress address;
>>>
>>> - int port = 0;
>>> + int port = 0;
>>>
>>> - long requestTime = 0L;
>>> + long requestTime = 0L;
>>>
>>> - boolean passive = false;
>>> + boolean passive = false;
>>>
>>> - boolean secure = false;
>>> + boolean secure = false;
>>>
>>> - private boolean isZip = false;
>>> + private boolean isZip = false;
>>>
>>> - InetAddress serverControlAddress;
>>> + InetAddress serverControlAddress;
>>>
>>> - FtpIoSession session;
>>> + FtpIoSession session;
>>>
>>> - public IODataConnectionFactory(final FtpServerContext serverContext,
>>> - final FtpIoSession session) {
>>> - this.session = session;
>>> - this.serverContext = serverContext;
>>> - if (session.getListener().getDataConnectionConfiguration()
>>> - .isImplicitSsl()) {
>>> - secure = true;
>>> - }
>>> - }
>>> + public IODataConnectionFactory(final FtpServerContext serverContext,
>>> + final FtpIoSession session) {
>>> + this.session = session;
>>> + this.serverContext = serverContext;
>>> + if (session.getListener().getDataConnectionConfiguration()
>>> + .isImplicitSsl()) {
>>> + secure = true;
>>> + }
>>> + }
>>>
>>> - /**
>>> - * Close data socket.
>>> - * This method must be idempotent as we might call it multiple
>>> times during disconnect.
>>> - */
>>> - public synchronized void closeDataConnection() {
>>> + /**
>>> + * Close data socket. This method must be idempotent as we might call it
>>> + * multiple times during disconnect.
>>> + */
>>> + public synchronized void closeDataConnection() {
>>>
>>> - // close client socket if any
>>> - if (dataSoc != null) {
>>> - try {
>>> - dataSoc.close();
>>> - } catch (Exception ex) {
>>> - LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>>> - }
>>> - dataSoc = null;
>>> - }
>>> + // close client socket if any
>>> + if (dataSoc != null) {
>>> + try {
>>> + dataSoc.close();
>>> + } catch (Exception ex) {
>>> + LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>>> + }
>>> + dataSoc = null;
>>> + }
>>>
>>> - // close server socket if any
>>> - if (servSoc != null) {
>>> - try {
>>> - servSoc.close();
>>> - } catch (Exception ex) {
>>> - LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>>> - }
>>> + // close server socket if any
>>> + if (servSoc != null) {
>>> + try {
>>> + servSoc.close();
>>> + } catch (Exception ex) {
>>> + LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>>> + }
>>>
>>> - if (session != null) {
>>> - DataConnectionConfiguration dcc = session.getListener()
>>> - .getDataConnectionConfiguration();
>>> - if (dcc != null) {
>>> - dcc.releasePassivePort(port);
>>> - }
>>> - }
>>> + servSoc = null;
>>> + }
>>>
>>> - servSoc = null;
>>> - }
>>> + // release passive ports
>>> + if (session != null) {
>>> + DataConnectionConfiguration dcc = session.getListener()
>>> + .getDataConnectionConfiguration();
>>> + if (dcc != null) {
>>> + dcc.releasePassivePort(port);
>>> + }
>>> + }
>>> +
>>> + // reset request time
>>> + requestTime = 0L;
>>> + }
>>>
>>> - // reset request time
>>> - requestTime = 0L;
>>> - }
>>> + /**
>>> + * Port command.
>>> + */
>>> + public synchronized void initActiveDataConnection(
>>> + final InetSocketAddress address) {
>>>
>>> - /**
>>> - * Port command.
>>> - */
>>> - public synchronized void initActiveDataConnection(
>>> - final InetSocketAddress address) {
>>> + // close old sockets if any
>>> + closeDataConnection();
>>>
>>> - // close old sockets if any
>>> - closeDataConnection();
>>> + // set variables
>>> + passive = false;
>>> + this.address = address.getAddress();
>>> + port = address.getPort();
>>> + requestTime = System.currentTimeMillis();
>>> + }
>>>
>>> - // set variables
>>> - passive = false;
>>> - this.address = address.getAddress();
>>> - port = address.getPort();
>>> - requestTime = System.currentTimeMillis();
>>> - }
>>> + private SslConfiguration getSslConfiguration() {
>>> + DataConnectionConfiguration dataCfg = session.getListener()
>>> + .getDataConnectionConfiguration();
>>>
>>> - private SslConfiguration getSslConfiguration() {
>>> - DataConnectionConfiguration dataCfg = session.getListener()
>>> - .getDataConnectionConfiguration();
>>> + SslConfiguration configuration = dataCfg.getSslConfiguration();
>>>
>>> - SslConfiguration configuration = dataCfg.getSslConfiguration();
>>> + // fall back if no configuration has been provided on the data
>>> + // connection config
>>> + if (configuration == null) {
>>> + configuration = session.getListener().getSslConfiguration();
>>> + }
>>>
>>> - // fall back if no configuration has been provided on the
>>> data connection config
>>> - if (configuration == null) {
>>> - configuration = session.getListener().getSslConfiguration();
>>> - }
>>> + return configuration;
>>> + }
>>>
>>> - return configuration;
>>> - }
>>> + /**
>>> + * Initiate a data connection in passive mode (server listening).
>>> + */
>>> + public synchronized InetSocketAddress initPassiveDataConnection()
>>> + throws DataConnectionException {
>>> + LOG.debug("Initiating passive data connection");
>>> + // close old sockets if any
>>> + closeDataConnection();
>>>
>>> - /**
>>> - * Initiate a data connection in passive mode (server listening).
>>> - */
>>> - public synchronized InetSocketAddress initPassiveDataConnection()
>>> - throws DataConnectionException {
>>> - LOG.debug("Initiating passive data connection");
>>> - // close old sockets if any
>>> - closeDataConnection();
>>> + // get the passive port
>>> + int passivePort = session.getListener()
>>> + .getDataConnectionConfiguration().requestPassivePort();
>>> + if (passivePort == -1) {
>>> + servSoc = null;
>>> + throw new DataConnectionException(
>>> + "Cannot find an available passive port.");
>>> + }
>>>
>>> - // get the passive port
>>> - int passivePort = session.getListener()
>>> - .getDataConnectionConfiguration().requestPassivePort();
>>> - if (passivePort == -1) {
>>> - servSoc = null;
>>> - throw new DataConnectionException(
>>> - "Cannot find an available passive port.");
>>> - }
>>> + // open passive server socket and get parameters
>>> + try {
>>> + DataConnectionConfiguration dataCfg = session.getListener()
>>> + .getDataConnectionConfiguration();
>>>
>>> - // open passive server socket and get parameters
>>> - try {
>>> - DataConnectionConfiguration dataCfg = session.getListener()
>>> - .getDataConnectionConfiguration();
>>> + String passiveAddress = dataCfg.getPassiveAddress();
>>>
>>> - String passiveAddress = dataCfg.getPassiveAddress();
>>> + if (passiveAddress == null) {
>>> + address = serverControlAddress;
>>> + } else {
>>> + address = resolveAddress(dataCfg.getPassiveAddress());
>>> + }
>>>
>>> - if (passiveAddress == null) {
>>> - address = serverControlAddress;
>>> - } else {
>>> - address = resolveAddress(dataCfg.getPassiveAddress());
>>> - }
>>> + if (secure) {
>>> + LOG
>>> + .debug(
>>> + "Opening SSL passive data connection on address \"{}\" and port {}",
>>> + address, passivePort);
>>> + SslConfiguration ssl = getSslConfiguration();
>>> + if (ssl == null) {
>>> + throw new DataConnectionException(
>>> + "Data connection SSL required but not configured.");
>>> + }
>>>
>>> - if (secure) {
>>> - LOG
>>> - .debug(
>>> - "Opening SSL passive data connection
>>> on address \"{}\" and port {}",
>>> - address, passivePort);
>>> - SslConfiguration ssl = getSslConfiguration();
>>> - if (ssl == null) {
>>> - throw new DataConnectionException(
>>> - "Data connection SSL required but not
>>> configured.");
>>> - }
>>> + // this method does not actually create the SSL socket, due to a
>>> + // JVM bug
>>> + // (https://issues.apache.org/jira/browse/FTPSERVER-241).
>>> + // Instead, it creates a regular
>>> + // ServerSocket that will be wrapped as a SSL socket in
>>> + // createDataSocket()
>>> + servSoc = new ServerSocket(passivePort, 0, address);
>>> + LOG
>>> + .debug(
>>> + "SSL Passive data connection created on address \"{}\" and port {}",
>>> + address, passivePort);
>>> + } else {
>>> + LOG
>>> + .debug(
>>> + "Opening passive data connection on address \"{}\" and port {}",
>>> + address, passivePort);
>>> + servSoc = new ServerSocket(passivePort, 0, address);
>>> + LOG
>>> + .debug(
>>> + "Passive data connection created on address \"{}\" and port {}",
>>> + address, passivePort);
>>> + }
>>> + port = servSoc.getLocalPort();
>>> + servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>>>
>>> - // this method does not actually create the SSL
>>> socket, due to a JVM bug
>>> - // (https://issues.apache.org/jira/browse/FTPSERVER-241).
>>> - // Instead, it creates a regular
>>> - // ServerSocket that will be wrapped as a SSL socket
>>> in createDataSocket()
>>> - servSoc = new ServerSocket(passivePort, 0, address);
>>> - LOG
>>> - .debug(
>>> - "SSL Passive data connection created
>>> on address \"{}\" and port {}",
>>> - address, passivePort);
>>> - } else {
>>> - LOG
>>> - .debug(
>>> - "Opening passive data connection on
>>> address \"{}\" and port {}",
>>> - address, passivePort);
>>> - servSoc = new ServerSocket(passivePort, 0, address);
>>> - LOG
>>> - .debug(
>>> - "Passive data connection created on
>>> address \"{}\" and port {}",
>>> - address, passivePort);
>>> - }
>>> - port = servSoc.getLocalPort();
>>> - servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>>> + // set different state variables
>>> + passive = true;
>>> + requestTime = System.currentTimeMillis();
>>>
>>> - // set different state variables
>>> - passive = true;
>>> - requestTime = System.currentTimeMillis();
>>> + return new InetSocketAddress(address, port);
>>> + } catch (Exception ex) {
>>> + servSoc = null;
>>> + closeDataConnection();
>>> + throw new DataConnectionException(
>>> + "Failed to initate passive data connection: "
>>> + + ex.getMessage(), ex);
>>> + }
>>> + }
>>>
>>> - return new InetSocketAddress(address, port);
>>> - } catch (Exception ex) {
>>> - servSoc = null;
>>> - closeDataConnection();
>>> - throw new DataConnectionException(
>>> - "Failed to initate passive data connection: "
>>> - + ex.getMessage(), ex);
>>> - }
>>> - }
>>> + /*
>>> + * (non-Javadoc)
>>> + *
>>> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
>>> + */
>>> + public InetAddress getInetAddress() {
>>> + return address;
>>> + }
>>>
>>> - /*
>>> - * (non-Javadoc)
>>> - *
>>> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
>>> - */
>>> - public InetAddress getInetAddress() {
>>> - return address;
>>> - }
>>> + /*
>>> + * (non-Javadoc)
>>> + *
>>> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
>>> + */
>>> + public int getPort() {
>>> + return port;
>>> + }
>>>
>>> - /*
>>> - * (non-Javadoc)
>>> - *
>>> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
>>> - */
>>> - public int getPort() {
>>> - return port;
>>> - }
>>> + /*
>>> + * (non-Javadoc)
>>> + *
>>> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
>>> + */
>>> + public DataConnection openConnection() throws Exception {
>>> + return new IODataConnection(createDataSocket(), session, this);
>>> + }
>>>
>>> - /*
>>> - * (non-Javadoc)
>>> - *
>>> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
>>> - */
>>> - public DataConnection openConnection() throws Exception {
>>> - return new IODataConnection(createDataSocket(), session, this);
>>> - }
>>> + /**
>>> + * Get the data socket. In case of error returns null.
>>> + */
>>> + private synchronized Socket createDataSocket() throws Exception {
>>>
>>> - /**
>>> - * Get the data socket. In case of error returns null.
>>> - */
>>> - private synchronized Socket createDataSocket() throws Exception {
>>> + // get socket depending on the selection
>>> + dataSoc = null;
>>> + DataConnectionConfiguration dataConfig = session.getListener()
>>> + .getDataConnectionConfiguration();
>>> + try {
>>> + if (!passive) {
>>> + if (secure) {
>>> + LOG.debug("Opening secure active data connection");
>>> + SslConfiguration ssl = getSslConfiguration();
>>> + if (ssl == null) {
>>> + throw new FtpException(
>>> + "Data connection SSL not configured");
>>> + }
>>>
>>> - // get socket depending on the selection
>>> - dataSoc = null;
>>> - DataConnectionConfiguration dataConfig = session.getListener()
>>> - .getDataConnectionConfiguration();
>>> - try {
>>> - if (!passive) {
>>> - if (secure) {
>>> - LOG.debug("Opening secure active data connection");
>>> - SslConfiguration ssl = getSslConfiguration();
>>> - if (ssl == null) {
>>> - throw new FtpException(
>>> - "Data connection SSL not configured");
>>> - }
>>> + // get socket factory
>>> + SSLSocketFactory socFactory = ssl.getSocketFactory();
>>>
>>> - // get socket factory
>>> - SSLSocketFactory socFactory = ssl.getSocketFactory();
>>> + // create socket
>>> + SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
>>> + ssoc.setUseClientMode(false);
>>>
>>> - // create socket
>>> - SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
>>> - ssoc.setUseClientMode(false);
>>> + // initialize socket
>>> + if (ssl.getEnabledCipherSuites() != null) {
>>> + ssoc.setEnabledCipherSuites(ssl
>>> + .getEnabledCipherSuites());
>>> + }
>>> + dataSoc = ssoc;
>>> + } else {
>>> + LOG.debug("Opening active data connection");
>>> + dataSoc = new Socket();
>>> + }
>>>
>>> - // initialize socket
>>> - if (ssl.getEnabledCipherSuites() != null) {
>>> -
>>> ssoc.setEnabledCipherSuites(ssl.getEnabledCipherSuites());
>>> - }
>>> - dataSoc = ssoc;
>>> - } else {
>>> - LOG.debug("Opening active data connection");
>>> - dataSoc = new Socket();
>>> - }
>>> + dataSoc.setReuseAddress(true);
>>>
>>> - dataSoc.setReuseAddress(true);
>>> + InetAddress localAddr = resolveAddress(dataConfig
>>> + .getActiveLocalAddress());
>>>
>>> - InetAddress localAddr = resolveAddress(dataConfig
>>> - .getActiveLocalAddress());
>>> + // if no local address has been configured, make sure we use the
>>> + // same as the client connects from
>>> + if (localAddr == null) {
>>> + localAddr = ((InetSocketAddress) session.getLocalAddress())
>>> + .getAddress();
>>> + }
>>>
>>> - // if no local address has been configured, make sure
>>> we use the same as the client connects from
>>> - if(localAddr == null) {
>>> - localAddr =
>>> ((InetSocketAddress)session.getLocalAddress()).getAddress();
>>> - }
>>> + SocketAddress localSocketAddress = new InetSocketAddress(
>>> + localAddr, dataConfig.getActiveLocalPort());
>>>
>>> - SocketAddress localSocketAddress = new
>>> InetSocketAddress(localAddr, dataConfig.getActiveLocalPort());
>>> -
>>> - LOG.debug("Binding active data connection to {}",
>>> localSocketAddress);
>>> - dataSoc.bind(localSocketAddress);
>>> + LOG.debug("Binding active data connection to {}",
>>> + localSocketAddress);
>>> + dataSoc.bind(localSocketAddress);
>>>
>>> - dataSoc.connect(new InetSocketAddress(address, port));
>>> - } else {
>>> + dataSoc.connect(new InetSocketAddress(address, port));
>>> + } else {
>>>
>>> - if (secure) {
>>> - LOG.debug("Opening secure passive data connection");
>>> - // this is where we wrap the unsecured socket as
>>> a SSLSocket. This is
>>> - // due to the JVM bug described in FTPSERVER-241.
>>> + if (secure) {
>>> + LOG.debug("Opening secure passive data connection");
>>> + // this is where we wrap the unsecured socket as a
>>> + // SSLSocket. This is
>>> + // due to the JVM bug described in FTPSERVER-241.
>>>
>>> - // get server socket factory
>>> - SslConfiguration ssl = getSslConfiguration();
>>> -
>>> - // we've already checked this, but let's do it again
>>> - if (ssl == null) {
>>> - throw new FtpException(
>>> - "Data connection SSL not configured");
>>> - }
>>> + // get server socket factory
>>> + SslConfiguration ssl = getSslConfiguration();
>>>
>>> - SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
>>> + // we've already checked this, but let's do it again
>>> + if (ssl == null) {
>>> + throw new FtpException(
>>> + "Data connection SSL not configured");
>>> + }
>>>
>>> - Socket serverSocket = servSoc.accept();
>>> + SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
>>>
>>> - SSLSocket sslSocket = (SSLSocket) ssocketFactory
>>> - .createSocket(serverSocket, serverSocket
>>> - .getInetAddress().getHostAddress(),
>>> - serverSocket.getPort(), true);
>>> - sslSocket.setUseClientMode(false);
>>> + Socket serverSocket = servSoc.accept();
>>>
>>> - // initialize server socket
>>> - if (ssl.getClientAuth() == ClientAuth.NEED) {
>>> - sslSocket.setNeedClientAuth(true);
>>> - } else if (ssl.getClientAuth() == ClientAuth.WANT) {
>>> - sslSocket.setWantClientAuth(true);
>>> - }
>>> + SSLSocket sslSocket = (SSLSocket) ssocketFactory
>>> + .createSocket(serverSocket, serverSocket
>>> + .getInetAddress().getHostAddress(),
>>> + serverSocket.getPort(), true);
>>> + sslSocket.setUseClientMode(false);
>>>
>>> - if (ssl.getEnabledCipherSuites() != null) {
>>> - sslSocket.setEnabledCipherSuites(ssl
>>> - .getEnabledCipherSuites());
>>> - }
>>> + // initialize server socket
>>> + if (ssl.getClientAuth() == ClientAuth.NEED) {
>>> + sslSocket.setNeedClientAuth(true);
>>> + } else if (ssl.getClientAuth() == ClientAuth.WANT) {
>>> + sslSocket.setWantClientAuth(true);
>>> + }
>>>
>>> - dataSoc = sslSocket;
>>> - } else {
>>> - LOG.debug("Opening passive data connection");
>>> + if (ssl.getEnabledCipherSuites() != null) {
>>> + sslSocket.setEnabledCipherSuites(ssl
>>> + .getEnabledCipherSuites());
>>> + }
>>>
>>> - dataSoc = servSoc.accept();
>>> - }
>>> -
>>> - if (dataConfig.isPassiveIpCheck()) {
>>> + dataSoc = sslSocket;
>>> + } else {
>>> + LOG.debug("Opening passive data connection");
>>> +
>>> + dataSoc = servSoc.accept();
>>> + }
>>> +
>>> + if (dataConfig.isPassiveIpCheck()) {
>>> // Let's make sure we got the connection from the same
>>> // client that we are expecting
>>> - InetAddress remoteAddress = ((InetSocketAddress)
>>> session.getRemoteAddress()).getAddress();
>>> + InetAddress remoteAddress = ((InetSocketAddress) session
>>> + .getRemoteAddress()).getAddress();
>>> InetAddress dataSocketAddress = dataSoc.getInetAddress();
>>> if (!dataSocketAddress.equals(remoteAddress)) {
>>> - LOG.warn("Passive IP Check failed. Closing data connection from "
>>> - + dataSocketAddress
>>> - + " as it does not match the expected address "
>>> - + remoteAddress);
>>> + LOG
>>> + .warn("Passive IP Check failed. Closing data connection from "
>>> + + dataSocketAddress
>>> + + " as it does not match the expected address "
>>> + + remoteAddress);
>>> closeDataConnection();
>>> return null;
>>> }
>>> }
>>> -
>>> - DataConnectionConfiguration dataCfg = session.getListener()
>>> - .getDataConnectionConfiguration();
>>> -
>>> - dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>>> - LOG.debug("Passive data connection opened");
>>> - }
>>> - } catch (Exception ex) {
>>> - closeDataConnection();
>>> - LOG.warn("FtpDataConnection.getDataSocket()", ex);
>>> - throw ex;
>>> - }
>>> - dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
>>>
>>> - // Make sure we initiate the SSL handshake, or we'll
>>> - // get an error if we turn out not to send any data
>>> - // e.g. during the listing of an empty directory
>>> - if (dataSoc instanceof SSLSocket) {
>>> - ((SSLSocket) dataSoc).startHandshake();
>>> - }
>>> + DataConnectionConfiguration dataCfg = session.getListener()
>>> + .getDataConnectionConfiguration();
>>>
>>> - return dataSoc;
>>> - }
>>> + dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>>> + LOG.debug("Passive data connection opened");
>>> + }
>>> + } catch (Exception ex) {
>>> + closeDataConnection();
>>> + LOG.warn("FtpDataConnection.getDataSocket()", ex);
>>> + throw ex;
>>> + }
>>> + dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
>>>
>>> - /*
>>> - * (non-Javadoc)
>>> - * Returns an InetAddress object from a hostname or IP address.
>>> - */
>>> - private InetAddress resolveAddress(String host)
>>> - throws DataConnectionException {
>>> - if (host == null) {
>>> - return null;
>>> - } else {
>>> - try {
>>> - return InetAddress.getByName(host);
>>> - } catch (UnknownHostException ex) {
>>> - throw new DataConnectionException("Failed to resolve
>>> address", ex);
>>> - }
>>> - }
>>> - }
>>> + // Make sure we initiate the SSL handshake, or we'll
>>> + // get an error if we turn out not to send any data
>>> + // e.g. during the listing of an empty directory
>>> + if (dataSoc instanceof SSLSocket) {
>>> + ((SSLSocket) dataSoc).startHandshake();
>>> + }
>>>
>>> - /*
>>> - * (non-Javadoc)
>>> - *
>>> - * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
>>> - */
>>> - public boolean isSecure() {
>>> - return secure;
>>> - }
>>> + return dataSoc;
>>> + }
>>>
>>> - /**
>>> - * Set the security protocol.
>>> - */
>>> - public void setSecure(final boolean secure) {
>>> - this.secure = secure;
>>> - }
>>> + /*
>>> + * (non-Javadoc) Returns an InetAddress object from a hostname or IP
>>> + * address.
>>> + */
>>> + private InetAddress resolveAddress(String host)
>>> + throws DataConnectionException {
>>> + if (host == null) {
>>> + return null;
>>> + } else {
>>> + try {
>>> + return InetAddress.getByName(host);
>>> + } catch (UnknownHostException ex) {
>>> + throw new DataConnectionException("Failed to resolve address",
>>> + ex);
>>> + }
>>> + }
>>> + }
>>>
>>> - /*
>>> - * (non-Javadoc)
>>> - *
>>> - * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
>>> - */
>>> - public boolean isZipMode() {
>>> - return isZip;
>>> - }
>>> + /*
>>> + * (non-Javadoc)
>>> + *
>>> + * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
>>> + */
>>> + public boolean isSecure() {
>>> + return secure;
>>> + }
>>>
>>> - /**
>>> - * Set zip mode.
>>> - */
>>> - public void setZipMode(final boolean zip) {
>>> - isZip = zip;
>>> - }
>>> + /**
>>> + * Set the security protocol.
>>> + */
>>> + public void setSecure(final boolean secure) {
>>> + this.secure = secure;
>>> + }
>>>
>>> - /**
>>> - * Check the data connection idle status.
>>> - */
>>> - public synchronized boolean isTimeout(final long currTime) {
>>> + /*
>>> + * (non-Javadoc)
>>> + *
>>> + * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
>>> + */
>>> + public boolean isZipMode() {
>>> + return isZip;
>>> + }
>>>
>>> - // data connection not requested - not a timeout
>>> - if (requestTime == 0L) {
>>> - return false;
>>> - }
>>> + /**
>>> + * Set zip mode.
>>> + */
>>> + public void setZipMode(final boolean zip) {
>>> + isZip = zip;
>>> + }
>>>
>>> - // data connection active - not a timeout
>>> - if (dataSoc != null) {
>>> - return false;
>>> - }
>>> + /**
>>> + * Check the data connection idle status.
>>> + */
>>> + public synchronized boolean isTimeout(final long currTime) {
>>>
>>> - // no idle time limit - not a timeout
>>> - int maxIdleTime = session.getListener()
>>> - .getDataConnectionConfiguration().getIdleTime() * 1000;
>>> - if (maxIdleTime == 0) {
>>> - return false;
>>> - }
>>> + // data connection not requested - not a timeout
>>> + if (requestTime == 0L) {
>>> + return false;
>>> + }
>>>
>>> - // idle time is within limit - not a timeout
>>> - if ((currTime - requestTime) < maxIdleTime) {
>>> - return false;
>>> - }
>>> + // data connection active - not a timeout
>>> + if (dataSoc != null) {
>>> + return false;
>>> + }
>>>
>>> - return true;
>>> - }
>>> + // no idle time limit - not a timeout
>>> + int maxIdleTime = session.getListener()
>>> + .getDataConnectionConfiguration().getIdleTime() * 1000;
>>> + if (maxIdleTime == 0) {
>>> + return false;
>>> + }
>>>
>>> - /**
>>> - * Dispose data connection - close all the sockets.
>>> - */
>>> - public void dispose() {
>>> - closeDataConnection();
>>> - }
>>> + // idle time is within limit - not a timeout
>>> + if ((currTime - requestTime) < maxIdleTime) {
>>> + return false;
>>> + }
>>>
>>> - /**
>>> - * Sets the server's control address.
>>> - */
>>> - public void setServerControlAddress(final InetAddress
>>> serverControlAddress) {
>>> - this.serverControlAddress = serverControlAddress;
>>> - }
>>> + return true;
>>> + }
>>> +
>>> + /**
>>> + * Dispose data connection - close all the sockets.
>>> + */
>>> + public void dispose() {
>>> + closeDataConnection();
>>> + }
>>> +
>>> + /**
>>> + * Sets the server's control address.
>>> + */
>>> + public void setServerControlAddress(final InetAddress serverControlAddress) {
>>> + this.serverControlAddress = serverControlAddress;
>>> + }
>>> }
>>>
>>>
>>>
>>>
>>>
>>> 2010/3/25 Sai Pullabhotla <sa...@jmethods.com>:
>>>> Just so we are all on the same page -
>>>>
>>>> Do you think the issue is with the LIST command, or the code that
>>>> creates passive data connection? Could you briefly explain some of the
>>>> issues that made you think it should be rewritten, so everyone gets a
>>>> chance to evaluate?
>>>>
>>>> Thanks.
>>>>
>>>> Regards,
>>>> Sai Pullabhotla
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Thu, Mar 25, 2010 at 5:56 AM, David Latorre <dv...@gmail.com> wrote:
>>>>> 2010/3/24 Niklas Gustavsson <ni...@protocol7.com>:
>>>>>> On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fr...@gmail.com> wrote:
>>>>>>> we found an issue related to requestPassivePort() which may lead to an
>>>>>>> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced.
>>>>>>>
>>>>>>> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full description
>>>>>>> of the symptoms and a minimalist java client and server to reproduce it.
>>>>>>
>>>>>> I haven't yet looked closer at the code you attached. But, I have seen
>>>>>> similar behavior myself during performance testing FtpServer. In that
>>>>>> case, I had a very similar behavior at around 20 threads. However, the
>>>>>> reason for the problem in that test was that the test case uses up
>>>>>> file handles (for the sockets) so fast that they will run out. Since
>>>>>> sockets hang around for some time also after closing, they were not
>>>>>> freed quickly enough and thus FtpServer could not open new ones. Could
>>>>>> you please verify that this is not the case here? You could look at
>>>>>> netstat output and look into increasing the allowed number of file
>>>>>> handles our the timeout time for sockets in your OS.
>>>>>
>>>>>
>>>>> Actually it is quite easy to reproduce this error (I just wrote a
>>>>> client test case with throws >20~30 threads that list a directory in
>>>>> the server ) and it's not file handle related:
>>>>> we have several bugs in our code that cause this behaviour , i
>>>>> think we hould rewrite all the request/release passive port mechanism
>>>>> as there are several issues with it.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> /niklas
>>>>>>
>>>>>
>>>>
>>>
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
The wait appears to be the culprit. I've opened a new case for the
quick fix (to error out immediately) and attached a patch. Could you
or Fred test this patch with the test case and let us know the result?
The JIRA issue/patch is available at
https://issues.apache.org/jira/browse/FTPSERVER-360.
Regards,
Sai Pullabhotla
On Fri, Mar 26, 2010 at 7:11 AM, David Latorre <dv...@gmail.com> wrote:
> 2010/3/26 Niklas Gustavsson <ni...@protocol7.com>:
>> On Fri, Mar 26, 2010 at 9:50 AM, Fred Moore <fr...@gmail.com> wrote:
>>> 1\ Priority of passive port sharing ehnancement: Niklas survey shows that we
>>> are indeed in good company here, but it's problably worth having a better
>>> look at this anyway, there might be good technical reasons that led all the
>>> other teams not to support this or it may turn up that it's "simply" because
>>> it's somewhat hard to develop and test.
>>
>> After this discussion I'm significantly less thrilled at implementing
>> shared passive ports :-)
>
> Shared passive ports would be a nice feature if they aren't too hard
> to implement. Among the opensource servers, I think coloradoFTP -a
> NIO-based java FTPServer under the LGPL license- offered this (since
> their data connections also use async sockets this shouldn't be too
> hard for them, but I don't know if they solved the use case depicted
> by Sai: when there are several sessions open from the same IP) but it
> seems that commercial solutions offer this and more...
>
>
>
>>> 2\ Quick fix for 1.0.x codebase: pushing a 40x to the client when no
>>> passive port is available (or probably better: no passive port is available
>>> within X seconds) it's probably something we need to do anyway.
>>
>> Thinking some more about this, I'm personally now convinced that
>> should simple return an error (not waiting). I'm not sure what the
>> best reply code should be, but "425 Can't open data connection" seems
>> fitting although not specified as valid return from the PASV command.
>>
>>> 3\ Suspect race condition: the problem description for the originally
>>> reported http://issues.apache.org/jira/browse/FTPSERVER-359 (see also repro
>>> code attached to the jira) actually hints also to something different as
>>> well, in fact we state that a few (say 20) parallel threads issuing LISTs in
>>> passive mode are able to "lock-up" the server forever. Questions:
>>>
>>> 3.1\ Is this interely explained by this thread discussion? (I don't think
>>> so: the server should *always* be able to recover)
>>
>> Agreed, the server should always recover from a situation like this.
>> After looking into how to fix item 2, we need to rerun your tests and
>> make sure we always survive.
>
> Thinking about this issue my understanding of the problem is as follows:
>
> 1. We have a number of connections to FTPServer > the Executor
> threadpool max size (I think it is 16) sending the PASV command.
>
> 2. The first one of them requests the only available port and gets it.
> Now the port is in use by a server socket and any subsequent call to
> requestPassivePort will end up invoking wait().
>
> 3. The thread that processed this PASV command is now available and a
> new PASV request is assigned to it.
>
> 4. Now all threads are trying to request a passive port, but since
> there are no ports available all the threads in the OrderedThreadPool
> get blocked by the wait() method.
>
> I wonder if we are suffering a similar problem in any other cases; if
> it was so, we might need to delay the opening of the ServerSocket
> until the LIST (or GET or PUT...) commands are executed.
>
> I hope I made myself clear and that my understanding was right.
>
>
>>> 3.2\ Would this be fixed by a quick fix as per 2\? (likely, but it's sort of
>>> like using nukes to for mowing the lawn)
>>
>> I really have no idea, but I think we should fix 2 first and then make
>> sure we handle your test case.
>>
>>> In short my current position can be stated as follows: I think that
>>> FTPSERVER-359 has a different root cause from what we discussed, the problem
>>> impact is not completely known at the moment but it appears to *severely*
>>> affect the server availabily... having just one port is an easy way of
>>> reproducing it (but not the cause of it).
>>
>> Agreed.
>>
>> /niklas
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by David Latorre <dv...@gmail.com>.
2010/3/26 Sai Pullabhotla <sa...@jmethods.com>:
> David,
>
> I just re-read your comments towards the end of your previous email:
>
> "I wonder if we are suffering a similar problem in any other cases; if
> it was so, we might need to delay the opening of the ServerSocket
> until the LIST (or GET or PUT...) commands are executed"
>
> Do you think creating/binding a new ServerSocket could potentially
> take a long time? Is that your concern?
Not really, my concern here was that we could have some concurrency
issue, but this shouldn't be a problem anymore with the wait() calls
removed.
> Regards,
> Sai Pullabhotla
>
>
>
>
>
> On Fri, Mar 26, 2010 at 7:11 AM, David Latorre <dv...@gmail.com> wrote:
>> 2010/3/26 Niklas Gustavsson <ni...@protocol7.com>:
>>> On Fri, Mar 26, 2010 at 9:50 AM, Fred Moore <fr...@gmail.com> wrote:
>>>> 1\ Priority of passive port sharing ehnancement: Niklas survey shows that we
>>>> are indeed in good company here, but it's problably worth having a better
>>>> look at this anyway, there might be good technical reasons that led all the
>>>> other teams not to support this or it may turn up that it's "simply" because
>>>> it's somewhat hard to develop and test.
>>>
>>> After this discussion I'm significantly less thrilled at implementing
>>> shared passive ports :-)
>>
>> Shared passive ports would be a nice feature if they aren't too hard
>> to implement. Among the opensource servers, I think coloradoFTP -a
>> NIO-based java FTPServer under the LGPL license- offered this (since
>> their data connections also use async sockets this shouldn't be too
>> hard for them, but I don't know if they solved the use case depicted
>> by Sai: when there are several sessions open from the same IP) but it
>> seems that commercial solutions offer this and more...
>>
>>
>>
>>>> 2\ Quick fix for 1.0.x codebase: pushing a 40x to the client when no
>>>> passive port is available (or probably better: no passive port is available
>>>> within X seconds) it's probably something we need to do anyway.
>>>
>>> Thinking some more about this, I'm personally now convinced that
>>> should simple return an error (not waiting). I'm not sure what the
>>> best reply code should be, but "425 Can't open data connection" seems
>>> fitting although not specified as valid return from the PASV command.
>>>
>>>> 3\ Suspect race condition: the problem description for the originally
>>>> reported http://issues.apache.org/jira/browse/FTPSERVER-359 (see also repro
>>>> code attached to the jira) actually hints also to something different as
>>>> well, in fact we state that a few (say 20) parallel threads issuing LISTs in
>>>> passive mode are able to "lock-up" the server forever. Questions:
>>>>
>>>> 3.1\ Is this interely explained by this thread discussion? (I don't think
>>>> so: the server should *always* be able to recover)
>>>
>>> Agreed, the server should always recover from a situation like this.
>>> After looking into how to fix item 2, we need to rerun your tests and
>>> make sure we always survive.
>>
>> Thinking about this issue my understanding of the problem is as follows:
>>
>> 1. We have a number of connections to FTPServer > the Executor
>> threadpool max size (I think it is 16) sending the PASV command.
>>
>> 2. The first one of them requests the only available port and gets it.
>> Now the port is in use by a server socket and any subsequent call to
>> requestPassivePort will end up invoking wait().
>>
>> 3. The thread that processed this PASV command is now available and a
>> new PASV request is assigned to it.
>>
>> 4. Now all threads are trying to request a passive port, but since
>> there are no ports available all the threads in the OrderedThreadPool
>> get blocked by the wait() method.
>>
>> I wonder if we are suffering a similar problem in any other cases; if
>> it was so, we might need to delay the opening of the ServerSocket
>> until the LIST (or GET or PUT...) commands are executed.
>>
>> I hope I made myself clear and that my understanding was right.
>>
>>
>>>> 3.2\ Would this be fixed by a quick fix as per 2\? (likely, but it's sort of
>>>> like using nukes to for mowing the lawn)
>>>
>>> I really have no idea, but I think we should fix 2 first and then make
>>> sure we handle your test case.
>>>
>>>> In short my current position can be stated as follows: I think that
>>>> FTPSERVER-359 has a different root cause from what we discussed, the problem
>>>> impact is not completely known at the moment but it appears to *severely*
>>>> affect the server availabily... having just one port is an easy way of
>>>> reproducing it (but not the cause of it).
>>>
>>> Agreed.
>>>
>>> /niklas
>>>
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
David,
I just re-read your comments towards the end of your previous email:
"I wonder if we are suffering a similar problem in any other cases; if
it was so, we might need to delay the opening of the ServerSocket
until the LIST (or GET or PUT...) commands are executed"
Do you think creating/binding a new ServerSocket could potentially
take a long time? Is that your concern?
Regards,
Sai Pullabhotla
On Fri, Mar 26, 2010 at 7:11 AM, David Latorre <dv...@gmail.com> wrote:
> 2010/3/26 Niklas Gustavsson <ni...@protocol7.com>:
>> On Fri, Mar 26, 2010 at 9:50 AM, Fred Moore <fr...@gmail.com> wrote:
>>> 1\ Priority of passive port sharing ehnancement: Niklas survey shows that we
>>> are indeed in good company here, but it's problably worth having a better
>>> look at this anyway, there might be good technical reasons that led all the
>>> other teams not to support this or it may turn up that it's "simply" because
>>> it's somewhat hard to develop and test.
>>
>> After this discussion I'm significantly less thrilled at implementing
>> shared passive ports :-)
>
> Shared passive ports would be a nice feature if they aren't too hard
> to implement. Among the opensource servers, I think coloradoFTP -a
> NIO-based java FTPServer under the LGPL license- offered this (since
> their data connections also use async sockets this shouldn't be too
> hard for them, but I don't know if they solved the use case depicted
> by Sai: when there are several sessions open from the same IP) but it
> seems that commercial solutions offer this and more...
>
>
>
>>> 2\ Quick fix for 1.0.x codebase: pushing a 40x to the client when no
>>> passive port is available (or probably better: no passive port is available
>>> within X seconds) it's probably something we need to do anyway.
>>
>> Thinking some more about this, I'm personally now convinced that
>> should simple return an error (not waiting). I'm not sure what the
>> best reply code should be, but "425 Can't open data connection" seems
>> fitting although not specified as valid return from the PASV command.
>>
>>> 3\ Suspect race condition: the problem description for the originally
>>> reported http://issues.apache.org/jira/browse/FTPSERVER-359 (see also repro
>>> code attached to the jira) actually hints also to something different as
>>> well, in fact we state that a few (say 20) parallel threads issuing LISTs in
>>> passive mode are able to "lock-up" the server forever. Questions:
>>>
>>> 3.1\ Is this interely explained by this thread discussion? (I don't think
>>> so: the server should *always* be able to recover)
>>
>> Agreed, the server should always recover from a situation like this.
>> After looking into how to fix item 2, we need to rerun your tests and
>> make sure we always survive.
>
> Thinking about this issue my understanding of the problem is as follows:
>
> 1. We have a number of connections to FTPServer > the Executor
> threadpool max size (I think it is 16) sending the PASV command.
>
> 2. The first one of them requests the only available port and gets it.
> Now the port is in use by a server socket and any subsequent call to
> requestPassivePort will end up invoking wait().
>
> 3. The thread that processed this PASV command is now available and a
> new PASV request is assigned to it.
>
> 4. Now all threads are trying to request a passive port, but since
> there are no ports available all the threads in the OrderedThreadPool
> get blocked by the wait() method.
>
> I wonder if we are suffering a similar problem in any other cases; if
> it was so, we might need to delay the opening of the ServerSocket
> until the LIST (or GET or PUT...) commands are executed.
>
> I hope I made myself clear and that my understanding was right.
>
>
>>> 3.2\ Would this be fixed by a quick fix as per 2\? (likely, but it's sort of
>>> like using nukes to for mowing the lawn)
>>
>> I really have no idea, but I think we should fix 2 first and then make
>> sure we handle your test case.
>>
>>> In short my current position can be stated as follows: I think that
>>> FTPSERVER-359 has a different root cause from what we discussed, the problem
>>> impact is not completely known at the moment but it appears to *severely*
>>> affect the server availabily... having just one port is an easy way of
>>> reproducing it (but not the cause of it).
>>
>> Agreed.
>>
>> /niklas
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by David Latorre <dv...@gmail.com>.
2010/3/26 Niklas Gustavsson <ni...@protocol7.com>:
> On Fri, Mar 26, 2010 at 9:50 AM, Fred Moore <fr...@gmail.com> wrote:
>> 1\ Priority of passive port sharing ehnancement: Niklas survey shows that we
>> are indeed in good company here, but it's problably worth having a better
>> look at this anyway, there might be good technical reasons that led all the
>> other teams not to support this or it may turn up that it's "simply" because
>> it's somewhat hard to develop and test.
>
> After this discussion I'm significantly less thrilled at implementing
> shared passive ports :-)
Shared passive ports would be a nice feature if they aren't too hard
to implement. Among the opensource servers, I think coloradoFTP -a
NIO-based java FTPServer under the LGPL license- offered this (since
their data connections also use async sockets this shouldn't be too
hard for them, but I don't know if they solved the use case depicted
by Sai: when there are several sessions open from the same IP) but it
seems that commercial solutions offer this and more...
>> 2\ Quick fix for 1.0.x codebase: pushing a 40x to the client when no
>> passive port is available (or probably better: no passive port is available
>> within X seconds) it's probably something we need to do anyway.
>
> Thinking some more about this, I'm personally now convinced that
> should simple return an error (not waiting). I'm not sure what the
> best reply code should be, but "425 Can't open data connection" seems
> fitting although not specified as valid return from the PASV command.
>
>> 3\ Suspect race condition: the problem description for the originally
>> reported http://issues.apache.org/jira/browse/FTPSERVER-359 (see also repro
>> code attached to the jira) actually hints also to something different as
>> well, in fact we state that a few (say 20) parallel threads issuing LISTs in
>> passive mode are able to "lock-up" the server forever. Questions:
>>
>> 3.1\ Is this interely explained by this thread discussion? (I don't think
>> so: the server should *always* be able to recover)
>
> Agreed, the server should always recover from a situation like this.
> After looking into how to fix item 2, we need to rerun your tests and
> make sure we always survive.
Thinking about this issue my understanding of the problem is as follows:
1. We have a number of connections to FTPServer > the Executor
threadpool max size (I think it is 16) sending the PASV command.
2. The first one of them requests the only available port and gets it.
Now the port is in use by a server socket and any subsequent call to
requestPassivePort will end up invoking wait().
3. The thread that processed this PASV command is now available and a
new PASV request is assigned to it.
4. Now all threads are trying to request a passive port, but since
there are no ports available all the threads in the OrderedThreadPool
get blocked by the wait() method.
I wonder if we are suffering a similar problem in any other cases; if
it was so, we might need to delay the opening of the ServerSocket
until the LIST (or GET or PUT...) commands are executed.
I hope I made myself clear and that my understanding was right.
>> 3.2\ Would this be fixed by a quick fix as per 2\? (likely, but it's sort of
>> like using nukes to for mowing the lawn)
>
> I really have no idea, but I think we should fix 2 first and then make
> sure we handle your test case.
>
>> In short my current position can be stated as follows: I think that
>> FTPSERVER-359 has a different root cause from what we discussed, the problem
>> impact is not completely known at the moment but it appears to *severely*
>> affect the server availabily... having just one port is an easy way of
>> reproducing it (but not the cause of it).
>
> Agreed.
>
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Fri, Mar 26, 2010 at 9:50 AM, Fred Moore <fr...@gmail.com> wrote:
> 1\ Priority of passive port sharing ehnancement: Niklas survey shows that we
> are indeed in good company here, but it's problably worth having a better
> look at this anyway, there might be good technical reasons that led all the
> other teams not to support this or it may turn up that it's "simply" because
> it's somewhat hard to develop and test.
After this discussion I'm significantly less thrilled at implementing
shared passive ports :-)
> 2\ Quick fix for 1.0.x codebase: pushing a 40x to the client when no
> passive port is available (or probably better: no passive port is available
> within X seconds) it's probably something we need to do anyway.
Thinking some more about this, I'm personally now convinced that
should simple return an error (not waiting). I'm not sure what the
best reply code should be, but "425 Can't open data connection" seems
fitting although not specified as valid return from the PASV command.
> 3\ Suspect race condition: the problem description for the originally
> reported http://issues.apache.org/jira/browse/FTPSERVER-359 (see also repro
> code attached to the jira) actually hints also to something different as
> well, in fact we state that a few (say 20) parallel threads issuing LISTs in
> passive mode are able to "lock-up" the server forever. Questions:
>
> 3.1\ Is this interely explained by this thread discussion? (I don't think
> so: the server should *always* be able to recover)
Agreed, the server should always recover from a situation like this.
After looking into how to fix item 2, we need to rerun your tests and
make sure we always survive.
> 3.2\ Would this be fixed by a quick fix as per 2\? (likely, but it's sort of
> like using nukes to for mowing the lawn)
I really have no idea, but I think we should fix 2 first and then make
sure we handle your test case.
> In short my current position can be stated as follows: I think that
> FTPSERVER-359 has a different root cause from what we discussed, the problem
> impact is not completely known at the moment but it appears to *severely*
> affect the server availabily... having just one port is an easy way of
> reproducing it (but not the cause of it).
Agreed.
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Fred Moore <fr...@gmail.com>.
Hi folks,
given that I started this interesting thread :-) I'll add a few
considerations:
1\ Priority of passive port sharing ehnancement: Niklas survey shows that we
are indeed in good company here, but it's problably worth having a better
look at this anyway, there might be good technical reasons that led all the
other teams not to support this or it may turn up that it's "simply" because
it's somewhat hard to develop and test.
2\ Quick fix for 1.0.x codebase: pushing a 40x to the client when no
passive port is available (or probably better: no passive port is available
within X seconds) it's probably something we need to do anyway.
3\ Suspect race condition: the problem description for the originally
reported http://issues.apache.org/jira/browse/FTPSERVER-359 (see also repro
code attached to the jira) actually hints also to something different as
well, in fact we state that a few (say 20) parallel threads issuing LISTs in
passive mode are able to "lock-up" the server forever. Questions:
3.1\ Is this interely explained by this thread discussion? (I don't think
so: the server should *always* be able to recover)
3.2\ Would this be fixed by a quick fix as per 2\? (likely, but it's sort of
like using nukes to for mowing the lawn)
In short my current position can be stated as follows: I think that
FTPSERVER-359 has a different root cause from what we discussed, the problem
impact is not completely known at the moment but it appears to *severely*
affect the server availabily... having just one port is an easy way of
reproducing it (but not the cause of it).
What do you guys think?
Cheers,
F.
On Thu, Mar 25, 2010 at 8:09 PM, Sai Pullabhotla <
sai.pullabhotla@jmethods.com> wrote:
> Hmmm, if that's the case, it should be a low priority task for sure.
>
> Regards,
> Sai Pullabhotla
>
>
>
>
>
> On Thu, Mar 25, 2010 at 2:06 PM, Niklas Gustavsson <ni...@protocol7.com>
> wrote:
> > On Thu, Mar 25, 2010 at 7:42 PM, Sai Pullabhotla
> > <sa...@jmethods.com> wrote:
> >> I think so. Overall, the idea is pretty cool, but too scary. You
> >> mentioned that most FTP servers support this feature, but I could find
> >> any servers highlighting this feature.
> >
> > I should not have said "most" since I have not investigated this
> > myself. The report mentions "some commercial FTP-servers" whatever
> > that means.
> >
> > A quick survey:
> > IIS: one client per port, recommends using a large enoug range (Sai)
> > FileZilla: one client per port, recommends using a large enough range
> > proftpd: one client per port, recommends using a large enough range
> > WS_FTP: seems to only support one client per port
> >
> > So, as mentioned in the issue, open source servers does seem to
> > support this. Makes me think it might not be such high priority for
> > us.
> >
> > /niklas
> >
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
Hmmm, if that's the case, it should be a low priority task for sure.
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 2:06 PM, Niklas Gustavsson <ni...@protocol7.com> wrote:
> On Thu, Mar 25, 2010 at 7:42 PM, Sai Pullabhotla
> <sa...@jmethods.com> wrote:
>> I think so. Overall, the idea is pretty cool, but too scary. You
>> mentioned that most FTP servers support this feature, but I could find
>> any servers highlighting this feature.
>
> I should not have said "most" since I have not investigated this
> myself. The report mentions "some commercial FTP-servers" whatever
> that means.
>
> A quick survey:
> IIS: one client per port, recommends using a large enoug range (Sai)
> FileZilla: one client per port, recommends using a large enough range
> proftpd: one client per port, recommends using a large enough range
> WS_FTP: seems to only support one client per port
>
> So, as mentioned in the issue, open source servers does seem to
> support this. Makes me think it might not be such high priority for
> us.
>
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Thu, Mar 25, 2010 at 7:42 PM, Sai Pullabhotla
<sa...@jmethods.com> wrote:
> I think so. Overall, the idea is pretty cool, but too scary. You
> mentioned that most FTP servers support this feature, but I could find
> any servers highlighting this feature.
I should not have said "most" since I have not investigated this
myself. The report mentions "some commercial FTP-servers" whatever
that means.
A quick survey:
IIS: one client per port, recommends using a large enoug range (Sai)
FileZilla: one client per port, recommends using a large enough range
proftpd: one client per port, recommends using a large enough range
WS_FTP: seems to only support one client per port
So, as mentioned in the issue, open source servers does seem to
support this. Makes me think it might not be such high priority for
us.
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
I think so. Overall, the idea is pretty cool, but too scary. You
mentioned that most FTP servers support this feature, but I could find
any servers highlighting this feature. The Miscrosoft IIS
documentation specifically says to pick a port range that matches with
the number of concurrent data transfers that you expect. Can you
provide me some links to the FTP servers that support this, so I can
play with those and see how they function under various circumstances?
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 9:37 AM, Niklas Gustavsson <ni...@protocol7.com> wrote:
> On Thu, Mar 25, 2010 at 3:26 PM, Sai Pullabhotla
> <sa...@jmethods.com> wrote:
>> (1) would always first try to get an unused passive port. If it does,
>> everything is good and works the same old way.
>> (2) If (1) fails, it would try to get a port that is not shared by the
>> same source/client address. If it finds such a port, it would still
>> work the old way.
>> (3) If (1) and (2) fail to get a port number, a 4xx error is sent to
>> the client (may be after a timeout?)
>
> Ignoring the exact details of the current patch... how about:
> 1. Find the first passive port for which the same IP is not currently
> waiting to connect (doesn't matter if the client has already
> connected, right)
> 2. If none is found, fail with a 4xx number
>
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Thu, Mar 25, 2010 at 3:26 PM, Sai Pullabhotla
<sa...@jmethods.com> wrote:
> (1) would always first try to get an unused passive port. If it does,
> everything is good and works the same old way.
> (2) If (1) fails, it would try to get a port that is not shared by the
> same source/client address. If it finds such a port, it would still
> work the old way.
> (3) If (1) and (2) fail to get a port number, a 4xx error is sent to
> the client (may be after a timeout?)
Ignoring the exact details of the current patch... how about:
1. Find the first passive port for which the same IP is not currently
waiting to connect (doesn't matter if the client has already
connected, right)
2. If none is found, fail with a 4xx number
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
Yeah, it does makes sense. Just to make sure my understanding is correct -
The patch we have on file -
(1) would always first try to get an unused passive port. If it does,
everything is good and works the same old way.
(2) If (1) fails, it would try to get a port that is not shared by the
same source/client address. If it finds such a port, it would still
work the old way.
(3) If (1) and (2) fail to get a port number, a 4xx error is sent to
the client (may be after a timeout?)
Correct me if I'm still incorrect :).
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 9:13 AM, Niklas Gustavsson <ni...@protocol7.com> wrote:
> On Thu, Mar 25, 2010 at 3:04 PM, Sai Pullabhotla
> <sa...@jmethods.com> wrote:
>> Not sure what you meant by -
>>
>> "In that case, we would basically get what we would have in 1.0.x."
>>
>> 1.0.x never sends the same port number to two different clients, isn't it?
>
> Neither should 1.1.x if those clients are from the same remote IP.
> Thus, if you got two clients that are behind the same proxy and
> FtpServer thus see the same IP, the behavior for those two clients
> would be the same as we're talking about for 1.0.x. That is, the
> second client would get an error since no free port is available (for
> that IP).
>
> So, as far as I can see, nothing (besides the complexity of our code
> perhaps) gets worse in 1.1.x. In most case it's an improvement because
> multiple clients will be able to connect on the same port. In some
> cases (multiple clients from the same IP), it will be as in 1.0.x.
>
> Does that make any sense?
>
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Thu, Mar 25, 2010 at 3:04 PM, Sai Pullabhotla
<sa...@jmethods.com> wrote:
> Not sure what you meant by -
>
> "In that case, we would basically get what we would have in 1.0.x."
>
> 1.0.x never sends the same port number to two different clients, isn't it?
Neither should 1.1.x if those clients are from the same remote IP.
Thus, if you got two clients that are behind the same proxy and
FtpServer thus see the same IP, the behavior for those two clients
would be the same as we're talking about for 1.0.x. That is, the
second client would get an error since no free port is available (for
that IP).
So, as far as I can see, nothing (besides the complexity of our code
perhaps) gets worse in 1.1.x. In most case it's an improvement because
multiple clients will be able to connect on the same port. In some
cases (multiple clients from the same IP), it will be as in 1.0.x.
Does that make any sense?
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
Not sure what you meant by -
"In that case, we would basically get what we would have in 1.0.x."
1.0.x never sends the same port number to two different clients, isn't it?
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 8:46 AM, Niklas Gustavsson <ni...@protocol7.com> wrote:
> On Thu, Mar 25, 2010 at 2:42 PM, Sai Pullabhotla
> <sa...@jmethods.com> wrote:
>> If we reject simultaneous data connections from a given source IP,
>> What would be the implications when connections are in fact from two
>> different clients, but they all go through the same router (in a
>> typical work/home network)? The FTP server would see the public IP of
>> the router, isn't it?
>
> Yes. Same thing if FtpServer is behind a proxy. In that case, we would
> basically get what we would have in 1.0.x.
>
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Thu, Mar 25, 2010 at 2:42 PM, Sai Pullabhotla
<sa...@jmethods.com> wrote:
> If we reject simultaneous data connections from a given source IP,
> What would be the implications when connections are in fact from two
> different clients, but they all go through the same router (in a
> typical work/home network)? The FTP server would see the public IP of
> the router, isn't it?
Yes. Same thing if FtpServer is behind a proxy. In that case, we would
basically get what we would have in 1.0.x.
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
If we reject simultaneous data connections from a given source IP,
What would be the implications when connections are in fact from two
different clients, but they all go through the same router (in a
typical work/home network)? The FTP server would see the public IP of
the router, isn't it?
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 8:37 AM, Niklas Gustavsson <ni...@protocol7.com> wrote:
> On Thu, Mar 25, 2010 at 2:30 PM, Sai Pullabhotla
> <sa...@jmethods.com> wrote:
>> I've not looked at the patch that supports concurrent data connections
>> on a single passive port, but I've some serious doubts as to if it is
>> even legitimate to have such support and if we can gracefully handle
>> such scenario.
>
> I think most FTP servers support concurrent use of the same port.
>
>> Here is an example scenario -
>>
>> 1. Client A has more than one session (for this example let us say
>> two) open with the FTP server.
>> 2. Session 1 issues PASV command.
>> 3. Server replies back asking to connect on port 2000.
>> 4. About the same time, Session 2 issues PASV command
>> 5. Server replies back asking to connect on port 2000.
>> 6. Both session 1 and session 2 connect to port 2000 almost at the same time.
>> 7. How do we distinguish which data connection belongs to which
>> control session?
>>
>> Would we possibly be sending/receiving incorrect data on session 1/2?
>
> Step 5 must not be allowed. That is, we should not have two waiting
> passive ports from the same IP. In this case (if only port 2000 is
> used for passive ports), step 5 should be returning a 4XX reply.
>
> Would that work?
>
> All considered, adding support for this will require quite some work
> when it comes to testing.
>
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Thu, Mar 25, 2010 at 2:30 PM, Sai Pullabhotla
<sa...@jmethods.com> wrote:
> I've not looked at the patch that supports concurrent data connections
> on a single passive port, but I've some serious doubts as to if it is
> even legitimate to have such support and if we can gracefully handle
> such scenario.
I think most FTP servers support concurrent use of the same port.
> Here is an example scenario -
>
> 1. Client A has more than one session (for this example let us say
> two) open with the FTP server.
> 2. Session 1 issues PASV command.
> 3. Server replies back asking to connect on port 2000.
> 4. About the same time, Session 2 issues PASV command
> 5. Server replies back asking to connect on port 2000.
> 6. Both session 1 and session 2 connect to port 2000 almost at the same time.
> 7. How do we distinguish which data connection belongs to which
> control session?
>
> Would we possibly be sending/receiving incorrect data on session 1/2?
Step 5 must not be allowed. That is, we should not have two waiting
passive ports from the same IP. In this case (if only port 2000 is
used for passive ports), step 5 should be returning a 4XX reply.
Would that work?
All considered, adding support for this will require quite some work
when it comes to testing.
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
I've not looked at the patch that supports concurrent data connections
on a single passive port, but I've some serious doubts as to if it is
even legitimate to have such support and if we can gracefully handle
such scenario. Here is an example scenario -
1. Client A has more than one session (for this example let us say
two) open with the FTP server.
2. Session 1 issues PASV command.
3. Server replies back asking to connect on port 2000.
4. About the same time, Session 2 issues PASV command
5. Server replies back asking to connect on port 2000.
6. Both session 1 and session 2 connect to port 2000 almost at the same time.
7. How do we distinguish which data connection belongs to which
control session?
Would we possibly be sending/receiving incorrect data on session 1/2?
I apologize if I'm missing something here, but please correct me if I'm wrong.
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 8:14 AM, Niklas Gustavsson <ni...@protocol7.com> wrote:
> On Thu, Mar 25, 2010 at 2:09 PM, Sai Pullabhotla
> <sa...@jmethods.com> wrote:
>> Or even better, since the current version is not
>> supposed to be used with too little passive ports than the number of
>> concurrent clients, should we error out right way with 4XX message
>> indicating a data connection could not be opened?
>
> I think this makes a lot of sense for the 1.0.x branch. For 1.1.x, the
> plan is to merge in the existing patch to enable concurrent data
> connections on the same passive port.
>
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
On Thu, Mar 25, 2010 at 2:09 PM, Sai Pullabhotla
<sa...@jmethods.com> wrote:
> Or even better, since the current version is not
> supposed to be used with too little passive ports than the number of
> concurrent clients, should we error out right way with 4XX message
> indicating a data connection could not be opened?
I think this makes a lot of sense for the 1.0.x branch. For 1.1.x, the
plan is to merge in the existing patch to enable concurrent data
connections on the same passive port.
/niklas
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
I would like to make a comment on the current implementation -
Do you think we want to wait indefinitely when there is no available
passive port, or should we have a timed wait, say 60 seconds. If we
can't get an available passive port in the timeout period, we should
send an error back to the client. This is mostly based on the fact
that most FTP clients would have their data connection timeout set,
which is very small. Or even better, since the current version is not
supposed to be used with too little passive ports than the number of
concurrent clients, should we error out right way with 4XX message
indicating a data connection could not be opened?
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 7:51 AM, Niklas Gustavsson <ni...@protocol7.com> wrote:
> Interesting! Could you send the patch without whitespace/formatting
> changes, would make it much easier to follow.
>
> /niklas
>
> On Thu, Mar 25, 2010 at 1:46 PM, David Latorre <dv...@gmail.com> wrote:
>> hello Sai,
>>
>> I didn't have time to fully review the code but i made these nearly
>> random changes in IODataConnectionFactory and
>> DefaultDataConnectionConfiguration and the server is not getting
>> blocked anymore...
>>
>>
>>
>> Index: main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java
>> ===================================================================
>> --- main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (revision
>> 927354)
>> +++ main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (working
>> copy)
>> @@ -129,7 +129,8 @@
>> * port will be used.
>> */
>> public synchronized int requestPassivePort() {
>> - int dataPort = -1;
>> +
>> + int dataPort = -1;
>> int loopTimes = 2;
>> Thread currThread = Thread.currentThread();
>>
>> @@ -164,8 +165,7 @@
>> */
>> public synchronized void releasePassivePort(final int port) {
>> passivePorts.releasePort(port);
>> -
>> - notify();
>> + notifyAll();
>> }
>>
>> /**
>> Index: main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java
>> ===================================================================
>> --- main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (revision
>> 927354)
>> +++ main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (working
>> copy)
>> @@ -44,446 +44,459 @@
>> *
>> * We can get the FTP data connection using this class. It uses either PORT or
>> * PASV command.
>> - *
>> + *
>> * @author <a href="http://mina.apache.org">Apache MINA Project</a>
>> */
>> public class IODataConnectionFactory implements ServerDataConnectionFactory {
>>
>> - private final Logger LOG = LoggerFactory
>> - .getLogger(IODataConnectionFactory.class);
>> + private final Logger LOG = LoggerFactory
>> + .getLogger(IODataConnectionFactory.class);
>>
>> - private FtpServerContext serverContext;
>> + private FtpServerContext serverContext;
>>
>> - private Socket dataSoc;
>> + private Socket dataSoc;
>>
>> - ServerSocket servSoc;
>> + ServerSocket servSoc;
>>
>> - InetAddress address;
>> + InetAddress address;
>>
>> - int port = 0;
>> + int port = 0;
>>
>> - long requestTime = 0L;
>> + long requestTime = 0L;
>>
>> - boolean passive = false;
>> + boolean passive = false;
>>
>> - boolean secure = false;
>> + boolean secure = false;
>>
>> - private boolean isZip = false;
>> + private boolean isZip = false;
>>
>> - InetAddress serverControlAddress;
>> + InetAddress serverControlAddress;
>>
>> - FtpIoSession session;
>> + FtpIoSession session;
>>
>> - public IODataConnectionFactory(final FtpServerContext serverContext,
>> - final FtpIoSession session) {
>> - this.session = session;
>> - this.serverContext = serverContext;
>> - if (session.getListener().getDataConnectionConfiguration()
>> - .isImplicitSsl()) {
>> - secure = true;
>> - }
>> - }
>> + public IODataConnectionFactory(final FtpServerContext serverContext,
>> + final FtpIoSession session) {
>> + this.session = session;
>> + this.serverContext = serverContext;
>> + if (session.getListener().getDataConnectionConfiguration()
>> + .isImplicitSsl()) {
>> + secure = true;
>> + }
>> + }
>>
>> - /**
>> - * Close data socket.
>> - * This method must be idempotent as we might call it multiple
>> times during disconnect.
>> - */
>> - public synchronized void closeDataConnection() {
>> + /**
>> + * Close data socket. This method must be idempotent as we might call it
>> + * multiple times during disconnect.
>> + */
>> + public synchronized void closeDataConnection() {
>>
>> - // close client socket if any
>> - if (dataSoc != null) {
>> - try {
>> - dataSoc.close();
>> - } catch (Exception ex) {
>> - LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>> - }
>> - dataSoc = null;
>> - }
>> + // close client socket if any
>> + if (dataSoc != null) {
>> + try {
>> + dataSoc.close();
>> + } catch (Exception ex) {
>> + LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>> + }
>> + dataSoc = null;
>> + }
>>
>> - // close server socket if any
>> - if (servSoc != null) {
>> - try {
>> - servSoc.close();
>> - } catch (Exception ex) {
>> - LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>> - }
>> + // close server socket if any
>> + if (servSoc != null) {
>> + try {
>> + servSoc.close();
>> + } catch (Exception ex) {
>> + LOG.warn("FtpDataConnection.closeDataSocket()", ex);
>> + }
>>
>> - if (session != null) {
>> - DataConnectionConfiguration dcc = session.getListener()
>> - .getDataConnectionConfiguration();
>> - if (dcc != null) {
>> - dcc.releasePassivePort(port);
>> - }
>> - }
>> + servSoc = null;
>> + }
>>
>> - servSoc = null;
>> - }
>> + // release passive ports
>> + if (session != null) {
>> + DataConnectionConfiguration dcc = session.getListener()
>> + .getDataConnectionConfiguration();
>> + if (dcc != null) {
>> + dcc.releasePassivePort(port);
>> + }
>> + }
>> +
>> + // reset request time
>> + requestTime = 0L;
>> + }
>>
>> - // reset request time
>> - requestTime = 0L;
>> - }
>> + /**
>> + * Port command.
>> + */
>> + public synchronized void initActiveDataConnection(
>> + final InetSocketAddress address) {
>>
>> - /**
>> - * Port command.
>> - */
>> - public synchronized void initActiveDataConnection(
>> - final InetSocketAddress address) {
>> + // close old sockets if any
>> + closeDataConnection();
>>
>> - // close old sockets if any
>> - closeDataConnection();
>> + // set variables
>> + passive = false;
>> + this.address = address.getAddress();
>> + port = address.getPort();
>> + requestTime = System.currentTimeMillis();
>> + }
>>
>> - // set variables
>> - passive = false;
>> - this.address = address.getAddress();
>> - port = address.getPort();
>> - requestTime = System.currentTimeMillis();
>> - }
>> + private SslConfiguration getSslConfiguration() {
>> + DataConnectionConfiguration dataCfg = session.getListener()
>> + .getDataConnectionConfiguration();
>>
>> - private SslConfiguration getSslConfiguration() {
>> - DataConnectionConfiguration dataCfg = session.getListener()
>> - .getDataConnectionConfiguration();
>> + SslConfiguration configuration = dataCfg.getSslConfiguration();
>>
>> - SslConfiguration configuration = dataCfg.getSslConfiguration();
>> + // fall back if no configuration has been provided on the data
>> + // connection config
>> + if (configuration == null) {
>> + configuration = session.getListener().getSslConfiguration();
>> + }
>>
>> - // fall back if no configuration has been provided on the
>> data connection config
>> - if (configuration == null) {
>> - configuration = session.getListener().getSslConfiguration();
>> - }
>> + return configuration;
>> + }
>>
>> - return configuration;
>> - }
>> + /**
>> + * Initiate a data connection in passive mode (server listening).
>> + */
>> + public synchronized InetSocketAddress initPassiveDataConnection()
>> + throws DataConnectionException {
>> + LOG.debug("Initiating passive data connection");
>> + // close old sockets if any
>> + closeDataConnection();
>>
>> - /**
>> - * Initiate a data connection in passive mode (server listening).
>> - */
>> - public synchronized InetSocketAddress initPassiveDataConnection()
>> - throws DataConnectionException {
>> - LOG.debug("Initiating passive data connection");
>> - // close old sockets if any
>> - closeDataConnection();
>> + // get the passive port
>> + int passivePort = session.getListener()
>> + .getDataConnectionConfiguration().requestPassivePort();
>> + if (passivePort == -1) {
>> + servSoc = null;
>> + throw new DataConnectionException(
>> + "Cannot find an available passive port.");
>> + }
>>
>> - // get the passive port
>> - int passivePort = session.getListener()
>> - .getDataConnectionConfiguration().requestPassivePort();
>> - if (passivePort == -1) {
>> - servSoc = null;
>> - throw new DataConnectionException(
>> - "Cannot find an available passive port.");
>> - }
>> + // open passive server socket and get parameters
>> + try {
>> + DataConnectionConfiguration dataCfg = session.getListener()
>> + .getDataConnectionConfiguration();
>>
>> - // open passive server socket and get parameters
>> - try {
>> - DataConnectionConfiguration dataCfg = session.getListener()
>> - .getDataConnectionConfiguration();
>> + String passiveAddress = dataCfg.getPassiveAddress();
>>
>> - String passiveAddress = dataCfg.getPassiveAddress();
>> + if (passiveAddress == null) {
>> + address = serverControlAddress;
>> + } else {
>> + address = resolveAddress(dataCfg.getPassiveAddress());
>> + }
>>
>> - if (passiveAddress == null) {
>> - address = serverControlAddress;
>> - } else {
>> - address = resolveAddress(dataCfg.getPassiveAddress());
>> - }
>> + if (secure) {
>> + LOG
>> + .debug(
>> + "Opening SSL passive data connection on address \"{}\" and port {}",
>> + address, passivePort);
>> + SslConfiguration ssl = getSslConfiguration();
>> + if (ssl == null) {
>> + throw new DataConnectionException(
>> + "Data connection SSL required but not configured.");
>> + }
>>
>> - if (secure) {
>> - LOG
>> - .debug(
>> - "Opening SSL passive data connection
>> on address \"{}\" and port {}",
>> - address, passivePort);
>> - SslConfiguration ssl = getSslConfiguration();
>> - if (ssl == null) {
>> - throw new DataConnectionException(
>> - "Data connection SSL required but not
>> configured.");
>> - }
>> + // this method does not actually create the SSL socket, due to a
>> + // JVM bug
>> + // (https://issues.apache.org/jira/browse/FTPSERVER-241).
>> + // Instead, it creates a regular
>> + // ServerSocket that will be wrapped as a SSL socket in
>> + // createDataSocket()
>> + servSoc = new ServerSocket(passivePort, 0, address);
>> + LOG
>> + .debug(
>> + "SSL Passive data connection created on address \"{}\" and port {}",
>> + address, passivePort);
>> + } else {
>> + LOG
>> + .debug(
>> + "Opening passive data connection on address \"{}\" and port {}",
>> + address, passivePort);
>> + servSoc = new ServerSocket(passivePort, 0, address);
>> + LOG
>> + .debug(
>> + "Passive data connection created on address \"{}\" and port {}",
>> + address, passivePort);
>> + }
>> + port = servSoc.getLocalPort();
>> + servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>>
>> - // this method does not actually create the SSL
>> socket, due to a JVM bug
>> - // (https://issues.apache.org/jira/browse/FTPSERVER-241).
>> - // Instead, it creates a regular
>> - // ServerSocket that will be wrapped as a SSL socket
>> in createDataSocket()
>> - servSoc = new ServerSocket(passivePort, 0, address);
>> - LOG
>> - .debug(
>> - "SSL Passive data connection created
>> on address \"{}\" and port {}",
>> - address, passivePort);
>> - } else {
>> - LOG
>> - .debug(
>> - "Opening passive data connection on
>> address \"{}\" and port {}",
>> - address, passivePort);
>> - servSoc = new ServerSocket(passivePort, 0, address);
>> - LOG
>> - .debug(
>> - "Passive data connection created on
>> address \"{}\" and port {}",
>> - address, passivePort);
>> - }
>> - port = servSoc.getLocalPort();
>> - servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>> + // set different state variables
>> + passive = true;
>> + requestTime = System.currentTimeMillis();
>>
>> - // set different state variables
>> - passive = true;
>> - requestTime = System.currentTimeMillis();
>> + return new InetSocketAddress(address, port);
>> + } catch (Exception ex) {
>> + servSoc = null;
>> + closeDataConnection();
>> + throw new DataConnectionException(
>> + "Failed to initate passive data connection: "
>> + + ex.getMessage(), ex);
>> + }
>> + }
>>
>> - return new InetSocketAddress(address, port);
>> - } catch (Exception ex) {
>> - servSoc = null;
>> - closeDataConnection();
>> - throw new DataConnectionException(
>> - "Failed to initate passive data connection: "
>> - + ex.getMessage(), ex);
>> - }
>> - }
>> + /*
>> + * (non-Javadoc)
>> + *
>> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
>> + */
>> + public InetAddress getInetAddress() {
>> + return address;
>> + }
>>
>> - /*
>> - * (non-Javadoc)
>> - *
>> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
>> - */
>> - public InetAddress getInetAddress() {
>> - return address;
>> - }
>> + /*
>> + * (non-Javadoc)
>> + *
>> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
>> + */
>> + public int getPort() {
>> + return port;
>> + }
>>
>> - /*
>> - * (non-Javadoc)
>> - *
>> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
>> - */
>> - public int getPort() {
>> - return port;
>> - }
>> + /*
>> + * (non-Javadoc)
>> + *
>> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
>> + */
>> + public DataConnection openConnection() throws Exception {
>> + return new IODataConnection(createDataSocket(), session, this);
>> + }
>>
>> - /*
>> - * (non-Javadoc)
>> - *
>> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
>> - */
>> - public DataConnection openConnection() throws Exception {
>> - return new IODataConnection(createDataSocket(), session, this);
>> - }
>> + /**
>> + * Get the data socket. In case of error returns null.
>> + */
>> + private synchronized Socket createDataSocket() throws Exception {
>>
>> - /**
>> - * Get the data socket. In case of error returns null.
>> - */
>> - private synchronized Socket createDataSocket() throws Exception {
>> + // get socket depending on the selection
>> + dataSoc = null;
>> + DataConnectionConfiguration dataConfig = session.getListener()
>> + .getDataConnectionConfiguration();
>> + try {
>> + if (!passive) {
>> + if (secure) {
>> + LOG.debug("Opening secure active data connection");
>> + SslConfiguration ssl = getSslConfiguration();
>> + if (ssl == null) {
>> + throw new FtpException(
>> + "Data connection SSL not configured");
>> + }
>>
>> - // get socket depending on the selection
>> - dataSoc = null;
>> - DataConnectionConfiguration dataConfig = session.getListener()
>> - .getDataConnectionConfiguration();
>> - try {
>> - if (!passive) {
>> - if (secure) {
>> - LOG.debug("Opening secure active data connection");
>> - SslConfiguration ssl = getSslConfiguration();
>> - if (ssl == null) {
>> - throw new FtpException(
>> - "Data connection SSL not configured");
>> - }
>> + // get socket factory
>> + SSLSocketFactory socFactory = ssl.getSocketFactory();
>>
>> - // get socket factory
>> - SSLSocketFactory socFactory = ssl.getSocketFactory();
>> + // create socket
>> + SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
>> + ssoc.setUseClientMode(false);
>>
>> - // create socket
>> - SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
>> - ssoc.setUseClientMode(false);
>> + // initialize socket
>> + if (ssl.getEnabledCipherSuites() != null) {
>> + ssoc.setEnabledCipherSuites(ssl
>> + .getEnabledCipherSuites());
>> + }
>> + dataSoc = ssoc;
>> + } else {
>> + LOG.debug("Opening active data connection");
>> + dataSoc = new Socket();
>> + }
>>
>> - // initialize socket
>> - if (ssl.getEnabledCipherSuites() != null) {
>> -
>> ssoc.setEnabledCipherSuites(ssl.getEnabledCipherSuites());
>> - }
>> - dataSoc = ssoc;
>> - } else {
>> - LOG.debug("Opening active data connection");
>> - dataSoc = new Socket();
>> - }
>> + dataSoc.setReuseAddress(true);
>>
>> - dataSoc.setReuseAddress(true);
>> + InetAddress localAddr = resolveAddress(dataConfig
>> + .getActiveLocalAddress());
>>
>> - InetAddress localAddr = resolveAddress(dataConfig
>> - .getActiveLocalAddress());
>> + // if no local address has been configured, make sure we use the
>> + // same as the client connects from
>> + if (localAddr == null) {
>> + localAddr = ((InetSocketAddress) session.getLocalAddress())
>> + .getAddress();
>> + }
>>
>> - // if no local address has been configured, make sure
>> we use the same as the client connects from
>> - if(localAddr == null) {
>> - localAddr =
>> ((InetSocketAddress)session.getLocalAddress()).getAddress();
>> - }
>> + SocketAddress localSocketAddress = new InetSocketAddress(
>> + localAddr, dataConfig.getActiveLocalPort());
>>
>> - SocketAddress localSocketAddress = new
>> InetSocketAddress(localAddr, dataConfig.getActiveLocalPort());
>> -
>> - LOG.debug("Binding active data connection to {}",
>> localSocketAddress);
>> - dataSoc.bind(localSocketAddress);
>> + LOG.debug("Binding active data connection to {}",
>> + localSocketAddress);
>> + dataSoc.bind(localSocketAddress);
>>
>> - dataSoc.connect(new InetSocketAddress(address, port));
>> - } else {
>> + dataSoc.connect(new InetSocketAddress(address, port));
>> + } else {
>>
>> - if (secure) {
>> - LOG.debug("Opening secure passive data connection");
>> - // this is where we wrap the unsecured socket as
>> a SSLSocket. This is
>> - // due to the JVM bug described in FTPSERVER-241.
>> + if (secure) {
>> + LOG.debug("Opening secure passive data connection");
>> + // this is where we wrap the unsecured socket as a
>> + // SSLSocket. This is
>> + // due to the JVM bug described in FTPSERVER-241.
>>
>> - // get server socket factory
>> - SslConfiguration ssl = getSslConfiguration();
>> -
>> - // we've already checked this, but let's do it again
>> - if (ssl == null) {
>> - throw new FtpException(
>> - "Data connection SSL not configured");
>> - }
>> + // get server socket factory
>> + SslConfiguration ssl = getSslConfiguration();
>>
>> - SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
>> + // we've already checked this, but let's do it again
>> + if (ssl == null) {
>> + throw new FtpException(
>> + "Data connection SSL not configured");
>> + }
>>
>> - Socket serverSocket = servSoc.accept();
>> + SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
>>
>> - SSLSocket sslSocket = (SSLSocket) ssocketFactory
>> - .createSocket(serverSocket, serverSocket
>> - .getInetAddress().getHostAddress(),
>> - serverSocket.getPort(), true);
>> - sslSocket.setUseClientMode(false);
>> + Socket serverSocket = servSoc.accept();
>>
>> - // initialize server socket
>> - if (ssl.getClientAuth() == ClientAuth.NEED) {
>> - sslSocket.setNeedClientAuth(true);
>> - } else if (ssl.getClientAuth() == ClientAuth.WANT) {
>> - sslSocket.setWantClientAuth(true);
>> - }
>> + SSLSocket sslSocket = (SSLSocket) ssocketFactory
>> + .createSocket(serverSocket, serverSocket
>> + .getInetAddress().getHostAddress(),
>> + serverSocket.getPort(), true);
>> + sslSocket.setUseClientMode(false);
>>
>> - if (ssl.getEnabledCipherSuites() != null) {
>> - sslSocket.setEnabledCipherSuites(ssl
>> - .getEnabledCipherSuites());
>> - }
>> + // initialize server socket
>> + if (ssl.getClientAuth() == ClientAuth.NEED) {
>> + sslSocket.setNeedClientAuth(true);
>> + } else if (ssl.getClientAuth() == ClientAuth.WANT) {
>> + sslSocket.setWantClientAuth(true);
>> + }
>>
>> - dataSoc = sslSocket;
>> - } else {
>> - LOG.debug("Opening passive data connection");
>> + if (ssl.getEnabledCipherSuites() != null) {
>> + sslSocket.setEnabledCipherSuites(ssl
>> + .getEnabledCipherSuites());
>> + }
>>
>> - dataSoc = servSoc.accept();
>> - }
>> -
>> - if (dataConfig.isPassiveIpCheck()) {
>> + dataSoc = sslSocket;
>> + } else {
>> + LOG.debug("Opening passive data connection");
>> +
>> + dataSoc = servSoc.accept();
>> + }
>> +
>> + if (dataConfig.isPassiveIpCheck()) {
>> // Let's make sure we got the connection from the same
>> // client that we are expecting
>> - InetAddress remoteAddress = ((InetSocketAddress)
>> session.getRemoteAddress()).getAddress();
>> + InetAddress remoteAddress = ((InetSocketAddress) session
>> + .getRemoteAddress()).getAddress();
>> InetAddress dataSocketAddress = dataSoc.getInetAddress();
>> if (!dataSocketAddress.equals(remoteAddress)) {
>> - LOG.warn("Passive IP Check failed. Closing data connection from "
>> - + dataSocketAddress
>> - + " as it does not match the expected address "
>> - + remoteAddress);
>> + LOG
>> + .warn("Passive IP Check failed. Closing data connection from "
>> + + dataSocketAddress
>> + + " as it does not match the expected address "
>> + + remoteAddress);
>> closeDataConnection();
>> return null;
>> }
>> }
>> -
>> - DataConnectionConfiguration dataCfg = session.getListener()
>> - .getDataConnectionConfiguration();
>> -
>> - dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>> - LOG.debug("Passive data connection opened");
>> - }
>> - } catch (Exception ex) {
>> - closeDataConnection();
>> - LOG.warn("FtpDataConnection.getDataSocket()", ex);
>> - throw ex;
>> - }
>> - dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
>>
>> - // Make sure we initiate the SSL handshake, or we'll
>> - // get an error if we turn out not to send any data
>> - // e.g. during the listing of an empty directory
>> - if (dataSoc instanceof SSLSocket) {
>> - ((SSLSocket) dataSoc).startHandshake();
>> - }
>> + DataConnectionConfiguration dataCfg = session.getListener()
>> + .getDataConnectionConfiguration();
>>
>> - return dataSoc;
>> - }
>> + dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>> + LOG.debug("Passive data connection opened");
>> + }
>> + } catch (Exception ex) {
>> + closeDataConnection();
>> + LOG.warn("FtpDataConnection.getDataSocket()", ex);
>> + throw ex;
>> + }
>> + dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
>>
>> - /*
>> - * (non-Javadoc)
>> - * Returns an InetAddress object from a hostname or IP address.
>> - */
>> - private InetAddress resolveAddress(String host)
>> - throws DataConnectionException {
>> - if (host == null) {
>> - return null;
>> - } else {
>> - try {
>> - return InetAddress.getByName(host);
>> - } catch (UnknownHostException ex) {
>> - throw new DataConnectionException("Failed to resolve
>> address", ex);
>> - }
>> - }
>> - }
>> + // Make sure we initiate the SSL handshake, or we'll
>> + // get an error if we turn out not to send any data
>> + // e.g. during the listing of an empty directory
>> + if (dataSoc instanceof SSLSocket) {
>> + ((SSLSocket) dataSoc).startHandshake();
>> + }
>>
>> - /*
>> - * (non-Javadoc)
>> - *
>> - * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
>> - */
>> - public boolean isSecure() {
>> - return secure;
>> - }
>> + return dataSoc;
>> + }
>>
>> - /**
>> - * Set the security protocol.
>> - */
>> - public void setSecure(final boolean secure) {
>> - this.secure = secure;
>> - }
>> + /*
>> + * (non-Javadoc) Returns an InetAddress object from a hostname or IP
>> + * address.
>> + */
>> + private InetAddress resolveAddress(String host)
>> + throws DataConnectionException {
>> + if (host == null) {
>> + return null;
>> + } else {
>> + try {
>> + return InetAddress.getByName(host);
>> + } catch (UnknownHostException ex) {
>> + throw new DataConnectionException("Failed to resolve address",
>> + ex);
>> + }
>> + }
>> + }
>>
>> - /*
>> - * (non-Javadoc)
>> - *
>> - * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
>> - */
>> - public boolean isZipMode() {
>> - return isZip;
>> - }
>> + /*
>> + * (non-Javadoc)
>> + *
>> + * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
>> + */
>> + public boolean isSecure() {
>> + return secure;
>> + }
>>
>> - /**
>> - * Set zip mode.
>> - */
>> - public void setZipMode(final boolean zip) {
>> - isZip = zip;
>> - }
>> + /**
>> + * Set the security protocol.
>> + */
>> + public void setSecure(final boolean secure) {
>> + this.secure = secure;
>> + }
>>
>> - /**
>> - * Check the data connection idle status.
>> - */
>> - public synchronized boolean isTimeout(final long currTime) {
>> + /*
>> + * (non-Javadoc)
>> + *
>> + * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
>> + */
>> + public boolean isZipMode() {
>> + return isZip;
>> + }
>>
>> - // data connection not requested - not a timeout
>> - if (requestTime == 0L) {
>> - return false;
>> - }
>> + /**
>> + * Set zip mode.
>> + */
>> + public void setZipMode(final boolean zip) {
>> + isZip = zip;
>> + }
>>
>> - // data connection active - not a timeout
>> - if (dataSoc != null) {
>> - return false;
>> - }
>> + /**
>> + * Check the data connection idle status.
>> + */
>> + public synchronized boolean isTimeout(final long currTime) {
>>
>> - // no idle time limit - not a timeout
>> - int maxIdleTime = session.getListener()
>> - .getDataConnectionConfiguration().getIdleTime() * 1000;
>> - if (maxIdleTime == 0) {
>> - return false;
>> - }
>> + // data connection not requested - not a timeout
>> + if (requestTime == 0L) {
>> + return false;
>> + }
>>
>> - // idle time is within limit - not a timeout
>> - if ((currTime - requestTime) < maxIdleTime) {
>> - return false;
>> - }
>> + // data connection active - not a timeout
>> + if (dataSoc != null) {
>> + return false;
>> + }
>>
>> - return true;
>> - }
>> + // no idle time limit - not a timeout
>> + int maxIdleTime = session.getListener()
>> + .getDataConnectionConfiguration().getIdleTime() * 1000;
>> + if (maxIdleTime == 0) {
>> + return false;
>> + }
>>
>> - /**
>> - * Dispose data connection - close all the sockets.
>> - */
>> - public void dispose() {
>> - closeDataConnection();
>> - }
>> + // idle time is within limit - not a timeout
>> + if ((currTime - requestTime) < maxIdleTime) {
>> + return false;
>> + }
>>
>> - /**
>> - * Sets the server's control address.
>> - */
>> - public void setServerControlAddress(final InetAddress
>> serverControlAddress) {
>> - this.serverControlAddress = serverControlAddress;
>> - }
>> + return true;
>> + }
>> +
>> + /**
>> + * Dispose data connection - close all the sockets.
>> + */
>> + public void dispose() {
>> + closeDataConnection();
>> + }
>> +
>> + /**
>> + * Sets the server's control address.
>> + */
>> + public void setServerControlAddress(final InetAddress serverControlAddress) {
>> + this.serverControlAddress = serverControlAddress;
>> + }
>> }
>>
>>
>>
>>
>>
>> 2010/3/25 Sai Pullabhotla <sa...@jmethods.com>:
>>> Just so we are all on the same page -
>>>
>>> Do you think the issue is with the LIST command, or the code that
>>> creates passive data connection? Could you briefly explain some of the
>>> issues that made you think it should be rewritten, so everyone gets a
>>> chance to evaluate?
>>>
>>> Thanks.
>>>
>>> Regards,
>>> Sai Pullabhotla
>>>
>>>
>>>
>>>
>>>
>>> On Thu, Mar 25, 2010 at 5:56 AM, David Latorre <dv...@gmail.com> wrote:
>>>> 2010/3/24 Niklas Gustavsson <ni...@protocol7.com>:
>>>>> On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fr...@gmail.com> wrote:
>>>>>> we found an issue related to requestPassivePort() which may lead to an
>>>>>> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced.
>>>>>>
>>>>>> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full description
>>>>>> of the symptoms and a minimalist java client and server to reproduce it.
>>>>>
>>>>> I haven't yet looked closer at the code you attached. But, I have seen
>>>>> similar behavior myself during performance testing FtpServer. In that
>>>>> case, I had a very similar behavior at around 20 threads. However, the
>>>>> reason for the problem in that test was that the test case uses up
>>>>> file handles (for the sockets) so fast that they will run out. Since
>>>>> sockets hang around for some time also after closing, they were not
>>>>> freed quickly enough and thus FtpServer could not open new ones. Could
>>>>> you please verify that this is not the case here? You could look at
>>>>> netstat output and look into increasing the allowed number of file
>>>>> handles our the timeout time for sockets in your OS.
>>>>
>>>>
>>>> Actually it is quite easy to reproduce this error (I just wrote a
>>>> client test case with throws >20~30 threads that list a directory in
>>>> the server ) and it's not file handle related:
>>>> we have several bugs in our code that cause this behaviour , i
>>>> think we hould rewrite all the request/release passive port mechanism
>>>> as there are several issues with it.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> /niklas
>>>>>
>>>>
>>>
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Niklas Gustavsson <ni...@protocol7.com>.
Interesting! Could you send the patch without whitespace/formatting
changes, would make it much easier to follow.
/niklas
On Thu, Mar 25, 2010 at 1:46 PM, David Latorre <dv...@gmail.com> wrote:
> hello Sai,
>
> I didn't have time to fully review the code but i made these nearly
> random changes in IODataConnectionFactory and
> DefaultDataConnectionConfiguration and the server is not getting
> blocked anymore...
>
>
>
> Index: main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java
> ===================================================================
> --- main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (revision
> 927354)
> +++ main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (working
> copy)
> @@ -129,7 +129,8 @@
> * port will be used.
> */
> public synchronized int requestPassivePort() {
> - int dataPort = -1;
> +
> + int dataPort = -1;
> int loopTimes = 2;
> Thread currThread = Thread.currentThread();
>
> @@ -164,8 +165,7 @@
> */
> public synchronized void releasePassivePort(final int port) {
> passivePorts.releasePort(port);
> -
> - notify();
> + notifyAll();
> }
>
> /**
> Index: main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java
> ===================================================================
> --- main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (revision
> 927354)
> +++ main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (working
> copy)
> @@ -44,446 +44,459 @@
> *
> * We can get the FTP data connection using this class. It uses either PORT or
> * PASV command.
> - *
> + *
> * @author <a href="http://mina.apache.org">Apache MINA Project</a>
> */
> public class IODataConnectionFactory implements ServerDataConnectionFactory {
>
> - private final Logger LOG = LoggerFactory
> - .getLogger(IODataConnectionFactory.class);
> + private final Logger LOG = LoggerFactory
> + .getLogger(IODataConnectionFactory.class);
>
> - private FtpServerContext serverContext;
> + private FtpServerContext serverContext;
>
> - private Socket dataSoc;
> + private Socket dataSoc;
>
> - ServerSocket servSoc;
> + ServerSocket servSoc;
>
> - InetAddress address;
> + InetAddress address;
>
> - int port = 0;
> + int port = 0;
>
> - long requestTime = 0L;
> + long requestTime = 0L;
>
> - boolean passive = false;
> + boolean passive = false;
>
> - boolean secure = false;
> + boolean secure = false;
>
> - private boolean isZip = false;
> + private boolean isZip = false;
>
> - InetAddress serverControlAddress;
> + InetAddress serverControlAddress;
>
> - FtpIoSession session;
> + FtpIoSession session;
>
> - public IODataConnectionFactory(final FtpServerContext serverContext,
> - final FtpIoSession session) {
> - this.session = session;
> - this.serverContext = serverContext;
> - if (session.getListener().getDataConnectionConfiguration()
> - .isImplicitSsl()) {
> - secure = true;
> - }
> - }
> + public IODataConnectionFactory(final FtpServerContext serverContext,
> + final FtpIoSession session) {
> + this.session = session;
> + this.serverContext = serverContext;
> + if (session.getListener().getDataConnectionConfiguration()
> + .isImplicitSsl()) {
> + secure = true;
> + }
> + }
>
> - /**
> - * Close data socket.
> - * This method must be idempotent as we might call it multiple
> times during disconnect.
> - */
> - public synchronized void closeDataConnection() {
> + /**
> + * Close data socket. This method must be idempotent as we might call it
> + * multiple times during disconnect.
> + */
> + public synchronized void closeDataConnection() {
>
> - // close client socket if any
> - if (dataSoc != null) {
> - try {
> - dataSoc.close();
> - } catch (Exception ex) {
> - LOG.warn("FtpDataConnection.closeDataSocket()", ex);
> - }
> - dataSoc = null;
> - }
> + // close client socket if any
> + if (dataSoc != null) {
> + try {
> + dataSoc.close();
> + } catch (Exception ex) {
> + LOG.warn("FtpDataConnection.closeDataSocket()", ex);
> + }
> + dataSoc = null;
> + }
>
> - // close server socket if any
> - if (servSoc != null) {
> - try {
> - servSoc.close();
> - } catch (Exception ex) {
> - LOG.warn("FtpDataConnection.closeDataSocket()", ex);
> - }
> + // close server socket if any
> + if (servSoc != null) {
> + try {
> + servSoc.close();
> + } catch (Exception ex) {
> + LOG.warn("FtpDataConnection.closeDataSocket()", ex);
> + }
>
> - if (session != null) {
> - DataConnectionConfiguration dcc = session.getListener()
> - .getDataConnectionConfiguration();
> - if (dcc != null) {
> - dcc.releasePassivePort(port);
> - }
> - }
> + servSoc = null;
> + }
>
> - servSoc = null;
> - }
> + // release passive ports
> + if (session != null) {
> + DataConnectionConfiguration dcc = session.getListener()
> + .getDataConnectionConfiguration();
> + if (dcc != null) {
> + dcc.releasePassivePort(port);
> + }
> + }
> +
> + // reset request time
> + requestTime = 0L;
> + }
>
> - // reset request time
> - requestTime = 0L;
> - }
> + /**
> + * Port command.
> + */
> + public synchronized void initActiveDataConnection(
> + final InetSocketAddress address) {
>
> - /**
> - * Port command.
> - */
> - public synchronized void initActiveDataConnection(
> - final InetSocketAddress address) {
> + // close old sockets if any
> + closeDataConnection();
>
> - // close old sockets if any
> - closeDataConnection();
> + // set variables
> + passive = false;
> + this.address = address.getAddress();
> + port = address.getPort();
> + requestTime = System.currentTimeMillis();
> + }
>
> - // set variables
> - passive = false;
> - this.address = address.getAddress();
> - port = address.getPort();
> - requestTime = System.currentTimeMillis();
> - }
> + private SslConfiguration getSslConfiguration() {
> + DataConnectionConfiguration dataCfg = session.getListener()
> + .getDataConnectionConfiguration();
>
> - private SslConfiguration getSslConfiguration() {
> - DataConnectionConfiguration dataCfg = session.getListener()
> - .getDataConnectionConfiguration();
> + SslConfiguration configuration = dataCfg.getSslConfiguration();
>
> - SslConfiguration configuration = dataCfg.getSslConfiguration();
> + // fall back if no configuration has been provided on the data
> + // connection config
> + if (configuration == null) {
> + configuration = session.getListener().getSslConfiguration();
> + }
>
> - // fall back if no configuration has been provided on the
> data connection config
> - if (configuration == null) {
> - configuration = session.getListener().getSslConfiguration();
> - }
> + return configuration;
> + }
>
> - return configuration;
> - }
> + /**
> + * Initiate a data connection in passive mode (server listening).
> + */
> + public synchronized InetSocketAddress initPassiveDataConnection()
> + throws DataConnectionException {
> + LOG.debug("Initiating passive data connection");
> + // close old sockets if any
> + closeDataConnection();
>
> - /**
> - * Initiate a data connection in passive mode (server listening).
> - */
> - public synchronized InetSocketAddress initPassiveDataConnection()
> - throws DataConnectionException {
> - LOG.debug("Initiating passive data connection");
> - // close old sockets if any
> - closeDataConnection();
> + // get the passive port
> + int passivePort = session.getListener()
> + .getDataConnectionConfiguration().requestPassivePort();
> + if (passivePort == -1) {
> + servSoc = null;
> + throw new DataConnectionException(
> + "Cannot find an available passive port.");
> + }
>
> - // get the passive port
> - int passivePort = session.getListener()
> - .getDataConnectionConfiguration().requestPassivePort();
> - if (passivePort == -1) {
> - servSoc = null;
> - throw new DataConnectionException(
> - "Cannot find an available passive port.");
> - }
> + // open passive server socket and get parameters
> + try {
> + DataConnectionConfiguration dataCfg = session.getListener()
> + .getDataConnectionConfiguration();
>
> - // open passive server socket and get parameters
> - try {
> - DataConnectionConfiguration dataCfg = session.getListener()
> - .getDataConnectionConfiguration();
> + String passiveAddress = dataCfg.getPassiveAddress();
>
> - String passiveAddress = dataCfg.getPassiveAddress();
> + if (passiveAddress == null) {
> + address = serverControlAddress;
> + } else {
> + address = resolveAddress(dataCfg.getPassiveAddress());
> + }
>
> - if (passiveAddress == null) {
> - address = serverControlAddress;
> - } else {
> - address = resolveAddress(dataCfg.getPassiveAddress());
> - }
> + if (secure) {
> + LOG
> + .debug(
> + "Opening SSL passive data connection on address \"{}\" and port {}",
> + address, passivePort);
> + SslConfiguration ssl = getSslConfiguration();
> + if (ssl == null) {
> + throw new DataConnectionException(
> + "Data connection SSL required but not configured.");
> + }
>
> - if (secure) {
> - LOG
> - .debug(
> - "Opening SSL passive data connection
> on address \"{}\" and port {}",
> - address, passivePort);
> - SslConfiguration ssl = getSslConfiguration();
> - if (ssl == null) {
> - throw new DataConnectionException(
> - "Data connection SSL required but not
> configured.");
> - }
> + // this method does not actually create the SSL socket, due to a
> + // JVM bug
> + // (https://issues.apache.org/jira/browse/FTPSERVER-241).
> + // Instead, it creates a regular
> + // ServerSocket that will be wrapped as a SSL socket in
> + // createDataSocket()
> + servSoc = new ServerSocket(passivePort, 0, address);
> + LOG
> + .debug(
> + "SSL Passive data connection created on address \"{}\" and port {}",
> + address, passivePort);
> + } else {
> + LOG
> + .debug(
> + "Opening passive data connection on address \"{}\" and port {}",
> + address, passivePort);
> + servSoc = new ServerSocket(passivePort, 0, address);
> + LOG
> + .debug(
> + "Passive data connection created on address \"{}\" and port {}",
> + address, passivePort);
> + }
> + port = servSoc.getLocalPort();
> + servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
>
> - // this method does not actually create the SSL
> socket, due to a JVM bug
> - // (https://issues.apache.org/jira/browse/FTPSERVER-241).
> - // Instead, it creates a regular
> - // ServerSocket that will be wrapped as a SSL socket
> in createDataSocket()
> - servSoc = new ServerSocket(passivePort, 0, address);
> - LOG
> - .debug(
> - "SSL Passive data connection created
> on address \"{}\" and port {}",
> - address, passivePort);
> - } else {
> - LOG
> - .debug(
> - "Opening passive data connection on
> address \"{}\" and port {}",
> - address, passivePort);
> - servSoc = new ServerSocket(passivePort, 0, address);
> - LOG
> - .debug(
> - "Passive data connection created on
> address \"{}\" and port {}",
> - address, passivePort);
> - }
> - port = servSoc.getLocalPort();
> - servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
> + // set different state variables
> + passive = true;
> + requestTime = System.currentTimeMillis();
>
> - // set different state variables
> - passive = true;
> - requestTime = System.currentTimeMillis();
> + return new InetSocketAddress(address, port);
> + } catch (Exception ex) {
> + servSoc = null;
> + closeDataConnection();
> + throw new DataConnectionException(
> + "Failed to initate passive data connection: "
> + + ex.getMessage(), ex);
> + }
> + }
>
> - return new InetSocketAddress(address, port);
> - } catch (Exception ex) {
> - servSoc = null;
> - closeDataConnection();
> - throw new DataConnectionException(
> - "Failed to initate passive data connection: "
> - + ex.getMessage(), ex);
> - }
> - }
> + /*
> + * (non-Javadoc)
> + *
> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
> + */
> + public InetAddress getInetAddress() {
> + return address;
> + }
>
> - /*
> - * (non-Javadoc)
> - *
> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
> - */
> - public InetAddress getInetAddress() {
> - return address;
> - }
> + /*
> + * (non-Javadoc)
> + *
> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
> + */
> + public int getPort() {
> + return port;
> + }
>
> - /*
> - * (non-Javadoc)
> - *
> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
> - */
> - public int getPort() {
> - return port;
> - }
> + /*
> + * (non-Javadoc)
> + *
> + * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
> + */
> + public DataConnection openConnection() throws Exception {
> + return new IODataConnection(createDataSocket(), session, this);
> + }
>
> - /*
> - * (non-Javadoc)
> - *
> - * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
> - */
> - public DataConnection openConnection() throws Exception {
> - return new IODataConnection(createDataSocket(), session, this);
> - }
> + /**
> + * Get the data socket. In case of error returns null.
> + */
> + private synchronized Socket createDataSocket() throws Exception {
>
> - /**
> - * Get the data socket. In case of error returns null.
> - */
> - private synchronized Socket createDataSocket() throws Exception {
> + // get socket depending on the selection
> + dataSoc = null;
> + DataConnectionConfiguration dataConfig = session.getListener()
> + .getDataConnectionConfiguration();
> + try {
> + if (!passive) {
> + if (secure) {
> + LOG.debug("Opening secure active data connection");
> + SslConfiguration ssl = getSslConfiguration();
> + if (ssl == null) {
> + throw new FtpException(
> + "Data connection SSL not configured");
> + }
>
> - // get socket depending on the selection
> - dataSoc = null;
> - DataConnectionConfiguration dataConfig = session.getListener()
> - .getDataConnectionConfiguration();
> - try {
> - if (!passive) {
> - if (secure) {
> - LOG.debug("Opening secure active data connection");
> - SslConfiguration ssl = getSslConfiguration();
> - if (ssl == null) {
> - throw new FtpException(
> - "Data connection SSL not configured");
> - }
> + // get socket factory
> + SSLSocketFactory socFactory = ssl.getSocketFactory();
>
> - // get socket factory
> - SSLSocketFactory socFactory = ssl.getSocketFactory();
> + // create socket
> + SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
> + ssoc.setUseClientMode(false);
>
> - // create socket
> - SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
> - ssoc.setUseClientMode(false);
> + // initialize socket
> + if (ssl.getEnabledCipherSuites() != null) {
> + ssoc.setEnabledCipherSuites(ssl
> + .getEnabledCipherSuites());
> + }
> + dataSoc = ssoc;
> + } else {
> + LOG.debug("Opening active data connection");
> + dataSoc = new Socket();
> + }
>
> - // initialize socket
> - if (ssl.getEnabledCipherSuites() != null) {
> -
> ssoc.setEnabledCipherSuites(ssl.getEnabledCipherSuites());
> - }
> - dataSoc = ssoc;
> - } else {
> - LOG.debug("Opening active data connection");
> - dataSoc = new Socket();
> - }
> + dataSoc.setReuseAddress(true);
>
> - dataSoc.setReuseAddress(true);
> + InetAddress localAddr = resolveAddress(dataConfig
> + .getActiveLocalAddress());
>
> - InetAddress localAddr = resolveAddress(dataConfig
> - .getActiveLocalAddress());
> + // if no local address has been configured, make sure we use the
> + // same as the client connects from
> + if (localAddr == null) {
> + localAddr = ((InetSocketAddress) session.getLocalAddress())
> + .getAddress();
> + }
>
> - // if no local address has been configured, make sure
> we use the same as the client connects from
> - if(localAddr == null) {
> - localAddr =
> ((InetSocketAddress)session.getLocalAddress()).getAddress();
> - }
> + SocketAddress localSocketAddress = new InetSocketAddress(
> + localAddr, dataConfig.getActiveLocalPort());
>
> - SocketAddress localSocketAddress = new
> InetSocketAddress(localAddr, dataConfig.getActiveLocalPort());
> -
> - LOG.debug("Binding active data connection to {}",
> localSocketAddress);
> - dataSoc.bind(localSocketAddress);
> + LOG.debug("Binding active data connection to {}",
> + localSocketAddress);
> + dataSoc.bind(localSocketAddress);
>
> - dataSoc.connect(new InetSocketAddress(address, port));
> - } else {
> + dataSoc.connect(new InetSocketAddress(address, port));
> + } else {
>
> - if (secure) {
> - LOG.debug("Opening secure passive data connection");
> - // this is where we wrap the unsecured socket as
> a SSLSocket. This is
> - // due to the JVM bug described in FTPSERVER-241.
> + if (secure) {
> + LOG.debug("Opening secure passive data connection");
> + // this is where we wrap the unsecured socket as a
> + // SSLSocket. This is
> + // due to the JVM bug described in FTPSERVER-241.
>
> - // get server socket factory
> - SslConfiguration ssl = getSslConfiguration();
> -
> - // we've already checked this, but let's do it again
> - if (ssl == null) {
> - throw new FtpException(
> - "Data connection SSL not configured");
> - }
> + // get server socket factory
> + SslConfiguration ssl = getSslConfiguration();
>
> - SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
> + // we've already checked this, but let's do it again
> + if (ssl == null) {
> + throw new FtpException(
> + "Data connection SSL not configured");
> + }
>
> - Socket serverSocket = servSoc.accept();
> + SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
>
> - SSLSocket sslSocket = (SSLSocket) ssocketFactory
> - .createSocket(serverSocket, serverSocket
> - .getInetAddress().getHostAddress(),
> - serverSocket.getPort(), true);
> - sslSocket.setUseClientMode(false);
> + Socket serverSocket = servSoc.accept();
>
> - // initialize server socket
> - if (ssl.getClientAuth() == ClientAuth.NEED) {
> - sslSocket.setNeedClientAuth(true);
> - } else if (ssl.getClientAuth() == ClientAuth.WANT) {
> - sslSocket.setWantClientAuth(true);
> - }
> + SSLSocket sslSocket = (SSLSocket) ssocketFactory
> + .createSocket(serverSocket, serverSocket
> + .getInetAddress().getHostAddress(),
> + serverSocket.getPort(), true);
> + sslSocket.setUseClientMode(false);
>
> - if (ssl.getEnabledCipherSuites() != null) {
> - sslSocket.setEnabledCipherSuites(ssl
> - .getEnabledCipherSuites());
> - }
> + // initialize server socket
> + if (ssl.getClientAuth() == ClientAuth.NEED) {
> + sslSocket.setNeedClientAuth(true);
> + } else if (ssl.getClientAuth() == ClientAuth.WANT) {
> + sslSocket.setWantClientAuth(true);
> + }
>
> - dataSoc = sslSocket;
> - } else {
> - LOG.debug("Opening passive data connection");
> + if (ssl.getEnabledCipherSuites() != null) {
> + sslSocket.setEnabledCipherSuites(ssl
> + .getEnabledCipherSuites());
> + }
>
> - dataSoc = servSoc.accept();
> - }
> -
> - if (dataConfig.isPassiveIpCheck()) {
> + dataSoc = sslSocket;
> + } else {
> + LOG.debug("Opening passive data connection");
> +
> + dataSoc = servSoc.accept();
> + }
> +
> + if (dataConfig.isPassiveIpCheck()) {
> // Let's make sure we got the connection from the same
> // client that we are expecting
> - InetAddress remoteAddress = ((InetSocketAddress)
> session.getRemoteAddress()).getAddress();
> + InetAddress remoteAddress = ((InetSocketAddress) session
> + .getRemoteAddress()).getAddress();
> InetAddress dataSocketAddress = dataSoc.getInetAddress();
> if (!dataSocketAddress.equals(remoteAddress)) {
> - LOG.warn("Passive IP Check failed. Closing data connection from "
> - + dataSocketAddress
> - + " as it does not match the expected address "
> - + remoteAddress);
> + LOG
> + .warn("Passive IP Check failed. Closing data connection from "
> + + dataSocketAddress
> + + " as it does not match the expected address "
> + + remoteAddress);
> closeDataConnection();
> return null;
> }
> }
> -
> - DataConnectionConfiguration dataCfg = session.getListener()
> - .getDataConnectionConfiguration();
> -
> - dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
> - LOG.debug("Passive data connection opened");
> - }
> - } catch (Exception ex) {
> - closeDataConnection();
> - LOG.warn("FtpDataConnection.getDataSocket()", ex);
> - throw ex;
> - }
> - dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
>
> - // Make sure we initiate the SSL handshake, or we'll
> - // get an error if we turn out not to send any data
> - // e.g. during the listing of an empty directory
> - if (dataSoc instanceof SSLSocket) {
> - ((SSLSocket) dataSoc).startHandshake();
> - }
> + DataConnectionConfiguration dataCfg = session.getListener()
> + .getDataConnectionConfiguration();
>
> - return dataSoc;
> - }
> + dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
> + LOG.debug("Passive data connection opened");
> + }
> + } catch (Exception ex) {
> + closeDataConnection();
> + LOG.warn("FtpDataConnection.getDataSocket()", ex);
> + throw ex;
> + }
> + dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
>
> - /*
> - * (non-Javadoc)
> - * Returns an InetAddress object from a hostname or IP address.
> - */
> - private InetAddress resolveAddress(String host)
> - throws DataConnectionException {
> - if (host == null) {
> - return null;
> - } else {
> - try {
> - return InetAddress.getByName(host);
> - } catch (UnknownHostException ex) {
> - throw new DataConnectionException("Failed to resolve
> address", ex);
> - }
> - }
> - }
> + // Make sure we initiate the SSL handshake, or we'll
> + // get an error if we turn out not to send any data
> + // e.g. during the listing of an empty directory
> + if (dataSoc instanceof SSLSocket) {
> + ((SSLSocket) dataSoc).startHandshake();
> + }
>
> - /*
> - * (non-Javadoc)
> - *
> - * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
> - */
> - public boolean isSecure() {
> - return secure;
> - }
> + return dataSoc;
> + }
>
> - /**
> - * Set the security protocol.
> - */
> - public void setSecure(final boolean secure) {
> - this.secure = secure;
> - }
> + /*
> + * (non-Javadoc) Returns an InetAddress object from a hostname or IP
> + * address.
> + */
> + private InetAddress resolveAddress(String host)
> + throws DataConnectionException {
> + if (host == null) {
> + return null;
> + } else {
> + try {
> + return InetAddress.getByName(host);
> + } catch (UnknownHostException ex) {
> + throw new DataConnectionException("Failed to resolve address",
> + ex);
> + }
> + }
> + }
>
> - /*
> - * (non-Javadoc)
> - *
> - * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
> - */
> - public boolean isZipMode() {
> - return isZip;
> - }
> + /*
> + * (non-Javadoc)
> + *
> + * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
> + */
> + public boolean isSecure() {
> + return secure;
> + }
>
> - /**
> - * Set zip mode.
> - */
> - public void setZipMode(final boolean zip) {
> - isZip = zip;
> - }
> + /**
> + * Set the security protocol.
> + */
> + public void setSecure(final boolean secure) {
> + this.secure = secure;
> + }
>
> - /**
> - * Check the data connection idle status.
> - */
> - public synchronized boolean isTimeout(final long currTime) {
> + /*
> + * (non-Javadoc)
> + *
> + * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
> + */
> + public boolean isZipMode() {
> + return isZip;
> + }
>
> - // data connection not requested - not a timeout
> - if (requestTime == 0L) {
> - return false;
> - }
> + /**
> + * Set zip mode.
> + */
> + public void setZipMode(final boolean zip) {
> + isZip = zip;
> + }
>
> - // data connection active - not a timeout
> - if (dataSoc != null) {
> - return false;
> - }
> + /**
> + * Check the data connection idle status.
> + */
> + public synchronized boolean isTimeout(final long currTime) {
>
> - // no idle time limit - not a timeout
> - int maxIdleTime = session.getListener()
> - .getDataConnectionConfiguration().getIdleTime() * 1000;
> - if (maxIdleTime == 0) {
> - return false;
> - }
> + // data connection not requested - not a timeout
> + if (requestTime == 0L) {
> + return false;
> + }
>
> - // idle time is within limit - not a timeout
> - if ((currTime - requestTime) < maxIdleTime) {
> - return false;
> - }
> + // data connection active - not a timeout
> + if (dataSoc != null) {
> + return false;
> + }
>
> - return true;
> - }
> + // no idle time limit - not a timeout
> + int maxIdleTime = session.getListener()
> + .getDataConnectionConfiguration().getIdleTime() * 1000;
> + if (maxIdleTime == 0) {
> + return false;
> + }
>
> - /**
> - * Dispose data connection - close all the sockets.
> - */
> - public void dispose() {
> - closeDataConnection();
> - }
> + // idle time is within limit - not a timeout
> + if ((currTime - requestTime) < maxIdleTime) {
> + return false;
> + }
>
> - /**
> - * Sets the server's control address.
> - */
> - public void setServerControlAddress(final InetAddress
> serverControlAddress) {
> - this.serverControlAddress = serverControlAddress;
> - }
> + return true;
> + }
> +
> + /**
> + * Dispose data connection - close all the sockets.
> + */
> + public void dispose() {
> + closeDataConnection();
> + }
> +
> + /**
> + * Sets the server's control address.
> + */
> + public void setServerControlAddress(final InetAddress serverControlAddress) {
> + this.serverControlAddress = serverControlAddress;
> + }
> }
>
>
>
>
>
> 2010/3/25 Sai Pullabhotla <sa...@jmethods.com>:
>> Just so we are all on the same page -
>>
>> Do you think the issue is with the LIST command, or the code that
>> creates passive data connection? Could you briefly explain some of the
>> issues that made you think it should be rewritten, so everyone gets a
>> chance to evaluate?
>>
>> Thanks.
>>
>> Regards,
>> Sai Pullabhotla
>>
>>
>>
>>
>>
>> On Thu, Mar 25, 2010 at 5:56 AM, David Latorre <dv...@gmail.com> wrote:
>>> 2010/3/24 Niklas Gustavsson <ni...@protocol7.com>:
>>>> On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fr...@gmail.com> wrote:
>>>>> we found an issue related to requestPassivePort() which may lead to an
>>>>> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced.
>>>>>
>>>>> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full description
>>>>> of the symptoms and a minimalist java client and server to reproduce it.
>>>>
>>>> I haven't yet looked closer at the code you attached. But, I have seen
>>>> similar behavior myself during performance testing FtpServer. In that
>>>> case, I had a very similar behavior at around 20 threads. However, the
>>>> reason for the problem in that test was that the test case uses up
>>>> file handles (for the sockets) so fast that they will run out. Since
>>>> sockets hang around for some time also after closing, they were not
>>>> freed quickly enough and thus FtpServer could not open new ones. Could
>>>> you please verify that this is not the case here? You could look at
>>>> netstat output and look into increasing the allowed number of file
>>>> handles our the timeout time for sockets in your OS.
>>>
>>>
>>> Actually it is quite easy to reproduce this error (I just wrote a
>>> client test case with throws >20~30 threads that list a directory in
>>> the server ) and it's not file handle related:
>>> we have several bugs in our code that cause this behaviour , i
>>> think we hould rewrite all the request/release passive port mechanism
>>> as there are several issues with it.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>> /niklas
>>>>
>>>
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by David Latorre <dv...@gmail.com>.
hello Sai,
I didn't have time to fully review the code but i made these nearly
random changes in IODataConnectionFactory and
DefaultDataConnectionConfiguration and the server is not getting
blocked anymore...
Index: main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java
===================================================================
--- main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (revision
927354)
+++ main/java/org/apache/ftpserver/impl/DefaultDataConnectionConfiguration.java (working
copy)
@@ -129,7 +129,8 @@
* port will be used.
*/
public synchronized int requestPassivePort() {
- int dataPort = -1;
+
+ int dataPort = -1;
int loopTimes = 2;
Thread currThread = Thread.currentThread();
@@ -164,8 +165,7 @@
*/
public synchronized void releasePassivePort(final int port) {
passivePorts.releasePort(port);
-
- notify();
+ notifyAll();
}
/**
Index: main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java
===================================================================
--- main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (revision
927354)
+++ main/java/org/apache/ftpserver/impl/IODataConnectionFactory.java (working
copy)
@@ -44,446 +44,459 @@
*
* We can get the FTP data connection using this class. It uses either PORT or
* PASV command.
- *
+ *
* @author <a href="http://mina.apache.org">Apache MINA Project</a>
*/
public class IODataConnectionFactory implements ServerDataConnectionFactory {
- private final Logger LOG = LoggerFactory
- .getLogger(IODataConnectionFactory.class);
+ private final Logger LOG = LoggerFactory
+ .getLogger(IODataConnectionFactory.class);
- private FtpServerContext serverContext;
+ private FtpServerContext serverContext;
- private Socket dataSoc;
+ private Socket dataSoc;
- ServerSocket servSoc;
+ ServerSocket servSoc;
- InetAddress address;
+ InetAddress address;
- int port = 0;
+ int port = 0;
- long requestTime = 0L;
+ long requestTime = 0L;
- boolean passive = false;
+ boolean passive = false;
- boolean secure = false;
+ boolean secure = false;
- private boolean isZip = false;
+ private boolean isZip = false;
- InetAddress serverControlAddress;
+ InetAddress serverControlAddress;
- FtpIoSession session;
+ FtpIoSession session;
- public IODataConnectionFactory(final FtpServerContext serverContext,
- final FtpIoSession session) {
- this.session = session;
- this.serverContext = serverContext;
- if (session.getListener().getDataConnectionConfiguration()
- .isImplicitSsl()) {
- secure = true;
- }
- }
+ public IODataConnectionFactory(final FtpServerContext serverContext,
+ final FtpIoSession session) {
+ this.session = session;
+ this.serverContext = serverContext;
+ if (session.getListener().getDataConnectionConfiguration()
+ .isImplicitSsl()) {
+ secure = true;
+ }
+ }
- /**
- * Close data socket.
- * This method must be idempotent as we might call it multiple
times during disconnect.
- */
- public synchronized void closeDataConnection() {
+ /**
+ * Close data socket. This method must be idempotent as we might call it
+ * multiple times during disconnect.
+ */
+ public synchronized void closeDataConnection() {
- // close client socket if any
- if (dataSoc != null) {
- try {
- dataSoc.close();
- } catch (Exception ex) {
- LOG.warn("FtpDataConnection.closeDataSocket()", ex);
- }
- dataSoc = null;
- }
+ // close client socket if any
+ if (dataSoc != null) {
+ try {
+ dataSoc.close();
+ } catch (Exception ex) {
+ LOG.warn("FtpDataConnection.closeDataSocket()", ex);
+ }
+ dataSoc = null;
+ }
- // close server socket if any
- if (servSoc != null) {
- try {
- servSoc.close();
- } catch (Exception ex) {
- LOG.warn("FtpDataConnection.closeDataSocket()", ex);
- }
+ // close server socket if any
+ if (servSoc != null) {
+ try {
+ servSoc.close();
+ } catch (Exception ex) {
+ LOG.warn("FtpDataConnection.closeDataSocket()", ex);
+ }
- if (session != null) {
- DataConnectionConfiguration dcc = session.getListener()
- .getDataConnectionConfiguration();
- if (dcc != null) {
- dcc.releasePassivePort(port);
- }
- }
+ servSoc = null;
+ }
- servSoc = null;
- }
+ // release passive ports
+ if (session != null) {
+ DataConnectionConfiguration dcc = session.getListener()
+ .getDataConnectionConfiguration();
+ if (dcc != null) {
+ dcc.releasePassivePort(port);
+ }
+ }
+
+ // reset request time
+ requestTime = 0L;
+ }
- // reset request time
- requestTime = 0L;
- }
+ /**
+ * Port command.
+ */
+ public synchronized void initActiveDataConnection(
+ final InetSocketAddress address) {
- /**
- * Port command.
- */
- public synchronized void initActiveDataConnection(
- final InetSocketAddress address) {
+ // close old sockets if any
+ closeDataConnection();
- // close old sockets if any
- closeDataConnection();
+ // set variables
+ passive = false;
+ this.address = address.getAddress();
+ port = address.getPort();
+ requestTime = System.currentTimeMillis();
+ }
- // set variables
- passive = false;
- this.address = address.getAddress();
- port = address.getPort();
- requestTime = System.currentTimeMillis();
- }
+ private SslConfiguration getSslConfiguration() {
+ DataConnectionConfiguration dataCfg = session.getListener()
+ .getDataConnectionConfiguration();
- private SslConfiguration getSslConfiguration() {
- DataConnectionConfiguration dataCfg = session.getListener()
- .getDataConnectionConfiguration();
+ SslConfiguration configuration = dataCfg.getSslConfiguration();
- SslConfiguration configuration = dataCfg.getSslConfiguration();
+ // fall back if no configuration has been provided on the data
+ // connection config
+ if (configuration == null) {
+ configuration = session.getListener().getSslConfiguration();
+ }
- // fall back if no configuration has been provided on the
data connection config
- if (configuration == null) {
- configuration = session.getListener().getSslConfiguration();
- }
+ return configuration;
+ }
- return configuration;
- }
+ /**
+ * Initiate a data connection in passive mode (server listening).
+ */
+ public synchronized InetSocketAddress initPassiveDataConnection()
+ throws DataConnectionException {
+ LOG.debug("Initiating passive data connection");
+ // close old sockets if any
+ closeDataConnection();
- /**
- * Initiate a data connection in passive mode (server listening).
- */
- public synchronized InetSocketAddress initPassiveDataConnection()
- throws DataConnectionException {
- LOG.debug("Initiating passive data connection");
- // close old sockets if any
- closeDataConnection();
+ // get the passive port
+ int passivePort = session.getListener()
+ .getDataConnectionConfiguration().requestPassivePort();
+ if (passivePort == -1) {
+ servSoc = null;
+ throw new DataConnectionException(
+ "Cannot find an available passive port.");
+ }
- // get the passive port
- int passivePort = session.getListener()
- .getDataConnectionConfiguration().requestPassivePort();
- if (passivePort == -1) {
- servSoc = null;
- throw new DataConnectionException(
- "Cannot find an available passive port.");
- }
+ // open passive server socket and get parameters
+ try {
+ DataConnectionConfiguration dataCfg = session.getListener()
+ .getDataConnectionConfiguration();
- // open passive server socket and get parameters
- try {
- DataConnectionConfiguration dataCfg = session.getListener()
- .getDataConnectionConfiguration();
+ String passiveAddress = dataCfg.getPassiveAddress();
- String passiveAddress = dataCfg.getPassiveAddress();
+ if (passiveAddress == null) {
+ address = serverControlAddress;
+ } else {
+ address = resolveAddress(dataCfg.getPassiveAddress());
+ }
- if (passiveAddress == null) {
- address = serverControlAddress;
- } else {
- address = resolveAddress(dataCfg.getPassiveAddress());
- }
+ if (secure) {
+ LOG
+ .debug(
+ "Opening SSL passive data connection on address \"{}\" and port {}",
+ address, passivePort);
+ SslConfiguration ssl = getSslConfiguration();
+ if (ssl == null) {
+ throw new DataConnectionException(
+ "Data connection SSL required but not configured.");
+ }
- if (secure) {
- LOG
- .debug(
- "Opening SSL passive data connection
on address \"{}\" and port {}",
- address, passivePort);
- SslConfiguration ssl = getSslConfiguration();
- if (ssl == null) {
- throw new DataConnectionException(
- "Data connection SSL required but not
configured.");
- }
+ // this method does not actually create the SSL socket, due to a
+ // JVM bug
+ // (https://issues.apache.org/jira/browse/FTPSERVER-241).
+ // Instead, it creates a regular
+ // ServerSocket that will be wrapped as a SSL socket in
+ // createDataSocket()
+ servSoc = new ServerSocket(passivePort, 0, address);
+ LOG
+ .debug(
+ "SSL Passive data connection created on address \"{}\" and port {}",
+ address, passivePort);
+ } else {
+ LOG
+ .debug(
+ "Opening passive data connection on address \"{}\" and port {}",
+ address, passivePort);
+ servSoc = new ServerSocket(passivePort, 0, address);
+ LOG
+ .debug(
+ "Passive data connection created on address \"{}\" and port {}",
+ address, passivePort);
+ }
+ port = servSoc.getLocalPort();
+ servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
- // this method does not actually create the SSL
socket, due to a JVM bug
- // (https://issues.apache.org/jira/browse/FTPSERVER-241).
- // Instead, it creates a regular
- // ServerSocket that will be wrapped as a SSL socket
in createDataSocket()
- servSoc = new ServerSocket(passivePort, 0, address);
- LOG
- .debug(
- "SSL Passive data connection created
on address \"{}\" and port {}",
- address, passivePort);
- } else {
- LOG
- .debug(
- "Opening passive data connection on
address \"{}\" and port {}",
- address, passivePort);
- servSoc = new ServerSocket(passivePort, 0, address);
- LOG
- .debug(
- "Passive data connection created on
address \"{}\" and port {}",
- address, passivePort);
- }
- port = servSoc.getLocalPort();
- servSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
+ // set different state variables
+ passive = true;
+ requestTime = System.currentTimeMillis();
- // set different state variables
- passive = true;
- requestTime = System.currentTimeMillis();
+ return new InetSocketAddress(address, port);
+ } catch (Exception ex) {
+ servSoc = null;
+ closeDataConnection();
+ throw new DataConnectionException(
+ "Failed to initate passive data connection: "
+ + ex.getMessage(), ex);
+ }
+ }
- return new InetSocketAddress(address, port);
- } catch (Exception ex) {
- servSoc = null;
- closeDataConnection();
- throw new DataConnectionException(
- "Failed to initate passive data connection: "
- + ex.getMessage(), ex);
- }
- }
+ /*
+ * (non-Javadoc)
+ *
+ * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
+ */
+ public InetAddress getInetAddress() {
+ return address;
+ }
- /*
- * (non-Javadoc)
- *
- * @see org.apache.ftpserver.FtpDataConnectionFactory2#getInetAddress()
- */
- public InetAddress getInetAddress() {
- return address;
- }
+ /*
+ * (non-Javadoc)
+ *
+ * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
+ */
+ public int getPort() {
+ return port;
+ }
- /*
- * (non-Javadoc)
- *
- * @see org.apache.ftpserver.FtpDataConnectionFactory2#getPort()
- */
- public int getPort() {
- return port;
- }
+ /*
+ * (non-Javadoc)
+ *
+ * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
+ */
+ public DataConnection openConnection() throws Exception {
+ return new IODataConnection(createDataSocket(), session, this);
+ }
- /*
- * (non-Javadoc)
- *
- * @see org.apache.ftpserver.FtpDataConnectionFactory2#openConnection()
- */
- public DataConnection openConnection() throws Exception {
- return new IODataConnection(createDataSocket(), session, this);
- }
+ /**
+ * Get the data socket. In case of error returns null.
+ */
+ private synchronized Socket createDataSocket() throws Exception {
- /**
- * Get the data socket. In case of error returns null.
- */
- private synchronized Socket createDataSocket() throws Exception {
+ // get socket depending on the selection
+ dataSoc = null;
+ DataConnectionConfiguration dataConfig = session.getListener()
+ .getDataConnectionConfiguration();
+ try {
+ if (!passive) {
+ if (secure) {
+ LOG.debug("Opening secure active data connection");
+ SslConfiguration ssl = getSslConfiguration();
+ if (ssl == null) {
+ throw new FtpException(
+ "Data connection SSL not configured");
+ }
- // get socket depending on the selection
- dataSoc = null;
- DataConnectionConfiguration dataConfig = session.getListener()
- .getDataConnectionConfiguration();
- try {
- if (!passive) {
- if (secure) {
- LOG.debug("Opening secure active data connection");
- SslConfiguration ssl = getSslConfiguration();
- if (ssl == null) {
- throw new FtpException(
- "Data connection SSL not configured");
- }
+ // get socket factory
+ SSLSocketFactory socFactory = ssl.getSocketFactory();
- // get socket factory
- SSLSocketFactory socFactory = ssl.getSocketFactory();
+ // create socket
+ SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
+ ssoc.setUseClientMode(false);
- // create socket
- SSLSocket ssoc = (SSLSocket) socFactory.createSocket();
- ssoc.setUseClientMode(false);
+ // initialize socket
+ if (ssl.getEnabledCipherSuites() != null) {
+ ssoc.setEnabledCipherSuites(ssl
+ .getEnabledCipherSuites());
+ }
+ dataSoc = ssoc;
+ } else {
+ LOG.debug("Opening active data connection");
+ dataSoc = new Socket();
+ }
- // initialize socket
- if (ssl.getEnabledCipherSuites() != null) {
-
ssoc.setEnabledCipherSuites(ssl.getEnabledCipherSuites());
- }
- dataSoc = ssoc;
- } else {
- LOG.debug("Opening active data connection");
- dataSoc = new Socket();
- }
+ dataSoc.setReuseAddress(true);
- dataSoc.setReuseAddress(true);
+ InetAddress localAddr = resolveAddress(dataConfig
+ .getActiveLocalAddress());
- InetAddress localAddr = resolveAddress(dataConfig
- .getActiveLocalAddress());
+ // if no local address has been configured, make sure we use the
+ // same as the client connects from
+ if (localAddr == null) {
+ localAddr = ((InetSocketAddress) session.getLocalAddress())
+ .getAddress();
+ }
- // if no local address has been configured, make sure
we use the same as the client connects from
- if(localAddr == null) {
- localAddr =
((InetSocketAddress)session.getLocalAddress()).getAddress();
- }
+ SocketAddress localSocketAddress = new InetSocketAddress(
+ localAddr, dataConfig.getActiveLocalPort());
- SocketAddress localSocketAddress = new
InetSocketAddress(localAddr, dataConfig.getActiveLocalPort());
-
- LOG.debug("Binding active data connection to {}",
localSocketAddress);
- dataSoc.bind(localSocketAddress);
+ LOG.debug("Binding active data connection to {}",
+ localSocketAddress);
+ dataSoc.bind(localSocketAddress);
- dataSoc.connect(new InetSocketAddress(address, port));
- } else {
+ dataSoc.connect(new InetSocketAddress(address, port));
+ } else {
- if (secure) {
- LOG.debug("Opening secure passive data connection");
- // this is where we wrap the unsecured socket as
a SSLSocket. This is
- // due to the JVM bug described in FTPSERVER-241.
+ if (secure) {
+ LOG.debug("Opening secure passive data connection");
+ // this is where we wrap the unsecured socket as a
+ // SSLSocket. This is
+ // due to the JVM bug described in FTPSERVER-241.
- // get server socket factory
- SslConfiguration ssl = getSslConfiguration();
-
- // we've already checked this, but let's do it again
- if (ssl == null) {
- throw new FtpException(
- "Data connection SSL not configured");
- }
+ // get server socket factory
+ SslConfiguration ssl = getSslConfiguration();
- SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
+ // we've already checked this, but let's do it again
+ if (ssl == null) {
+ throw new FtpException(
+ "Data connection SSL not configured");
+ }
- Socket serverSocket = servSoc.accept();
+ SSLSocketFactory ssocketFactory = ssl.getSocketFactory();
- SSLSocket sslSocket = (SSLSocket) ssocketFactory
- .createSocket(serverSocket, serverSocket
- .getInetAddress().getHostAddress(),
- serverSocket.getPort(), true);
- sslSocket.setUseClientMode(false);
+ Socket serverSocket = servSoc.accept();
- // initialize server socket
- if (ssl.getClientAuth() == ClientAuth.NEED) {
- sslSocket.setNeedClientAuth(true);
- } else if (ssl.getClientAuth() == ClientAuth.WANT) {
- sslSocket.setWantClientAuth(true);
- }
+ SSLSocket sslSocket = (SSLSocket) ssocketFactory
+ .createSocket(serverSocket, serverSocket
+ .getInetAddress().getHostAddress(),
+ serverSocket.getPort(), true);
+ sslSocket.setUseClientMode(false);
- if (ssl.getEnabledCipherSuites() != null) {
- sslSocket.setEnabledCipherSuites(ssl
- .getEnabledCipherSuites());
- }
+ // initialize server socket
+ if (ssl.getClientAuth() == ClientAuth.NEED) {
+ sslSocket.setNeedClientAuth(true);
+ } else if (ssl.getClientAuth() == ClientAuth.WANT) {
+ sslSocket.setWantClientAuth(true);
+ }
- dataSoc = sslSocket;
- } else {
- LOG.debug("Opening passive data connection");
+ if (ssl.getEnabledCipherSuites() != null) {
+ sslSocket.setEnabledCipherSuites(ssl
+ .getEnabledCipherSuites());
+ }
- dataSoc = servSoc.accept();
- }
-
- if (dataConfig.isPassiveIpCheck()) {
+ dataSoc = sslSocket;
+ } else {
+ LOG.debug("Opening passive data connection");
+
+ dataSoc = servSoc.accept();
+ }
+
+ if (dataConfig.isPassiveIpCheck()) {
// Let's make sure we got the connection from the same
// client that we are expecting
- InetAddress remoteAddress = ((InetSocketAddress)
session.getRemoteAddress()).getAddress();
+ InetAddress remoteAddress = ((InetSocketAddress) session
+ .getRemoteAddress()).getAddress();
InetAddress dataSocketAddress = dataSoc.getInetAddress();
if (!dataSocketAddress.equals(remoteAddress)) {
- LOG.warn("Passive IP Check failed. Closing data connection from "
- + dataSocketAddress
- + " as it does not match the expected address "
- + remoteAddress);
+ LOG
+ .warn("Passive IP Check failed. Closing data connection from "
+ + dataSocketAddress
+ + " as it does not match the expected address "
+ + remoteAddress);
closeDataConnection();
return null;
}
}
-
- DataConnectionConfiguration dataCfg = session.getListener()
- .getDataConnectionConfiguration();
-
- dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
- LOG.debug("Passive data connection opened");
- }
- } catch (Exception ex) {
- closeDataConnection();
- LOG.warn("FtpDataConnection.getDataSocket()", ex);
- throw ex;
- }
- dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
- // Make sure we initiate the SSL handshake, or we'll
- // get an error if we turn out not to send any data
- // e.g. during the listing of an empty directory
- if (dataSoc instanceof SSLSocket) {
- ((SSLSocket) dataSoc).startHandshake();
- }
+ DataConnectionConfiguration dataCfg = session.getListener()
+ .getDataConnectionConfiguration();
- return dataSoc;
- }
+ dataSoc.setSoTimeout(dataCfg.getIdleTime() * 1000);
+ LOG.debug("Passive data connection opened");
+ }
+ } catch (Exception ex) {
+ closeDataConnection();
+ LOG.warn("FtpDataConnection.getDataSocket()", ex);
+ throw ex;
+ }
+ dataSoc.setSoTimeout(dataConfig.getIdleTime() * 1000);
- /*
- * (non-Javadoc)
- * Returns an InetAddress object from a hostname or IP address.
- */
- private InetAddress resolveAddress(String host)
- throws DataConnectionException {
- if (host == null) {
- return null;
- } else {
- try {
- return InetAddress.getByName(host);
- } catch (UnknownHostException ex) {
- throw new DataConnectionException("Failed to resolve
address", ex);
- }
- }
- }
+ // Make sure we initiate the SSL handshake, or we'll
+ // get an error if we turn out not to send any data
+ // e.g. during the listing of an empty directory
+ if (dataSoc instanceof SSLSocket) {
+ ((SSLSocket) dataSoc).startHandshake();
+ }
- /*
- * (non-Javadoc)
- *
- * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
- */
- public boolean isSecure() {
- return secure;
- }
+ return dataSoc;
+ }
- /**
- * Set the security protocol.
- */
- public void setSecure(final boolean secure) {
- this.secure = secure;
- }
+ /*
+ * (non-Javadoc) Returns an InetAddress object from a hostname or IP
+ * address.
+ */
+ private InetAddress resolveAddress(String host)
+ throws DataConnectionException {
+ if (host == null) {
+ return null;
+ } else {
+ try {
+ return InetAddress.getByName(host);
+ } catch (UnknownHostException ex) {
+ throw new DataConnectionException("Failed to resolve address",
+ ex);
+ }
+ }
+ }
- /*
- * (non-Javadoc)
- *
- * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
- */
- public boolean isZipMode() {
- return isZip;
- }
+ /*
+ * (non-Javadoc)
+ *
+ * @see org.apache.ftpserver.DataConnectionFactory#isSecure()
+ */
+ public boolean isSecure() {
+ return secure;
+ }
- /**
- * Set zip mode.
- */
- public void setZipMode(final boolean zip) {
- isZip = zip;
- }
+ /**
+ * Set the security protocol.
+ */
+ public void setSecure(final boolean secure) {
+ this.secure = secure;
+ }
- /**
- * Check the data connection idle status.
- */
- public synchronized boolean isTimeout(final long currTime) {
+ /*
+ * (non-Javadoc)
+ *
+ * @see org.apache.ftpserver.DataConnectionFactory#isZipMode()
+ */
+ public boolean isZipMode() {
+ return isZip;
+ }
- // data connection not requested - not a timeout
- if (requestTime == 0L) {
- return false;
- }
+ /**
+ * Set zip mode.
+ */
+ public void setZipMode(final boolean zip) {
+ isZip = zip;
+ }
- // data connection active - not a timeout
- if (dataSoc != null) {
- return false;
- }
+ /**
+ * Check the data connection idle status.
+ */
+ public synchronized boolean isTimeout(final long currTime) {
- // no idle time limit - not a timeout
- int maxIdleTime = session.getListener()
- .getDataConnectionConfiguration().getIdleTime() * 1000;
- if (maxIdleTime == 0) {
- return false;
- }
+ // data connection not requested - not a timeout
+ if (requestTime == 0L) {
+ return false;
+ }
- // idle time is within limit - not a timeout
- if ((currTime - requestTime) < maxIdleTime) {
- return false;
- }
+ // data connection active - not a timeout
+ if (dataSoc != null) {
+ return false;
+ }
- return true;
- }
+ // no idle time limit - not a timeout
+ int maxIdleTime = session.getListener()
+ .getDataConnectionConfiguration().getIdleTime() * 1000;
+ if (maxIdleTime == 0) {
+ return false;
+ }
- /**
- * Dispose data connection - close all the sockets.
- */
- public void dispose() {
- closeDataConnection();
- }
+ // idle time is within limit - not a timeout
+ if ((currTime - requestTime) < maxIdleTime) {
+ return false;
+ }
- /**
- * Sets the server's control address.
- */
- public void setServerControlAddress(final InetAddress
serverControlAddress) {
- this.serverControlAddress = serverControlAddress;
- }
+ return true;
+ }
+
+ /**
+ * Dispose data connection - close all the sockets.
+ */
+ public void dispose() {
+ closeDataConnection();
+ }
+
+ /**
+ * Sets the server's control address.
+ */
+ public void setServerControlAddress(final InetAddress serverControlAddress) {
+ this.serverControlAddress = serverControlAddress;
+ }
}
2010/3/25 Sai Pullabhotla <sa...@jmethods.com>:
> Just so we are all on the same page -
>
> Do you think the issue is with the LIST command, or the code that
> creates passive data connection? Could you briefly explain some of the
> issues that made you think it should be rewritten, so everyone gets a
> chance to evaluate?
>
> Thanks.
>
> Regards,
> Sai Pullabhotla
>
>
>
>
>
> On Thu, Mar 25, 2010 at 5:56 AM, David Latorre <dv...@gmail.com> wrote:
>> 2010/3/24 Niklas Gustavsson <ni...@protocol7.com>:
>>> On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fr...@gmail.com> wrote:
>>>> we found an issue related to requestPassivePort() which may lead to an
>>>> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced.
>>>>
>>>> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full description
>>>> of the symptoms and a minimalist java client and server to reproduce it.
>>>
>>> I haven't yet looked closer at the code you attached. But, I have seen
>>> similar behavior myself during performance testing FtpServer. In that
>>> case, I had a very similar behavior at around 20 threads. However, the
>>> reason for the problem in that test was that the test case uses up
>>> file handles (for the sockets) so fast that they will run out. Since
>>> sockets hang around for some time also after closing, they were not
>>> freed quickly enough and thus FtpServer could not open new ones. Could
>>> you please verify that this is not the case here? You could look at
>>> netstat output and look into increasing the allowed number of file
>>> handles our the timeout time for sockets in your OS.
>>
>>
>> Actually it is quite easy to reproduce this error (I just wrote a
>> client test case with throws >20~30 threads that list a directory in
>> the server ) and it's not file handle related:
>> we have several bugs in our code that cause this behaviour , i
>> think we hould rewrite all the request/release passive port mechanism
>> as there are several issues with it.
>>
>>
>>
>>
>>
>>
>>
>>> /niklas
>>>
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Sai Pullabhotla <sa...@jmethods.com>.
Just so we are all on the same page -
Do you think the issue is with the LIST command, or the code that
creates passive data connection? Could you briefly explain some of the
issues that made you think it should be rewritten, so everyone gets a
chance to evaluate?
Thanks.
Regards,
Sai Pullabhotla
On Thu, Mar 25, 2010 at 5:56 AM, David Latorre <dv...@gmail.com> wrote:
> 2010/3/24 Niklas Gustavsson <ni...@protocol7.com>:
>> On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fr...@gmail.com> wrote:
>>> we found an issue related to requestPassivePort() which may lead to an
>>> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced.
>>>
>>> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full description
>>> of the symptoms and a minimalist java client and server to reproduce it.
>>
>> I haven't yet looked closer at the code you attached. But, I have seen
>> similar behavior myself during performance testing FtpServer. In that
>> case, I had a very similar behavior at around 20 threads. However, the
>> reason for the problem in that test was that the test case uses up
>> file handles (for the sockets) so fast that they will run out. Since
>> sockets hang around for some time also after closing, they were not
>> freed quickly enough and thus FtpServer could not open new ones. Could
>> you please verify that this is not the case here? You could look at
>> netstat output and look into increasing the allowed number of file
>> handles our the timeout time for sockets in your OS.
>
>
> Actually it is quite easy to reproduce this error (I just wrote a
> client test case with throws >20~30 threads that list a directory in
> the server ) and it's not file handle related:
> we have several bugs in our code that cause this behaviour , i
> think we hould rewrite all the request/release passive port mechanism
> as there are several issues with it.
>
>
>
>
>
>
>
>> /niklas
>>
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by David Latorre <dv...@gmail.com>.
2010/3/24 Niklas Gustavsson <ni...@protocol7.com>:
> On Wed, Mar 24, 2010 at 6:03 PM, Fred Moore <fr...@gmail.com> wrote:
>> we found an issue related to requestPassivePort() which may lead to an
>> unresponsive V1.0.4 FTP (or FTP/S) Server, this issue can be reproduced.
>>
>> http://issues.apache.org/jira/browse/FTPSERVER-359 contains full description
>> of the symptoms and a minimalist java client and server to reproduce it.
>
> I haven't yet looked closer at the code you attached. But, I have seen
> similar behavior myself during performance testing FtpServer. In that
> case, I had a very similar behavior at around 20 threads. However, the
> reason for the problem in that test was that the test case uses up
> file handles (for the sockets) so fast that they will run out. Since
> sockets hang around for some time also after closing, they were not
> freed quickly enough and thus FtpServer could not open new ones. Could
> you please verify that this is not the case here? You could look at
> netstat output and look into increasing the allowed number of file
> handles our the timeout time for sockets in your OS.
Actually it is quite easy to reproduce this error (I just wrote a
client test case with throws >20~30 threads that list a directory in
the server ) and it's not file handle related:
we have several bugs in our code that cause this behaviour , i
think we hould rewrite all the request/release passive port mechanism
as there are several issues with it.
> /niklas
>
Re: FTPServer 1.0.4: suspect race condition during
requestPassivePort() hanging FTP(S) server with one passive data connection
port
Posted by Fred Moore <fr...@gmail.com>.
Hi Niklas,
thanks for the response... we double checked our "socket footprint": it
seems to be perfectly normal (a little bit more than the number of client
threads, less than 32).
The second finding is that the repro tends to be more effective when you run
the repro client -- which only issues LISTs -- against a directory preloaded
with a several hundreds files.
I'd say that this is not the same thing you experienced then...
Any clues?
Cheers,
F.
--
http://issues.apache.org/jira/browse/FTPSERVER-359