Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection not cleanly closed with CURLOPT_CONNECT_ONLY #9335

Closed
Thalhammer opened this issue Aug 19, 2022 · 7 comments
Closed

Connection not cleanly closed with CURLOPT_CONNECT_ONLY #9335

Thalhammer opened this issue Aug 19, 2022 · 7 comments

Comments

@Thalhammer
Copy link

Thalhammer commented Aug 19, 2022

I did this

My intention is to use a combination of CURLOPT_CONNECT_ONLY, curl_multi_* and curl_easy_send/curl_easy_recv to implement a custom protocol while leveraging the proxy and ssl handling of libcurl (together with the libcurl multi workers I already have for async http). Despite many warnings and false claims (e.g. curl_waitfd.revents has a unsupported comment, despite being supported on at least windows and posix) this works fine in my case.

However I think I found a issue when it comes to closing connections, which might be related to #4426 but I am not 100% sure.
When I open a connection with CURLOPT_CONNECT_ONLY it seems to outlive the curl_easy_close call despite being removed from all multi handles. Looking at a wireshark capture it seems like the connection is keep alive until the multi handle it was associated with gets closed.

Another issue is that even though the connection is closed after the multi handle is gone, its not close cleanly but rather just reset, which (while usually not a problem) is not a clean tcp shutdown.

Another thing of note is that if I use ssl encryption everything seems to work correctly.

I expected the following

The connection should get closed cleanly using a fin packet as soon as I dispose of the curl_easy handle.

curl/libcurl version

libcurl 7.74.0 built using CMAKE with HTTP_ONLY=ON CURL_DISABLE_CRYPTO_AUTH=ON.

operating system

Linux <redacted> 5.15.0-46-generic #49~20.04.1-Ubuntu SMP Thu Aug 4 19:15:44 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

EDIT: If I try to open a second connection to any other host after closing the first handle.has been removed for a while, then curl will actually notice that the first connection has no handle anymore and close it (still not cleanly).
EDIT2: A workaround seems to be to use the synchronous curl_easy_perform method to establish the connection and never call curl_multi_add_handle for the handle, in which case the connection is closed as soon as I cleanup the handle (still not cleanly though). I assume libcurl creates an implicit multi handle inside and drops it along with the easy handle, causing the connection to get closed. This however is not really good workaround cause the connection establishment is now synchronous.

@bagder
Copy link
Member

bagder commented Aug 19, 2022

I think we can do better for this use case (I believe what you describe might still happen in a modern libcurl version).

Let me just mention that libcurl cannot decide to deliberately do a "clean" or a "not clean" close, it simply closes the socket and sending a FIN or RST is something that the TCP/IP stack in the kernel decides on. Not libcurl.

@Thalhammer
Copy link
Author

Thalhammer commented Aug 20, 2022

Let me just mention that libcurl cannot decide to deliberately do a "clean" or a "not clean" close

I dont think thats completely true. It's been a while since I last did raw sockets, but if my memory serves me right you are supposed to invoke shutdown() on the socket to signal to the kernel that you want to close the connection. This will cause the kernel to send a FIN packet to the remote host which in turn flushes its buffers and sends a FIN as well. close immediately closes the socket and if the kernel does this closing dance its more a happy coincidence than guaranteed. You can read more about that here.

However in most cases the unclean shutdown isn't really an issue cause if you close the socket you probably dont care about data in the remotes buffers anyway. The bigger issue is that you can't tell libcurl to immediately close the connection without resorting to ugly hacks. In the ideal case multi would behave the same as easy except async.

@bagder
Copy link
Member

bagder commented Aug 20, 2022

Thanks for educating me on TCP, but this is not a problem of using or not using shutdown.

@bagder
Copy link
Member

bagder commented Aug 20, 2022

Do you happen to have a (small) example code that reproduces this issue?

@Thalhammer
Copy link
Author

Thalhammer commented Aug 21, 2022

Thanks for educating me on TCP, but this is not a problem of using or not using shutdown.

I by no means wanted to overbearing, but I have experienced that most people dont care what happens on the lower layers.

Do you happen to have a (small) example code that reproduces this issue?

Of course, I wrote a small one up. It uses the tcpbin.com echo service, but its not really important whats the target server, as long as it accepts tcp connections and doesn't immediately close them. You can toggle USE_MULTI to trigger the issue. With multi handles disabled everything works fine and the lsofcommand reports no active connections. With multi handles enabled the connection is not closed by the curl_easy_cleanup() call but lives on until the multi handle gets cleaned.

#include <curl/curl.h>  // For curl*
#include <stdio.h>      // For sprintf, printf
#include <stdlib.h>     // For system()
#include <unistd.h>     // For getpid()

#define USE_MULTI 1

int main() {
  CURL* hdl = curl_easy_init();
  curl_easy_setopt(hdl, CURLOPT_VERBOSE, (long)1);
  curl_easy_setopt(hdl, CURLOPT_CONNECT_ONLY, (long)1);
  curl_easy_setopt(hdl, CURLOPT_URL, "http://tcpbin.com:4242");

#if USE_MULTI
  CURLM* multi = curl_multi_init();
  curl_multi_add_handle(multi, hdl);

  CURLMsg* msg=NULL;
  int num= 0;
  while(!(msg = curl_multi_info_read(multi, &num)) || msg->msg != CURLMSG_DONE) {
      curl_multi_perform(multi, &num);
      curl_multi_wait(multi, NULL, 0, 100, NULL);
  }
  if(msg->data.result == CURLE_OK)
      printf("Connected\n");
  
  curl_multi_remove_handle(multi, hdl);
#else
  if(curl_easy_perform(hdl) == CURLE_OK)
      printf("Connected\n");
#endif

  // Connection should get closed here, but it isn't
  curl_easy_cleanup(hdl);

  // List active connections, will contain a handle
  char cmd_list_conns[128];
  sprintf(cmd_list_conns, "lsof -ai -p %d", (int)getpid());
  system(cmd_list_conns);

#if USE_MULTI
  // Even keeping the multi driven doesnt change anything
  for(int i =0; i<10; i++) {
      curl_multi_perform(multi, &num);
      curl_multi_wait(multi, NULL, 0, 100, NULL);
  }

  // Connection is still active here
  system(cmd_list_conns);

  // Connection only closed after the multi handle is cleaned
  curl_multi_cleanup(multi);
#endif
}

@bagder
Copy link
Member

bagder commented Aug 22, 2022

Thanks for the reproducer, it helped me work on this!

This behavior/bug happens because when we remove the easy handle from the multi handle, the association between the easy handle and the connection is cut and the connection is left in the connection cache (marked as cannot-be-reused). The connection cache is held in the multi handle.

When curl_easy_cleanup() is then called, it has no connection to close. It doesn't even know in which multi handle it was once added because it is blanked (and in fact the multi handle could be gone completely). The cleanup function cannot close the connection.

The fix is probably to make curl_multi_remove_handle() close the connection if CONNECT_ONLY was set, as there is no way to reach/use that connection again after the function is called.

PR pending.

bagder added a commit that referenced this issue Aug 22, 2022
Ẃhen it has been used in the multi interface, it is otherwise left in
the connection cache, can't be reused and nothing will close them since
the easy handle loses the association with the multi handle and thus the
connection cache - until the multi handle is closed or it gets pruned
because the cache is full.

Reported-by: Dominik Thalhammer
Fixes #9335
@bagder
Copy link
Member

bagder commented Aug 22, 2022

#9342 fixes the problem in my tests

@bagder bagder closed this as completed in 31a41d4 Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants