Double-free when using HTTPS with tunneling proxy

[sagebind]

I did this

Run a program using libcurl such as the following example:

#include <curl/curl.h>

int main() {
    curl_global_init(0); // for posterity
    printf("curl version: %s\n", curl_version());

    CURLM *multi = curl_multi_init();
    CURL *easy = curl_easy_init();

    // Crash only happens when using HTTPS.
    curl_easy_setopt(easy, CURLOPT_URL, "https://example.org");
    // Any old HTTP tunneling proxy will do here.
    curl_easy_setopt(easy, CURLOPT_PROXY, "http://127.0.0.1:9000");

    // We're going to drive the transfer using multi interface here, because we
    // want to stop during the middle.
    curl_multi_add_handle(multi, easy);

    // Run the multi handle once, just enough to start establishing an HTTPS
    // connection.
    int running_handles;
    curl_multi_perform(multi, &running_handles);

    // Close the easy handle *before* the multi handle. Doing it the other way
    // around avoids the issue.
    curl_easy_cleanup(easy);

    curl_multi_cleanup(multi); // double-free happens here
}

The program crashes with the following output:

curl version: libcurl/7.80.0-DEV OpenSSL/1.1.1f zlib/1.2.11 nghttp2/1.40.0
free(): double free detected in tcache 2
fish: './a.out' terminated by signal SIGABRT (Abort)

When running the example program through a debugger the program appears to crash at this line:

curl/lib/url.c

Line 793 in 9db25d2

Curl_safefree(conn->connect_state);

Relevant stack trace from my debugger:

conn_free
Curl_disconnect
Curl_conncache_close_all_connections
curl_multi_cleanup

I'm not totally sure if this is specifically related to proxies, as I can only reproduce when nghttp2 is enabled and using HTTPS and using a proxy, but @iiibui who originally brought the issue to my attention could reproduce with other scenarios as well.

I believe in all scenarios, rearranging the cleanup calls to close the multi handle first and then the easy handle avoids the issue. The documentation for curl_multi_cleanup definitely recommends doing it in the order that I used, but it is unclear how important the order is. Is it a suggestion? Is it undefined behavior to cleanup in the reverse order? The docs don't say. I'm also not calling curl_multi_remove_handle at all, but it seems that it is called automatically by curl_easy_cleanup:

curl/lib/url.c

Lines 378 to 382 in 9db25d2

	m = data->multi;
	if(m)
	/* This handle is still part of a multi handle, take care of this first
	and detach this handle from there. */
	curl_multi_remove_handle(data->multi, data);

Note this also seems similar to #7236, which was supposedly fixed.

I expected the following

The program should exit without error.

• If calling curl_easy_cleanup on a handle without calling curl_multi_remove_handle first is undefined behavior, then I feel like the docs should say this more explicitly. The source code seems to expect people to not do this.
• If it is OK to call curl_easy_cleanup without calling curl_multi_remove_handle, then this is bug introduced in 51c0ebc. Everything works just fine without calling curl_multi_remove_handle on versions prior to that commit that I tested.
  • If it is OK to do this, then I feel the docs should also be more clear as to whether it matters if curl_easy_cleanup is called before curl_multi_cleanup or not. The phrase "should be" seems a bit non-committal. What happens if I do it out-of-order?

I help maintain the Rust bindings for libcurl, so ultimately I just want to know what the rules are for these calls so we can enforce them in the bindings.

curl/libcurl version

I can reproduce this on the latest master commit (9db25d2 as of writing) but using git bisect I identified 51c0ebc as the offending commit where this issue started.

I am configuring curl as follows:

./configure --with-openssl --with-nghttp2

operating system

Windows 10 under WSL2 (Ubuntu-flavored), however the original reporter of the issue reproduced on CentOS 7.3.1611.

[jay]

Note for the repro that on Windows curl_global_init must use win32 flag (unless manually initializing winsock), suggest changing to CURL_GLOBAL_DEFAULT.

The first free is here when curl_easy_cleanup -> Curl_free_request_state is called:

curl/lib/url.c

Lines 2198 to 2205 in 9e560d1

	/*
	* Curl_free_request_state() should free temp data that was allocated in the
	* Curl_easy for this single request.
	*/
	void Curl_free_request_state(struct Curl_easy *data)
	{
	Curl_safefree(data->req.p.http);

data->req.p.http == conn->connect_state which is probably the problem

Then I get an access violation when curl_multi_cleanup -> conn_shutdown is called and it tries to dereference conn->connect_state->prot_save:

curl/lib/url.c

Lines 739 to 746 in 9e560d1

	#ifndef USE_HYPER
	if(conn->connect_state && conn->connect_state->prot_save) {
	/* If this was closed with a CONNECT in progress, cleanup this temporary
	struct arrangement */
	data->req.p.http = NULL;
	Curl_safefree(conn->connect_state->prot_save);
	}
	#endif

[bagder]

If calling curl_easy_cleanup on a handle without calling curl_multi_remove_handle first is undefined behavior, then I feel like the docs should say this more explicitly

I think it should be discouraged, even if I also think that the code at least once tried to make sure that it would handle it. I'll make a PR and propose this addition to the curl_easy_cleanup man page:

   To  close  an  easy  handle that has been used with the multi interface,
   make sure to call curl_multi_remove_handle(3) first to  remove  it  from
   the multi handle before it is closed.

It should of course still not cause a crash.

[bagder]

I've managed to convert @sagebind's code into a new libcurl test case that reproduces this problem. Working on a fix.

[bagder]

#7986 fixes this issue in my tests

[jay]

#7986 fixes this issue in my tests

Fixes it for me as well, no more access violation in Windows