This project is read-only.
3
Vote

"Malformed PASV response" with latest version

description

Hi,

I've update from version 1.0.5281.14359 to the latest version and i get this exception:

An unhandled exception of type 'System.Net.FtpClient.FtpException' occurred in FtpClientTest.exe
Additional information: Malformed PASV response: 17599

It is thrown in FtpClient.cs after checking the regex:
if (!m.Success || m.Groups.Count != 7)
    throw new FtpException(string.Format("Malformed PASV response: {0}", reply.Message));
The "reply.Message" that is checked with the regex has the value "17599" - which is strange of course, but everything work perfectly fine with the older version.

Bernhard

file attachments

comments

bp74 wrote Dec 18, 2015 at 9:11 AM

Additional Information:

Let's take this sample code for testing, i download several files in a loop:
foreach (var file in files)
{
    Console.WriteLine(file.FullName);

    using (var stream = client.OpenRead(file.FullName, FtpDataType.Binary))
    {
        using (var memoryStream = new MemoryStream())
        {
            stream.CopyTo(memoryStream);
            var test = memoryStream.ToArray();
        }
    }
}
1) The exception does not occur on the first file that is downloaded, but on the second file.
2) If i run this sample code in the debugger and step over all of those lines it works fine too!

So maybe there is some kind of race condition that does not happen if i run the application slowly in the debugger, but does happen when the application runs at full speed.

bp74 wrote Dec 19, 2015 at 7:08 AM

Additional Information:

The number in the exception (in the original comment 17599) is the file size of the file that throws the exception. It seems that the response of the last command is used for the PASV response.

The file listing is requested with this code:
var files = client.GetListing(remotePath, FtpListOption.SizeModify);

jptrosclair wrote Dec 25, 2015 at 6:17 PM

I agree this is likely a race condition and there was a reworking of the locking I merged into the latest build. Could you post a full log to help debug?

bp74 wrote Jan 7, 2016 at 11:42 AM

Sorry for the late reply. The log file is attached to this message.

This first file is donwloaded successfully:
SIZE /aav/remotecontrols/028B7398-81DA-40DC-A749-23F40BEC944C.xml
213 10306
PASV
227 Entering Passive Mode (80,120,10,7,214,241).
InterNetwork: 80.120.10.7
RETR /aav/remotecontrols/028B7398-81DA-40DC-A749-23F40BEC944C.xml
125 Data connection already open; Transfer starting.
Disposing FtpSocketStream...
TYPE I
226 Transfer complete.
The second file mixes the response of the SIZE and PASV command
SIZE /aav/remotecontrols/32471C51-765F-485D-9B1F-EDDDE71ABD92.xml
200 Type set to I.
PASV
213 17599
A first chance exception of type 'System.Net.FtpClient.FtpException' occurred in FtpClientTest.exe
The program '[5912] FtpClientTest.vshost.exe' has exited with code -1 (0xffffffff).

bp74 wrote Feb 8, 2016 at 9:32 AM

No update on this issue? Meanwhile i've downgraded to version 1.0.5281.14359 which works well.

Btw. if you look at the logging above you can see that the problem starts even before the second file is downloaded. The "TYPE I" command get's the "226 Transfer complete" response which belongs to the preceding "RETR" command.

AlexCovecube wrote Jul 12, 2016 at 8:32 AM

This is not working because the Close / Dispose pattern implemented in FtpSocketStream / FtpDataStream is broken.

The actual issue is that when ControlConnection != null in FtpDataStream, the reply is not read back when that data connection is closed (be cause of the new redefinition of Close()). But in general, the IDisposable pattern is wrong.

To fix this:
  • When inheriting from Stream, do not override Close() and do not implement IDisposable, and do not use "new" to redefine Close().
  • Instead, rely on the fact that in the base Stream class Dispose() will always call Close(), which will always call Dispose(true).
  • So when inheriting from Stream, all you need to do is override Dispose(true) and perform all cleanup code in there. This is where you can also call base.Dispose(disposing), if necessary.
Here is the fixed code:

FtpSocketStream.cs (get rid of Close() and IDiposable)
/// <summary>
/// Dispose of this ftp socket stream.
/// </summary>
protected override void Dispose(bool disposing)
{
    base.Dispose(disposing);

    if (disposing)
    {
        FtpTrace.WriteLine("Closing FtpSocketStream...");

        if (m_socket != null)
        {
            try
            {
                if (m_socket.Connected)
                {
                    ////
                    // Calling Shutdown() with mono causes an
                    // exception if the remote host closed first
                    //m_socket.Shutdown(SocketShutdown.Both);
                    m_socket.Close();
                }

#if !NET2
                m_socket.Dispose();
#endif
            }
            catch (SocketException ex)
            {
                FtpTrace.WriteLine("Caught and discarded a SocketException while cleaning up the Socket: {0}", ex.ToString());
            }
            finally
            {
                m_socket = null;
            }
        }

        if (m_netStream != null)
        {
            try
            {
                m_netStream.Dispose();
            }
            catch (IOException ex)
            {
                FtpTrace.WriteLine("Caught and discarded an IOException while cleaning up the NetworkStream: {0}", ex.ToString());
            }
            finally
            {
                m_netStream = null;
            }
        }

        if (m_sslStream != null)
        {
            try
            {
                m_sslStream.Dispose();
            }
            catch (IOException ex)
            {
                FtpTrace.WriteLine("Caught and discarded an IOException while cleaning up the SslStream: {0}", ex.ToString());
            }
            finally
            {
                m_sslStream = null;
            }
        }
    }
}
FtpDataStream.cs (get rid of Close())
/// <summary>
/// Disconnects (if necessary) and releases associated resources
/// </summary>
/// <param name="disposing">Disposing</param>
protected override void Dispose(bool disposing) {
    var wasConnected = IsConnected;

    base.Dispose(disposing);

    if (disposing) {
        if (wasConnected)
            CloseWithReply();

        m_control = null;
    }
}

/// <summary>
/// Closes the connection and reads the server's reply
/// </summary>
public FtpReply CloseWithReply() {
    try {
        if (ControlConnection != null)
            return ControlConnection.CloseDataStream(this);
    }
    finally {
        m_commandStatus = new FtpReply();
        m_control = null;
    }

    return new FtpReply();
}

ThomasWilliams wrote Oct 18, 2016 at 3:23 PM

Unfortunately still an issue with version 1.0.5824.34026. So if you intend reusing the client for uploading/downloading several files, a "work-around" for me was to set FtpClient.EnableThreadSafeDataConnections = true.

ThomasWilliams wrote Oct 19, 2016 at 2:16 PM

The fix above has proven to solve my issue (checking/uploading multiple files from a multi-threaded client), thanks a lot for the work.