Re: [PATCH 0/2] block/curl: Disconnect sockets from CURLState

[Kevin Wolf]

Am 09.03.2021 um 14:05 hat Max Reitz geschrieben:
> Hi,
> 
> There’s been a bug report concerning our curl driver on Launchpad:
>   https://bugs.launchpad.net/qemu/+bug/1916501
> 
> When downloading an image from a certain URL, it crashes.
> (https://download.cirros-cloud.net/0.4.0/cirros-0.4.0-x86_64-disk.img)
> 
> The crash is a use-after-free: A CURLState (which basically represents a
> transfer) has several CURLSocket objects (encapsulating an FD) over
> which the data is transmitted.  Once that transfer is done, the state is
> purged and all CURLSocket objects belonging to it are freed, under the
> assumption that once the transfer is done, the sockets are no longer
> used.
> 
> That seems to work with most servers.
> 
> However, I suspect that in the above case, some sockets might be reused
> for later transfers; so libcurl doesn’t actually tell us to drop them
> (by invoking curl_sock_cb() with CURL_POLL_REMOVE), and that means our
> AIO handler (curl_multi_do()) is invoked for some socket after its
> corresponding CURLSocket object is freed, leading to said
> use-after-free.
> 
> I don’t think libcurl actually says anywhere that sockets are bound to
> CURL states (“CURL” objects), though one is always passed to
> curl_sock_cb().  But I can’t find any mention that a socket might not be
> reused by some other state.
> 
> In fact, there is absolutely no necessity to bind sockets to states.  We
> can trivially replace the CURLState pointer in CURLSocket by a
> BDRVCURLState pointer (patch 1), and very easily move the sockets from a
> per-state list to a global hash table (patch 2).
> 
> By doing so, there is no longer any need to free any socket object when
> purging a CURLState, because the sockets are then independent of any
> such state.  (As far as I can tell from testing, this does not lead to
> any memory leaks.  For every socket there is, libcurl does tell us
> eventually to remove it by invoking curl_sock_cb() with
> CURL_POLL_REMOVE.)

Thanks, applied to the block branch.

Kevin