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

Message

Max Reitz March 9, 2021, 1:05 p.m. UTC

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.)


Max Reitz (2):
  curl: Store BDRVCURLState pointer in CURLSocket
  curl: Disconnect sockets from CURLState

 block/curl.c | 50 ++++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

Comments

Kevin Wolf March 10, 2021, 11:51 a.m. UTC | #1

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