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

Jump to bottom

Segmentation fault when removing easy handle that was on the pending connections list #2677

Closed
jblazquez opened this issue Jun 22, 2018 · 3 comments
Closed

Segmentation fault when removing easy handle that was on the pending connections list #2677

jblazquez opened this issue Jun 22, 2018 · 3 comments
Labels
crash

Comments

@jblazquez
Copy link
Contributor

The following code reliably results in a segfault in curl 7.60.0 on Linux (Linux 4.15.0-23-generic #25-Ubuntu SMP Wed May 23 18:02:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux):

#include <curl/curl.h>

int main()
{
    curl_global_init(CURL_GLOBAL_ALL);

    CURLM* multi = curl_multi_init();
    curl_multi_setopt(multi, CURLMOPT_MAX_HOST_CONNECTIONS, 1);

    CURL* handle1 = curl_easy_init();
    curl_easy_setopt(handle1, CURLOPT_URL, "http://www.example.com/");
    curl_multi_add_handle(multi, handle1);

    CURL* handle2 = curl_easy_init();
    curl_easy_setopt(handle2, CURLOPT_URL, "http://www.example.com/");
    curl_multi_add_handle(multi, handle2);

    int running = 0;
    curl_multi_perform(multi, &running);

    curl_multi_remove_handle(multi, handle2);
    curl_easy_cleanup(handle2);
    curl_multi_remove_handle(multi, handle1);
    curl_easy_cleanup(handle1);

    return 0;
}

Steps to reproduce on Linux:

$ curl -L https://github.com/curl/curl/releases/download/curl-7_60_0/curl-7.60.0.tar.gz | tar xvz
$ cd curl-7.60.0/
$ ./configure && make
$ gcc -o test test.c -Llib/.libs -lcurl
$ LD_LIBRARY_PATH=lib/.libs gdb ./test
(gdb) run
Starting program: test 
Thread 1 "test" received signal SIGSEGV, Segmentation fault.
0x00007ffff7ba0c34 in process_pending_handles () from lib/.libs/libcurl.so.4
(gdb) bt
#0  0x00007ffff7ba0c34 in process_pending_handles () from lib/.libs/libcurl.so.4
#1  0x00007ffff7ba0d60 in multi_done () from lib/.libs/libcurl.so.4
#2  0x00007ffff7ba1248 in curl_multi_remove_handle () from lib/.libs/libcurl.so.4
#3  0x0000555555554b46 in main ()

The problems appears to occur because of the following:

  • The multi handle is configured to create at most 1 connection to a host.
  • Two easy handles are created to make a request to the same host.
  • The first handle grabs a connection and starts processing the request.
  • The second handle reaches the CURLM_STATE_CONNECT state but has no connections available, so it's added to the multi handle's list of connect-pending handles.
  • Before the first request completes, the second handle is removed from the multi handle. This doesn't remove the easy handle from the pending list. The handle is then freed, which leaves a freed entry on the pending list.
  • The first handle is removed from the multi handle. Because it has an associated connection, multi_done is called, which in turn calls process_pending_handles to dequeue the next handle waiting for a connection, segfaulting by accessing a freed entry.

Adding the following code to remove a handle from the connect-pending list in all cases seems to fix the issue:

*** a/multi.c	2018-05-07 02:18:03.000000000 -0700
--- b/multi.c	2018-06-21 17:31:56.248788436 -0700
***************
*** 698,703 ****
--- 698,708 ----
        Curl_getoff_all_pipelines(data, data->easy_conn);
    }
  
+   if(data->connect_queue.ptr)
+     /* the handle was on the pending list waiting for an available connection,
+        so go ahead and remove it */
+     Curl_llist_remove(&multi->pending, &data->connect_queue, NULL);
+ 
    if(data->dns.hostcachetype == HCACHE_MULTI) {
      /* stop using the multi handle's DNS cache, *after* the possible
         multi_done() call above */
@jeroen
Copy link
Contributor

jeroen commented Jun 22, 2018 •
edited

edit: scratch my earlier comment. This problem persists even in #2675

@bagder bagder added the crash label Jun 22, 2018
@bagder
Copy link
Member

bagder commented Jun 22, 2018

remove a handle from the connect-pending list in all cases

Thanks @jblazquez, that certainly sounds like the correct fix as the pending list certainly should not contain any handles that have been removed...

@jblazquez jblazquez mentioned this issue Jun 23, 2018
Closed
@jblazquez
Copy link
Contributor Author

Thanks Daniel, I submitted a pull request with the fix ^

@bagder bagder closed this as completed in 4c90163 Jun 23, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
crash
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bagder @jeroen @jblazquez