FTP connection reuse not working in libcurl 8.12.1

[Zenju]

I did this

1. Standard TLS-encypted connection to Apache FTP server.

2. Multiple FTP accesses using ::curl_easy_reset() on the same curl easy handle

=> TLS-enabled FTP control connection is not reused, see Wireshark log:

Analysis so far:

Problem is caused by:

  if((!(needle->handler->flags&PROTOPT_SSL) !=
      !Curl_conn_is_ssl(conn, FIRSTSOCKET)) &&
     !(get_protocol_family(conn->handler) == needle->handler->protocol &&
       conn->bits.tls_upgraded))
    /* Deny `conn` if it is not fit for `needle`'s SSL needs,
     * UNLESS `conn` is the same protocol family and was upgraded to SSL. */
      return FALSE;

https://github.com/icing/curl/blob/3be33a1a4777438e2ef9cca488322f789bdd40fd/lib/url.c#L955

In libcurl 8.11.1 this line was

  if ((needle->handler->flags&PROTOPT_SSL) !=
        (conn->handler->flags&PROTOPT_SSL))
        /* do not do mixed SSL and non-SSL connections */
        if (get_protocol_family(conn->handler) !=
            needle->handler->protocol || !conn->bits.tls_upgraded)
            /* except protocols that have been upgraded via TLS */
            return FALSE;

In the 8.12.1 code
Curl_conn_is_ssl(conn, FIRSTSOCKET) returns "true",

while in 8.11.1
(conn->handler->flags&PROTOPT_SSL) evaluated to "false".

Reverting this code section back to 8.11.1 fixed FTP connection reuse in my tests.

I expected the following

FTP control connection should be reused instead of needlessly creating a new one each time

curl/libcurl version

8.12.1

operating system

Windows 10

[Zenju]

Related: #16034

[bagder]

Multiple FTP accesses using ::curl_easy_reset() on the same curl easy handle

Does it work correctly without the reset done?

[Zenju]

Does it work correctly without the reset done?

I don't understand the question. How would you make multiple FTP accesses without using curl_easy_reset(), while reusing the same connection?

[bagder]

Just use the same handle again?

[Zenju]

The program crashes in ftp.c ftp_state_quote.

curl_easy_reset() is required for my use case as the FTP accesses vary a lot: file/folder creates, updates, deletes, folder traversal, lots of CURLOPT_CUSTOMREQUEST. On the other hand, not having connection reuse in libcurl is a showstopper bug.

[bagder]

The program crashes in ftp.c ftp_state_quote.

crashes ? That sounds like a separate bug then. Or you took away the list from under libcurl's feet.

[Zenju]

I have no idea. Just commenting out "curl_easy_reset" quick and dirty. The libcurl access pattern in my software FreeFileSync is complex.

[icing]

Thanks for the report. You pointed exactly at the location that was the problem.

I added checks in out test suite, reproducing the problem, and rewrote the logic in url.c again in #16392.

It would be great if you could verify that this solves your problem. Thanks.

[Zenju]

I added checks in out test suite, reproducing the problem, and rewrote the logic in url.c again in #16392.

It would be great if you could verify that this solves your problem. Thanks.

Yes, this works! The TLS-enabled control connection is being reused as expected!

[Zenju]

I'm wondering: Is FTP "AUTH TLS" considered an TLS-upgraded connection?

If "yes", then adding

        conn->bits.tls_upgraded = true;

after

        result = Curl_pp_sendf(data, &ftpc->pp, "AUTH %s",
                               ftpauth[ftpc->count1]);
        if(!result)
          ftp_state(data, FTP_AUTH);

https://github.com/curl/curl/blob/2335cbaa217af072b58b87911191acac8dbc7092/lib/ftp.c#L2727C1-L2727C2

would also fix the caching bug.

[icing]

I am not aware of what the "caching bug" is, but I am on a mission to eliminate bits.tls_upgraded completely. It had been the only way to track if the connection speaks SSL, but we now have a better way of checking that, namely if an SSL filter is in place at the connection or not.

The only place left where bits.tls_upgraded is being evaluated is url.c line 1101 and that looks very close to the check we are currently fixing. Hmm...

[Zenju]

I am not aware of what the "caching bug" is,

I mean what this ticket is about.

but I am on a mission to eliminate bits.tls_upgraded complete.

Ok, cool. Either way this showstopper bug is fixed for me. So thanks a lot!

[icing]

I am not aware of what the "caching bug" is,

I mean what this ticket is about.

but I am on a mission to eliminate bits.tls_upgraded complete.

Ok, cool. Either way this showstopper bug is fixed for me. So thanks a lot!

Just pushed to #16392 and the bits.tls_upgraded is gone. Added tests with ftps: urls. If you could throw that one in your grinder, would appreciate it.

[Zenju]

Just pushed to #16392 and the bits.tls_upgraded is gone. Added tests with ftps: urls. If you could throw that one in your grinder, would appreciate it.

Looking good! Works correctly with connection channel caching while traversing a large folder via FTP both with and without TLS enabled.