[PATCH qemu-server v2] fix #7654: prevent use after free

[PATCH qemu-server v2] fix #7654: prevent use after free

[Dominik Csapak]

to prevent a use after free on a client struct, instead of freeing it
inline in 'cleanup_client', only set a boolean flag 'pending_free'
and free it in the main 'handle_client' function.

the call graphs look like this:

handle_client
`-> cleanup_client
`-> handle_qmp_handshake
    `-> cleanup_client
    `-> send_qmp_cmd
        `-> cleanup_client
`-> handle_qmp_event
    `-> terminate_check
        `-> send_qmp_cmd
            `-> cleanup_client
`-> handle_qmp_return
    `-> terminate_check
        `-> send_qmp_cmd
            `-> cleanup_client

In addition, if we cleanup a 'vzdump' client, the corresponding VM
client will be looked up and 'terminate_check' will be called for it. If
that resulted in a cleanup, we have to free that immediately.

cleanup_client will not be called in other paths, so handling the
freeing in handle_client should suffice.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
---
changes from v1:
* picked up r-b trailer from fiona (thanks!)
* fixed commit message typo and nits

 src/qmeventd/qmeventd.c | 17 ++++++++++++++++-
 src/qmeventd/qmeventd.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/qmeventd/qmeventd.c b/src/qmeventd/qmeventd.c
index 1d9eb74a..5c873e88 100644
--- a/src/qmeventd/qmeventd.c
+++ b/src/qmeventd/qmeventd.c
@@ -405,6 +405,13 @@ void cleanup_client(struct Client *client) {
             VERBOSE_PRINT("%s: backup ended\n", client->vzdump.vmid);
             vmc->qemu.backup = false;
             terminate_check(vmc);
+
+            // we cleaned up a vm client while handling a vzdump client, so we
+            // have to free it here directly. the nested cleanup call must have
+            // removed it from the vm_clients hash table already in that case.
+            if (vmc->pending_free) {
+                free(vmc);
+            }
         }
         break;
 
@@ -418,7 +425,7 @@ void cleanup_client(struct Client *client) {
     }
     VERBOSE_PRINT("removing %s from forced cleanups\n", client->qemu.vmid);
     forced_cleanups = g_slist_remove(forced_cleanups, client);
-    free(client);
+    client->pending_free = true;
 }
 
 void terminate_client(struct Client *client) {
@@ -478,11 +485,13 @@ void handle_client(struct Client *client) {
         if (!(errno == EAGAIN || errno == EWOULDBLOCK)) {
             log_neg((int)len, "read");
             cleanup_client(client);
+            free(client);
         }
         return;
     } else if (len == 0) {
         VERBOSE_PRINT("pid%d: got EOF\n", client->pid);
         cleanup_client(client);
+        free(client);
         return;
     }
 
@@ -530,6 +539,12 @@ void handle_client(struct Client *client) {
             break;
         }
         json_object_put(jobj);
+
+        // if the client was cleaned up, we have to free it here
+        if (client->pending_free) {
+            free(client);
+            break;
+        }
     }
     json_tokener_free(tok);
 }
diff --git a/src/qmeventd/qmeventd.h b/src/qmeventd/qmeventd.h
index bb372077..6a293b84 100644
--- a/src/qmeventd/qmeventd.h
+++ b/src/qmeventd/qmeventd.h
@@ -66,6 +66,7 @@ struct Client {
     pid_t pid;
     int pidfd;
     time_t timeout;
+    bool pending_free;
 
     ClientType type;
     ClientState state;
-- 
2.47.3