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

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

Comments

Projects
None yet
3 participants
@jblazquez
Contributor

jblazquez commented Jun 22, 2018

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

This comment has been minimized.

Show comment
Hide comment
@jeroen

jeroen Jun 22, 2018

Contributor

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

Contributor

jeroen commented Jun 22, 2018

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

@bagder bagder added the crash label Jun 22, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jun 22, 2018

Member

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...

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

This comment has been minimized.

Show comment
Hide comment
@jblazquez

jblazquez Jun 23, 2018

Contributor

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

Contributor

jblazquez commented Jun 23, 2018

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

@bagder bagder closed this in 4c90163 Jun 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment