curl::multi::Multi will make double free memory problem when some ssl error occurred

[iiibui]

Problem

OS: CentOS Linux release 7.3.1611 (Core)

Use curl::multi::Multi to fetch more than one https url will cause the program to crash when some ssl error occurred(rel issue):

[root@hugo-devm-b4pc3 easycurl]# cargo run
   Compiling easycurl v0.1.0 (/mnt/vdc1/home/apps/easycurl)
    Finished dev [unoptimized + debuginfo] target(s) in 0.58s
     Running `target/debug/easycurl`
result for easy with token 0: Some(Err(Error { description: "SSL peer certificate or SSH remote key was not OK", code: 60, extra: Some("SSL: certificate subject name 'Test' does not match target host name '127.0.0.1'") }))
*** Error in `target/debug/easycurl': double free or corruption (!prev): 0x00007f74f921f4e0 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7c503)[0x7f74f6dec503]
/apps/svr/curl/lib/libcurl.so.4(+0x57d08)[0x7f74f7cc9d08]
/apps/svr/curl/lib/libcurl.so.4(+0x58c3c)[0x7f74f7ccac3c]
/apps/svr/curl/lib/libcurl.so.4(+0x1132c)[0x7f74f7c8332c]
/apps/svr/curl/lib/libcurl.so.4(curl_multi_cleanup+0xbf)[0x7f74f7cafbff]
target/debug/easycurl(_ZN63_$LT$curl..multi..RawMulti$u20$as$u20$core..ops..drop..Drop$GT$4drop17h76af921506dcf405E+0x12)[0x7f74f87a6a22]
target/debug/easycurl(_ZN4core3ptr42drop_in_place$LT$curl..multi..RawMulti$GT$17h3353ba6689bfbe2eE+0xb)[0x7f74f87a4f0b]
target/debug/easycurl(_ZN5alloc4sync12Arc$LT$T$GT$9drop_slow17h56cde95cf342e66dE+0x24)[0x7f74f87a2de4]
target/debug/easycurl(_ZN67_$LT$alloc..sync..Arc$LT$T$GT$$u20$as$u20$core..ops..drop..Drop$GT$4drop17hb73c8a2d4d00a004E+0x63)[0x7f74f87a2f43]
target/debug/easycurl(_ZN4core3ptr66drop_in_place$LT$alloc..sync..Arc$LT$curl..multi..RawMulti$GT$$GT$17he1315d4334cfd101E+0xb)[0x7f74f87a573b]
target/debug/easycurl(+0x24657)[0x7f74f879b657]
target/debug/easycurl(+0x2565f)[0x7f74f879c65f]
target/debug/easycurl(+0x245db)[0x7f74f879b5db]
target/debug/easycurl(+0x25e6e)[0x7f74f879ce6e]
target/debug/easycurl(+0x26ba1)[0x7f74f879dba1]
target/debug/easycurl(_ZN3std2rt19lang_start_internal17h571831ebdba142deE+0x431)[0x7f74f87d4e81]
target/debug/easycurl(+0x26b70)[0x7f74f879db70]
target/debug/easycurl(+0x25c4c)[0x7f74f879cc4c]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f74f6d91b35]
target/debug/easycurl(+0x23d69)[0x7f74f879ad69]

Rust code

///
/// Simulate cargo download_accessible to reproduce crash issue https://github.com/rust-lang/cargo/issues/10034
///
use curl::easy::{Easy, HttpVersion};
use curl::multi::{EasyHandle, Multi};
use curl::Error as CurlError;

fn main() {
    let ret = {
        let mut multi = Multi::new();
        multi.pipelining(false, true).unwrap();
        multi.set_max_host_connections(2).unwrap();
        let mut handles = Vec::new();
        handles.push(add_easy_to_multi(&mut multi, handles.len()));
        // push more than one EasyHandle to trigger crash
        handles.push(add_easy_to_multi(&mut multi, handles.len()));
        let (token, result) = wait_for_curl(&mut multi, &handles);
        let handle = handles.remove(token);
        multi.remove(handle).unwrap();
        /*
        // comment out this loop it won't crash
        while !handles.is_empty() {
            let handle = handles.pop().unwrap();
            multi.remove(handle).unwrap();
        }
         */
        result
    };
    {
        println!("wait for url: {:?}", ret);
    }
}

fn add_easy_to_multi(multi: &mut Multi, token: usize) -> EasyHandle {
    let mut easy = Easy::new();
    easy.get(true).unwrap();
    easy.url("https://127.0.0.1:443").unwrap();
    easy.follow_location(true).unwrap();
    easy.http_version(HttpVersion::V2).unwrap();
    easy.pipewait(true).unwrap();
    easy.ssl_verify_peer(false).unwrap(); // comment out this line it won't crash
    easy.write_function(move |data| {
        println!("easy with token {} read {} bytes", token, data.len());
        Ok(data.len())
    }).unwrap();

    let mut h = multi.add(easy).unwrap();
    h.set_token(token).unwrap();
    h
}

fn wait_for_curl(multi: &mut Multi, handles: &Vec<EasyHandle>) -> (usize, Option<Result<(), CurlError>>) {
    let mut e = None;
    let mut out_token = 0usize;
    loop {
        multi.perform().unwrap();
        multi.messages(|msg| {
            let token = msg.token().unwrap();
            let r = msg.result_for(&handles[token]);
            println!("result for easy with token {}: {:?}", token, r);
            e = r;
            out_token = token;
        });
        if e.is_some() {
            break;
        }
        multi.wait(&mut [], std::time::Duration::from_secs(5)).unwrap();
    }
    (out_token, e)
}

The url https://127.0.0.1:433 in the code is a service implemented by the simple golang code:

package main

import (
    "fmt"
    "net/http"
)

func handler(w http.ResponseWriter, r *http.Request) {
    fmt.Fprintf(w, "It works!")
}

func main() {
    http.HandleFunc("/", handler)
    http.ListenAndServeTLS(":443", "server.crt", "server.key", nil)
}

server.crt is a self signed certificate generated by OpenSSL, you can replace with your version.

Cargo.toml

[package]
name = "easycurl"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
curl = { version = "0.4.40", features = ["http2"] }
curl-sys = "0.4.50"

curl -V:

libcurl/7.80.0-DEV OpenSSL/1.1.1h zlib/1.2.7 libidn2/2.3.1 nghttp2/1.41.0

[ehuss]

Can you say how you got or installed libcurl 7.80.0-DEV?

[iiibui]

Can you say how you got or installed libcurl 7.80.0-DEV?

Build from source:

git clone --depth=1 https://github.com/curl/curl.git
cd curl
autoreconf -fi
./configure --prefix=/apps/svr/curl --with-openssl=/apps/svr/openssl --with-nghttp2=/apps/svr/nghttp2
make && make install

I found that if remove the http2 support, it won't crash, and also I can't reproduce this problem on my Mac and Windows, maybe I should compare the code of libcurl-7.73.0 and libcurl 7.80.0-DEV.
Any way, I think it's better to remove all EasyHandle from Multi before Multi cleaning up(by curl_multi_cleanup).

[sagebind]

You're right, it does seem that the curl docs recommend removing all easy handles from a multi handle first before freeing the multi handle: https://curl.se/libcurl/c/curl_multi_cleanup.html. Though it is unclear if this is related to this issue or not, or if the way we're currently doing it is strictly incorrect.

I also wouldn't rule out the possibility that this is an upstream issue in either curl or OpenSSL.

I'll see if I am able to reproduce and if so, whether removing all handles first corrects the problem.

[sagebind]

So far I am not able to reproduce this on my local machine or inside a Centos 7.3.1611 Docker container. Are you also building OpenSSL and nghttp2 yourself? I see you are pointing to nonstandard paths for those. It would be helpful to see how you are building those.

It might also help to strip out the superfluous parts in your example program, I've rewritten it down to the following. Could you verify that the below program also crashes for you with a double-free? I tried to isolate specifically the scenario you described (error from OpenSSL + dropping multi handle without removing easy handles):

use curl::{
    easy::{Easy, HttpVersion},
    multi::Multi,
};
use std::time::Duration;

fn main() {
    let multi = Multi::new();
    let mut handles = Vec::new();

    for _ in 0..2 {
        let mut easy = Easy::new();
        easy.url("https://self-signed.badssl.com").unwrap();
        easy.http_version(HttpVersion::V2).unwrap();
        handles.push(multi.add(easy).unwrap());
    }

    while multi.perform().unwrap() > 0 {
        multi.wait(&mut [], Duration::from_secs(5)).unwrap();
    }

    drop(multi);

    println!("Multi handle freed");

    drop(handles);

    println!("Easy handles freed");
}

[iiibui]

So far I am not able to reproduce this on my local machine or inside a Centos 7.3.1611 Docker container. Are you also building OpenSSL and nghttp2 yourself? I see you are pointing to nonstandard paths for those. It would be helpful to see how you are building those.

It might also help to strip out the superfluous parts in your example program, I've rewritten it down to the following. Could you verify that the below program also crashes for you with a double-free? I tried to isolate specifically the scenario you described (error from OpenSSL + dropping multi handle without removing easy handles):

use curl::{
    easy::{Easy, HttpVersion},
    multi::Multi,
};
use std::time::Duration;

fn main() {
    let multi = Multi::new();
    let mut handles = Vec::new();

    for _ in 0..2 {
        let mut easy = Easy::new();
        easy.url("https://self-signed.badssl.com").unwrap();
        easy.http_version(HttpVersion::V2).unwrap();
        handles.push(multi.add(easy).unwrap());
    }

    while multi.perform().unwrap() > 0 {
        multi.wait(&mut [], Duration::from_secs(5)).unwrap();
    }

    drop(multi);

    println!("Multi handle freed");

    drop(handles);

    println!("Easy handles freed");
}

Sorry for my lack of rigor. Your version don't crash on my machine, so I do some modify and finally it crashed:

1. set Easy pipewait like this.
2. change the while termination condition like this.
3. drop(handles) before drop(multi).

use curl::{
    easy::{Easy, HttpVersion},
    multi::Multi,
};
use std::time::Duration;

fn main() {
    let multi = Multi::new();
    let mut handles = Vec::new();

    for _ in 0..2 {
        let mut easy = Easy::new();
        easy.url("https://self-signed.badssl.com").unwrap();
        easy.http_version(HttpVersion::V2).unwrap();
        easy.pipewait(true).unwrap();
        handles.push(multi.add(easy).unwrap());
    }

    let mut some_msg_result = None;
    while some_msg_result.is_none() {
        multi.perform().unwrap();
        multi.messages(|msg| {
            some_msg_result = msg.result();
        });
        multi.wait(&mut [], Duration::from_secs(5)).unwrap();
    }

    drop(handles);

    println!("Easy handles freed");

    drop(multi);

    println!("Multi handle freed");
}

I also try to build it with mesalink backend, it crash too, but I copy the build result to other machine which has differece network env it won't crash, it may be network IO related.

[sagebind]

Thanks, having a minimal repro will make it easier to identify the problem and apply the appropriate fix. Its interesting that dropping the easy handles first is part of the issue, since curl_easy_cleanup calls curl_multi_remove_handle internally anyway if needed.

[iiibui]

Thanks, having a minimal repro will make it easier to identify the problem and apply the appropriate fix. Its interesting that dropping the easy handles first is part of the issue, since curl_easy_cleanup calls curl_multi_remove_handle internally anyway if needed.

It seem there is "safe guard" when cleanup, all I know now is double free this pointer and it‘s not because of duplicate conn_free call. I will do more debug later.

[iiibui]

Set a http proxy to easy handle will reproduce this preblem, on my machine it's setting by the $http_proxy environment variable, I also tested this simple proxy by easy.proxy call and it crashed too. @sagebind Can you try again?

[sagebind]

@iiibui Thanks, that did the trick, I can consistently reproduce the double-free crash now as well. I'll work on stripping down the repro program further and hopefully identify the cause of why conn_free in libcurl is being called multiple times.

[sagebind]

I think this might be an upstream curl bug. Using git bisect I was able to identify that this starts happening after this commit, which definitely looks like it could be related to this sort of problem: curl/curl@51c0ebc. I will work on translating the repro into a C program so we can open a ticket upstream.

In the meantime, @iiibui could you verify my findings? If you build libcurl at commit 63813a0325adec659bdb6866c061208266b68797 and run your original example program, there should be no issue, but at commit 51c0ebcff2140c38ff389b4fcfb8216f5e9d198c should cause double-free again like master does.

For reference, I am able to consistently reproduce a double free now with the following program:

use curl::{easy::Easy, multi::Multi};
use std::{
    io::{copy, BufRead, BufReader, Write},
    net::{SocketAddr, TcpListener, TcpStream},
    thread,
};

fn main() {
    let proxy_addr = spawn_http_proxy();
    let multi = Multi::new();

    let mut easy = Easy::new();
    easy.url("https://self-signed.badssl.com").unwrap();
    easy.proxy(&format!("http://{}", proxy_addr)).unwrap();
    let easy = multi.add(easy).unwrap();

    multi.perform().unwrap();

    drop(easy);

    println!("Easy handle freed");

    drop(multi);

    println!("Multi handle freed");
}

/// Spawn a simple HTTP proxy in a background thread for curl to talk to. Really
/// inefficient with threads but also very simple.
fn spawn_http_proxy() -> SocketAddr {
    let listener = TcpListener::bind("127.0.0.1:0").unwrap();
    let addr = listener.local_addr().unwrap();

    thread::spawn(move || loop {
        let (mut client, _) = listener.accept().unwrap();

        thread::spawn(move || {
            let mut reader = BufReader::new(client.try_clone().unwrap());

            let mut request_header = String::new();

            while !request_header.contains("\r\n\r\n") {
                reader.read_line(&mut request_header).unwrap();
            }

            client.write_all(b"HTTP/1.1 200 OK\r\n\r\n").unwrap();

            let upstream_addr = request_header.split(' ').nth(1).unwrap();
            let mut upstream_reader = TcpStream::connect(upstream_addr).unwrap();
            let mut upstream_writer = upstream_reader.try_clone().unwrap();

            thread::spawn(move || {
                let _ = copy(&mut reader, &mut upstream_writer);
            });

            let _ = copy(&mut upstream_reader, &mut client);
        });
    });

    addr
}

[sagebind]

Upstream issue opened: curl/curl#7982

[iiibui]

I think this might be an upstream curl bug. Using git bisect I was able to identify that this starts happening after this commit, which definitely looks like it could be related to this sort of problem: curl/curl@51c0ebc. I will work on translating the repro into a C program so we can open a ticket upstream.

In the meantime, @iiibui could you verify my findings? If you build libcurl at commit 63813a0325adec659bdb6866c061208266b68797 and run your original example program, there should be no issue, but at commit 51c0ebcff2140c38ff389b4fcfb8216f5e9d198c should cause double-free again like master does.

For reference, I am able to consistently reproduce a double free now with the following program:

use curl::{easy::Easy, multi::Multi};
use std::{
    io::{copy, BufRead, BufReader, Write},
    net::{SocketAddr, TcpListener, TcpStream},
    thread,
};

fn main() {
    let proxy_addr = spawn_http_proxy();
    let multi = Multi::new();

    let mut easy = Easy::new();
    easy.url("https://self-signed.badssl.com").unwrap();
    easy.proxy(&format!("http://{}", proxy_addr)).unwrap();
    let easy = multi.add(easy).unwrap();

    multi.perform().unwrap();

    drop(easy);

    println!("Easy handle freed");

    drop(multi);

    println!("Multi handle freed");
}

/// Spawn a simple HTTP proxy in a background thread for curl to talk to. Really
/// inefficient with threads but also very simple.
fn spawn_http_proxy() -> SocketAddr {
    let listener = TcpListener::bind("127.0.0.1:0").unwrap();
    let addr = listener.local_addr().unwrap();

    thread::spawn(move || loop {
        let (mut client, _) = listener.accept().unwrap();

        thread::spawn(move || {
            let mut reader = BufReader::new(client.try_clone().unwrap());

            let mut request_header = String::new();

            while !request_header.contains("\r\n\r\n") {
                reader.read_line(&mut request_header).unwrap();
            }

            client.write_all(b"HTTP/1.1 200 OK\r\n\r\n").unwrap();

            let upstream_addr = request_header.split(' ').nth(1).unwrap();
            let mut upstream_reader = TcpStream::connect(upstream_addr).unwrap();
            let mut upstream_writer = upstream_reader.try_clone().unwrap();

            thread::spawn(move || {
                let _ = copy(&mut reader, &mut upstream_writer);
            });

            let _ = copy(&mut upstream_reader, &mut client);
        });
    });

    addr
}

Yes, with commit 63813a0325adec659bdb6866c061208266b68797 my original example program has no issue and commit 51c0ebcff2140c38ff389b4fcfb8216f5e9d198c cause double-free.

[sagebind]

The fix for this is now available in curl 0.4.41.

[iiibui]

The fix for this is now available in curl 0.4.41.

Thanks. @ehuss I rebuild cargo 22ff7ac47c0e3a366637643c9cf38c61d649c10b which the rust-curl dep is aready updated to 0.4.41, I cannot reproduce the double-free crash on my machine now.