search

alexcrichton / curl-rust

Rust bindings to libcurl
MIT License
1k stars 234 forks source link

Guarantee that curl_multi_remove_handle is called #423

Closed sagebind closed 2 years ago

sagebind commented 2 years ago

Add a drop handler that guarantees that curl_multi_remove_handle is always called on easy handles added to a multi handle before the easy handle is dropped. Based on https://github.com/curl/curl/issues/7982, we should not be allowing users to drop a previously attached handle without first calling curl_multi_remove_handle. In curl versions between 7.77 and 7.80 this could cause a crash under certain circumstances. While this will likely be fixed in the next curl version, it is still recommended to not allow this, plus we must still handle this case for users who may not have updated to a patched libcurl.

I'm not totally happy with how this is implemented as it adds an Arc::clone call every time an easy handle is added to a multi handle, but I couldn't think of a better way of guaranteeing this behavior without breaking API changes.

I've verified that this does actually fix https://github.com/alexcrichton/curl-rust/issues/421 even without the upstream curl patch.

Fixes https://github.com/alexcrichton/curl-rust/issues/421.

sagebind commented 2 years ago

@iiibui Can you verify that using this branch fixes your crashes even with curl 7.80? It prevented the double-free errors in my tests.

iiibui commented 2 years ago

@iiibui Can you verify that using this branch fixes your crashes even with curl 7.80? It prevented the double-free errors in my tests.

This branch fixes my origin crashes with curl 7.80, but I found it has no effect with curl commit 51c0ebcff2140c38ff389b4fcfb8216f5e9d198c......

sagebind commented 2 years ago

@iiibui Hmm yeah, I still get the double-free against 51c0ebcff2140c38ff389b4fcfb8216f5e9d198c as well, which first appears in curl 7.77.0. Seems like after this PR though we're doing things exactly as curl recommends and any other issues are likely on curl. Here's the behavior I see:

So the only released version with this issue still is 7.77.0, but that version had a known issue around that which was fixed in 7.78.0: https://github.com/curl/curl/issues/7236.