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

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

[Max Reitz]

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

-- 
2.29.2

[PATCH 1/2] curl: Store BDRVCURLState pointer in CURLSocket

[Max Reitz]

A socket does not really belong to any specific state.  We do not need
to store a pointer to "its" state in it, a pointer to the common
BDRVCURLState is sufficient.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 4ff895df8f..43c79bcf36 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -78,7 +78,7 @@ typedef struct CURLAIOCB {
 
 typedef struct CURLSocket {
     int fd;
-    struct CURLState *state;
+    struct BDRVCURLState *s;
     QLIST_ENTRY(CURLSocket) next;
 } CURLSocket;
 
@@ -155,7 +155,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     if (!socket) {
         socket = g_new0(CURLSocket, 1);
         socket->fd = fd;
-        socket->state = state;
+        socket->s = s;
         QLIST_INSERT_HEAD(&state->sockets, socket, next);
     }
 
@@ -385,7 +385,7 @@ static void curl_multi_check_completion(BDRVCURLState *s)
 /* Called with s->mutex held.  */
 static void curl_multi_do_locked(CURLSocket *socket)
 {
-    BDRVCURLState *s = socket->state->s;
+    BDRVCURLState *s = socket->s;
     int running;
     int r;
 
@@ -401,7 +401,7 @@ static void curl_multi_do_locked(CURLSocket *socket)
 static void curl_multi_do(void *arg)
 {
     CURLSocket *socket = arg;
-    BDRVCURLState *s = socket->state->s;
+    BDRVCURLState *s = socket->s;
 
     qemu_mutex_lock(&s->mutex);
     curl_multi_do_locked(socket);
-- 
2.29.2

[PATCH 2/2] curl: Disconnect sockets from CURLState

[Max Reitz]

When a curl transfer is finished, that does not mean that CURL lets go
of all the sockets it used for it.  We therefore must not free a
CURLSocket object before CURL has invoked curl_sock_cb() to tell us to
remove it.  Otherwise, we may get a use-after-free, as described in this
bug report: https://bugs.launchpad.net/qemu/+bug/1916501

(Reproducer from that report:
  $ qemu-img convert -f qcow2 -O raw \
  https://download.cirros-cloud.net/0.4.0/cirros-0.4.0-x86_64-disk.img \
  out.img
)

(Alternatively, it might seem logical to force-drop all sockets that
have been used for a state when the respective transfer is done, kind of
like it is done now, but including unsetting the AIO handlers.
Unfortunately, doing so makes the driver just hang instead of crashing,
which seems to evidence that CURL still uses those sockets.)

Make the CURLSocket object independent of "its" CURLState by putting all
sockets into a hash table belonging to the BDRVCURLState instead of a
list that belongs to a CURLState.  Do not touch any sockets in
curl_clean_state().

Testing, it seems like all sockets are indeed gone by the time the curl
BDS is closed, so it seems like there really was no point in freeing any
socket just because a transfer is done.  libcurl does invoke
curl_sock_cb() with CURL_POLL_REMOVE for every socket it has.

Buglink: https://bugs.launchpad.net/qemu/+bug/1916501
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/curl.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 43c79bcf36..50e741a0d7 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -79,7 +79,6 @@ typedef struct CURLAIOCB {
 typedef struct CURLSocket {
     int fd;
     struct BDRVCURLState *s;
-    QLIST_ENTRY(CURLSocket) next;
 } CURLSocket;
 
 typedef struct CURLState
@@ -87,7 +86,6 @@ typedef struct CURLState
     struct BDRVCURLState *s;
     CURLAIOCB *acb[CURL_NUM_ACB];
     CURL *curl;
-    QLIST_HEAD(, CURLSocket) sockets;
     char *orig_buf;
     uint64_t buf_start;
     size_t buf_off;
@@ -102,6 +100,7 @@ typedef struct BDRVCURLState {
     QEMUTimer timer;
     uint64_t len;
     CURLState states[CURL_NUM_STATES];
+    GHashTable *sockets; /* GINT_TO_POINTER(fd) -> socket */
     char *url;
     size_t readahead_size;
     bool sslverify;
@@ -120,6 +119,21 @@ typedef struct BDRVCURLState {
 static void curl_clean_state(CURLState *s);
 static void curl_multi_do(void *arg);
 
+static gboolean curl_drop_socket(void *key, void *value, void *opaque)
+{
+    CURLSocket *socket = value;
+    BDRVCURLState *s = socket->s;
+
+    aio_set_fd_handler(s->aio_context, socket->fd, false,
+                       NULL, NULL, NULL, NULL);
+    return true;
+}
+
+static void curl_drop_all_sockets(GHashTable *sockets)
+{
+    g_hash_table_foreach_remove(sockets, curl_drop_socket, NULL);
+}
+
 /* Called from curl_multi_do_locked, with s->mutex held.  */
 static int curl_timer_cb(CURLM *multi, long timeout_ms, void *opaque)
 {
@@ -147,16 +161,12 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     curl_easy_getinfo(curl, CURLINFO_PRIVATE, (char **)&state);
     s = state->s;
 
-    QLIST_FOREACH(socket, &state->sockets, next) {
-        if (socket->fd == fd) {
-            break;
-        }
-    }
+    socket = g_hash_table_lookup(s->sockets, GINT_TO_POINTER(fd));
     if (!socket) {
         socket = g_new0(CURLSocket, 1);
         socket->fd = fd;
         socket->s = s;
-        QLIST_INSERT_HEAD(&state->sockets, socket, next);
+        g_hash_table_insert(s->sockets, GINT_TO_POINTER(fd), socket);
     }
 
     trace_curl_sock_cb(action, (int)fd);
@@ -180,8 +190,7 @@ static int curl_sock_cb(CURL *curl, curl_socket_t fd, int action,
     }
 
     if (action == CURL_POLL_REMOVE) {
-        QLIST_REMOVE(socket, next);
-        g_free(socket);
+        g_hash_table_remove(s->sockets, GINT_TO_POINTER(fd));
     }
 
     return 0;
@@ -498,7 +507,6 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
 #endif
     }
 
-    QLIST_INIT(&state->sockets);
     state->s = s;
 
     return 0;
@@ -515,13 +523,6 @@ static void curl_clean_state(CURLState *s)
     if (s->s->multi)
         curl_multi_remove_handle(s->s->multi, s->curl);
 
-    while (!QLIST_EMPTY(&s->sockets)) {
-        CURLSocket *socket = QLIST_FIRST(&s->sockets);
-
-        QLIST_REMOVE(socket, next);
-        g_free(socket);
-    }
-
     s->in_use = 0;
 
     qemu_co_enter_next(&s->s->free_state_waitq, &s->s->mutex);
@@ -539,6 +540,7 @@ static void curl_detach_aio_context(BlockDriverState *bs)
     int i;
 
     WITH_QEMU_LOCK_GUARD(&s->mutex) {
+        curl_drop_all_sockets(s->sockets);
         for (i = 0; i < CURL_NUM_STATES; i++) {
             if (s->states[i].in_use) {
                 curl_clean_state(&s->states[i]);
@@ -745,6 +747,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_co_queue_init(&s->free_state_waitq);
     s->aio_context = bdrv_get_aio_context(bs);
     s->url = g_strdup(file);
+    s->sockets = g_hash_table_new_full(NULL, NULL, NULL, g_free);
     qemu_mutex_lock(&s->mutex);
     state = curl_find_state(s);
     qemu_mutex_unlock(&s->mutex);
@@ -818,6 +821,8 @@ out_noclean:
     g_free(s->username);
     g_free(s->proxyusername);
     g_free(s->proxypassword);
+    curl_drop_all_sockets(s->sockets);
+    g_hash_table_destroy(s->sockets);
     qemu_opts_del(opts);
     return -EINVAL;
 }
@@ -916,6 +921,7 @@ static void curl_close(BlockDriverState *bs)
     curl_detach_aio_context(bs);
     qemu_mutex_destroy(&s->mutex);
 
+    g_hash_table_destroy(s->sockets);
     g_free(s->cookie);
     g_free(s->url);
     g_free(s->username);
-- 
2.29.2

Re: [PATCH 1/2] curl: Store BDRVCURLState pointer in CURLSocket

[Philippe Mathieu-Daudé]

On 3/9/21 2:05 PM, Max Reitz wrote:
> A socket does not really belong to any specific state.  We do not need
> to store a pointer to "its" state in it, a pointer to the common
> BDRVCURLState is sufficient.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/curl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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