[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/2] block/curl: Disconnect sockets from CURLState
From: |
Kevin Wolf |
Subject: |
Re: [PATCH 0/2] block/curl: Disconnect sockets from CURLState |
Date: |
Wed, 10 Mar 2021 12:51:57 +0100 |
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