Capabilities & HashAlgorithms can cause sync-over-async

[jnyrup]

FTP OS:
Unix (based on vsFTPd being for unix-like systems)

FTP Server:
vsFTPd 2.2.2 (based on STAT)
Url: ftp://tgftp.nws.noaa.gov

Computer OS:
Windows Server 2019

FluentFTP Version:
33.0.3

While investigating the two stack trace posted below I started wondering why Capabilities and HashAlgorithms contained

if (m_stream == null || !m_stream.IsConnected) {
    Connect();
}

My best guess is to initialize them, if someone calls them before calling Connect or ConnectAsync which initializes them.
I'm not sure why it would be necessary to reconnect to get those values if m_stream was nulled or disconnected, but it can cause a sync-over-async path, VerifyTransferAsync -> ... -> Connect() as seen in the two stack traces.

Without being familiar with the codebase or all the edge cases of ftp connections, I think this might improve the situation.

private List<FtpCapability> m_capabilities;

/// <summary>
/// Gets the server capabilities represented by an array of capability flags
/// </summary>
public List<FtpCapability> Capabilities {
    get {
        if (m_capabilities == null && (m_stream == null || !m_stream.IsConnected)) {
            Connect();
            if (m_capabilities == null) {
                m_capabilities = new List<FtpCapability>();
            }
        }

        return m_capabilities;
    }
    protected set => m_capabilities = value;
}

private FtpHashAlgorithm? m_hashAlgorithms;

/// <summary>
/// Get the hash types supported by the server, if any. This
/// is a recent extension to the protocol that is not fully
/// standardized and is not guaranteed to work. See here for
/// more details:
/// http://tools.ietf.org/html/draft-bryan-ftpext-hash-02
/// </summary>
public FtpHashAlgorithm HashAlgorithms {
    get {
        if (!m_hashAlgorithms.HasValue && (m_stream == null || !m_stream.IsConnected)) {
            Connect();
            if (!m_hashAlgorithms.HasValue) {
                m_hashAlgorithms = FtpHashAlgorithm.NONE;
            }
        }

        return m_hashAlgorithms.Value;
    }
    private set => m_hashAlgorithms = value;
}

Logs :

Not sure if trace logs are relevant here.

System.NullReferenceException: Object reference not set to an instance of an object.
   at FluentFTP.FtpSocketStream.Connect(String host, Int32 port, FtpIpVersion ipVersions)
   at FluentFTP.FtpClient.Connect()
   at FluentFTP.FtpClient.get_Capabilities()
   at FluentFTP.FtpClient.SupportsChecksum()
   at FluentFTP.FtpClient.<VerifyTransferAsync>d__121.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpClient.<DownloadFileToFileAsync>d__56.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpClient.<DownloadFileAsync>d__55.MoveNext()

System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'System.Net.Sockets.Socket'.
   at System.Net.Sockets.Socket.InternalEndConnect(IAsyncResult asyncResult)
   at System.Net.Sockets.Socket.EndConnect(IAsyncResult asyncResult)
   at FluentFTP.FtpSocketStream.Connect(String host, Int32 port, FtpIpVersion ipVersions)
   at FluentFTP.FtpClient.Connect()
   at FluentFTP.FtpClient.get_Capabilities()
   at FluentFTP.FtpClient.SupportsChecksum()
   at FluentFTP.FtpClient.<VerifyTransferAsync>d__121.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpClient.<DownloadFileToFileAsync>d__56.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpClient.<DownloadFileAsync>d__55.MoveNext()

[robinrodricks]

Very good find! I'll try fixing it and let you know.

[robinrodricks]

Fixed. Works for you?

https://www.nuget.org/packages/FluentFTP/33.1.2

[robinrodricks]

It will now throw an exception if you attempt to do any operation without Connect()ing first.

[jnyrup]

Hi Robin, thanks for the release.
I'll update the package and see how it behaves.

By looking at fc22c61 I still foresee a possible problematic scenario (I haven't tested it)

var client = new FtpClient();
client.Connect();
client.Disconnect(); // <-- let's say this happens implicitly somewhere
var capabilities = client.Capabilities; // throws an exception even though the data was populated due the explicit call to Connect()

[robinrodricks]

I've completely disabled calling Connect() within Capabilities & HashAlgorithms.

[jnyrup]

I see that an FtpException is now thrown instead of calling Connect().
I'm just unsure why it would want to throw an exception if the capabilities has already been retrieved from an earlier call to Connect(), i.e. m_capabilities has been populated with real data.

[robinrodricks]

What if you disconnected and changed the server info and reconnected? Then it would have outdated info.

[jnyrup]

Then the new call to Connect() overwrites the value from the previous server.

[robinrodricks]

Okay, we can do a check. If m_capabilities has been populated with real data, we can return unconditionally if you're connected or not. That should work right?

[jnyrup]

I would say so.
In the example above I used FtpHashAlgorithm? to distinguish between the two cases:

• Not having called Connect() yet =>m_hashAlgorithms is null
• Connect() has been called, but m_hashAlgorithms is FtpHashAlgorithm.None

I agree that is doesn't make much sense from a user perspective to explicitly disconnect and then and for capabilities, but the disconnect might happen from within the library which gives the strange stack trace.

[robinrodricks]

Done. Please verify.

https://www.nuget.org/packages/FluentFTP/33.1.3

[FanDjango]

Not good enough!

If client.Config.CheckCapabilities is set to false, this should behave differently. GetListing(), for example, checks for capability MLST, which get this failure exception and shouldn't really.

I agree not to call Connect in Capabilites and Hash_Algorithms - although with the current, much improved Async capabilities one could call a async path just as it is done in Execute() (the original issue was that the async path would call a sync connect, remember?). I would prefer that in Execute() would be the only place for a (re-)connect to avoid code duplication.

I would like to cross-reference here to issue #1698 - where the exception message "Please call connect.....before reading capabilites" is causing some confusion in relation to auto-connect and auto-reconnect.