Multi::close causes unsafe behavior

[sagebind]

Multi::close is not safe to use because it calls curl_multi_cleanup on the raw CURLM pointer, which in turn calls free on the pointer. But Multi::close only takes &self so nothing prevents you from attempting to call other methods on a Multi after you called close, which will attempt to use the freed pointer.

curl-rust/src/multi.rs

Lines 699 to 706 in ecc9d80

	/// Attempt to close the multi handle and clean up all associated resources.
	///
	/// Cleans up and removes a whole multi stack. It does not free or touch any
	/// individual easy handles in any way - they still need to be closed
	/// individually.
	pub fn close(&self) -> Result<(), MultiError> {
	unsafe { cvt(curl_sys::curl_multi_cleanup(self.raw)) }
	}

In fact, using Multi::close at all causes this, because the Drop handler itself calls curl_multi_cleanup.

I see two potential ways of fixing this:

• Change Multi::close to take self instead of &self, which is probably the original intention of the API and reflects the libcurl behavior correctly. This change would be backwards-incompatible.
• Add some sort of flag to our Multi struct indicating if curl_multi_cleanup has been called successfully and do not call curl_multi_cleanup again if that flag is set. All public methods must be updated to check the flag and return an error if the flag is set before calling any libcurl functions. This has an unfortunate runtime cost, but keeps backwards compatibility.

This was discovered downstream by @DBLouis in sagebind/isahc#198.

[DBLouis]

Valgrind also reports lots of memory leaks (hundreds of KB) that originate from libcurl and curl crate.

[alexcrichton]

Oh dear, that would be my mistake!

Given that this is equivalent to Drop I think it's fine to just "remove" the method. For now we can make it always return Ok(()), flag it #[doc(hidden)], and then remove it in the next major version bump

[sagebind]

It might be nice to keep it in a future version (taking self), as it is nice to be able to see any errors returned by curl_multi_cleanup instead of the drop handler just hiding it.

I like your temporary solution though to avoid a BC break.

[alexcrichton]

Ah that's a good point, yeah, but I agree that in that world it'd take by-value self.

I'd also be ok "fixing" it today and setting some sort of internal flag that it's defunkt to prevent the unsafety.