Crash in some use case using shared handle with connection sharing in 8.18 and above

[Dmitry Karpov via curl-library]

Hi All,

I discovered a crash case scenario with shared handle with connection sharing  
when trying libcurl 8.18.0 in my application.

The crash case can be described as follows:
- client create a share handle to share connections.
- client creates a multi handle.
- client creates an easy handle.
- client sets a share handle in the easy handle.
- client adds the easy handle to the multi handle.
- client performs the transfer via the multi-handle until it gets some data.
  NOTE: The download must be long enough, so we break the transfer before it is 
fully done.

- client sets a null share to the easy handle.
- client removes the easy handle from the multi handle (via 
curl_multi_remove_handle) - CRASH!

In the debug build the crash happens because of the assert in the 
cpool_remove_conn() function in \lib\conncache.c:

static void cpool_remove_conn(struct cpool *cpool,
                              struct connectdata *conn)
{
  ....
    else {
      /* Should have been in the bundle list */
      DEBUGASSERT(NULL); // Crash happens here!!!
    }
  }
}

In the release build it happens at some point after the assertion is ignored 
when trying to remove non-existing connection from the multi-handle connection 
pool.

The reason for the assertion/crash is that when we set a null share to the easy 
handle before calling the curl_multi_remove_handle(),
then cpool_get_instance() returns the connection pool for the multi-handle 
while the easy handle keep the connection data from the shared handle 
connection pool.

And when the curl_multi_remove_handle() tries to remove the connection data 
from the easy handle, it uses the connection pool from the multi-handle, not 
from the share and asserts/crashes.

A possible logical fix for this issue would be detaching the connection data 
from the easy handle when a new share handle is set, similar to the other easy 
settings, like:

\lib\setopt.c:

  case CURLOPT_SHARE: {
     ...
      if(data->share->specifier & (1 << CURL_LOCK_DATA_DNS)) {
        Curl_resolv_unlink(data, &data->state.dns[0]);
        Curl_resolv_unlink(data, &data->state.dns[1]);
      }

      /* Detaching the connection if the easy handle was using connection 
sharing */
      if (data->share->specifier & (1 << CURL_LOCK_DATA_CONNECT))
        Curl_detach_connection(data);
        ...
    }

It worked in my test, but maybe there is a better solution.

Here is some small example program that can help to reproduce the crash:

static size_t writeFunc(void* ptr, size_t size, size_t nmemb,
    void* data) {
    (void)ptr;

    bool* data_received = (bool*)data;
    *data_received = true;
    return size * nmemb;
}

void setNullShareRemoveTest() {
    // Creating share with connection sharing option.
    CURLSH* share = curl_share_init();
    curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);

    // Creating an easy handle and set share and other options.
    CURL* easy = curl_easy_init();

    curl_easy_setopt(easy, CURLOPT_SHARE, share);

    // The URL should be for a long enough download, so the transfer is not
    // completed when the first data chunk is delivered to the write function.
    curl_easy_setopt(easy, CURLOPT_URL, "http://example.com/file_1MB.bin";);

    bool data_received = false;

    curl_easy_setopt(easy, CURLOPT_WRITEFUNCTION, writeFunc);
    curl_easy_setopt(easy, CURLOPT_WRITEDATA, (void*)&data_received);

    curl_easy_setopt(easy, CURLOPT_VERBOSE, 1L);

    // Creating a multi handle.
    CURLM* multi = curl_multi_init();

    // Run transfer
    curl_multi_add_handle(multi, easy);

    int still_running = 0;

    for (;;) {
        CURLMcode mresult = curl_multi_perform(multi, &still_running);
        if (mresult != CURLM_OK) {
            printf("curl_multi_perform() failed, code %d.\n",
                (int)mresult);
            break;
        }

        if (!still_running || data_received) {
            break; // Break when first data chunk is received or transfer is 
done.
        }

        /* wait for activity, timeout or "nothing" */
        mresult = curl_multi_poll(multi, NULL, 0, 1000, NULL);
        if (mresult != CURLM_OK) {
            printf("curl_multi_poll() failed, code %d.\n", (int)mresult);
            break;
        }
    } /* if there are still transfers, loop */

    // Set a null share first.
    curl_easy_setopt(easy, CURLOPT_SHARE, NULL);

    // Remove the easy handle after clearing the share. !!! Crash!!!
    curl_multi_remove_handle(multi, easy);

// cleanup:
    curl_easy_cleanup(easy);
    curl_multi_cleanup(multi);
    curl_share_cleanup(share);

    printf("\nDone\n");
}

Thanks,
Dmitry Karpov





-- 
Unsubscribe: https://lists.haxx.se/mailman/listinfo/curl-library
Etiquette:   https://curl.se/mail/etiquette.html