585be3cc09
- util: keep track of full GSource object not source ID number - rpc: mark source returned by virEventGLibAddSocketWatch as unused - rpc: ensure temporary GSource is removed from client event loop (CVE-2024-4418)
304 lines
11 KiB
Diff
304 lines
11 KiB
Diff
From 4160ede0261dfccd8156721a22fa48d9cd3c448a Mon Sep 17 00:00:00 2001
|
|
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
|
|
Date: Tue, 28 Jul 2020 15:54:13 +0100
|
|
Subject: [PATCH 1/3] util: keep track of full GSource object not source ID
|
|
number
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
The source ID number is an alternative way to identify a source that has
|
|
been added to a GMainContext. Internally when a source ID is given, glib
|
|
will lookup the corresponding GSource and use that. The use of a source
|
|
ID is racy in some cases though, because it is invalid to continue to
|
|
use an ID number after the GSource has been removed. It is thus safer
|
|
to use the GSource object directly and have full control over the ref
|
|
counting and thus cleanup.
|
|
|
|
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
|
|
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
|
|
---
|
|
src/rpc/virnetclient.c | 27 +++++++------
|
|
src/util/vireventglib.c | 73 +++++++++++++++++++++++-------------
|
|
src/util/vireventglibwatch.c | 19 ++++++----
|
|
src/util/vireventglibwatch.h | 13 ++++---
|
|
4 files changed, 79 insertions(+), 53 deletions(-)
|
|
|
|
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
|
|
index 1c5bef86a1..0a413f0153 100644
|
|
--- a/src/rpc/virnetclient.c
|
|
+++ b/src/rpc/virnetclient.c
|
|
@@ -826,6 +826,7 @@ virNetClientIOEventTLS(int fd,
|
|
static gboolean
|
|
virNetClientTLSHandshake(virNetClientPtr client)
|
|
{
|
|
+ g_autoptr(GSource) source = NULL;
|
|
GIOCondition ev;
|
|
int ret;
|
|
|
|
@@ -840,10 +841,10 @@ virNetClientTLSHandshake(virNetClientPtr client)
|
|
else
|
|
ev = G_IO_OUT;
|
|
|
|
- virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
|
|
- ev,
|
|
- client->eventCtx,
|
|
- virNetClientIOEventTLS, client, NULL);
|
|
+ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
|
|
+ ev,
|
|
+ client->eventCtx,
|
|
+ virNetClientIOEventTLS, client, NULL);
|
|
|
|
return TRUE;
|
|
}
|
|
@@ -882,6 +883,7 @@ int virNetClientSetTLSSession(virNetClientPtr client,
|
|
int ret;
|
|
char buf[1];
|
|
int len;
|
|
+ g_autoptr(GSource) source = NULL;
|
|
|
|
#ifndef WIN32
|
|
sigset_t oldmask, blockedsigs;
|
|
@@ -934,10 +936,10 @@ int virNetClientSetTLSSession(virNetClientPtr client,
|
|
* etc. If we make the grade, it will send us a '\1' byte.
|
|
*/
|
|
|
|
- virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
|
|
- G_IO_IN,
|
|
- client->eventCtx,
|
|
- virNetClientIOEventTLSConfirm, client, NULL);
|
|
+ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
|
|
+ G_IO_IN,
|
|
+ client->eventCtx,
|
|
+ virNetClientIOEventTLSConfirm, client, NULL);
|
|
|
|
#ifndef WIN32
|
|
/* Block SIGWINCH from interrupting poll in curses programs */
|
|
@@ -1617,6 +1619,7 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
|
|
#endif /* !WIN32 */
|
|
int timeout = -1;
|
|
virNetMessagePtr msg = NULL;
|
|
+ g_autoptr(GSource) source = NULL;
|
|
GIOCondition ev = 0;
|
|
struct virNetClientIOEventData data = {
|
|
.client = client,
|
|
@@ -1651,10 +1654,10 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
|
|
if (client->nstreams)
|
|
ev |= G_IO_IN;
|
|
|
|
- virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
|
|
- ev,
|
|
- client->eventCtx,
|
|
- virNetClientIOEventFD, &data, NULL);
|
|
+ source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
|
|
+ ev,
|
|
+ client->eventCtx,
|
|
+ virNetClientIOEventFD, &data, NULL);
|
|
|
|
/* Release lock while poll'ing so other threads
|
|
* can stuff themselves on the queue */
|
|
diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
|
|
index 803332a6f8..6e334b3398 100644
|
|
--- a/src/util/vireventglib.c
|
|
+++ b/src/util/vireventglib.c
|
|
@@ -45,7 +45,7 @@ struct virEventGLibHandle
|
|
int fd;
|
|
int events;
|
|
int removed;
|
|
- guint source;
|
|
+ GSource *source;
|
|
virEventHandleCallback cb;
|
|
void *opaque;
|
|
virFreeCallback ff;
|
|
@@ -56,7 +56,7 @@ struct virEventGLibTimeout
|
|
int timer;
|
|
int interval;
|
|
int removed;
|
|
- guint source;
|
|
+ GSource *source;
|
|
virEventTimeoutCallback cb;
|
|
void *opaque;
|
|
virFreeCallback ff;
|
|
@@ -210,23 +210,25 @@ virEventGLibHandleUpdate(int watch,
|
|
if (events == data->events)
|
|
goto cleanup;
|
|
|
|
- if (data->source != 0) {
|
|
- VIR_DEBUG("Removed old handle watch=%d", data->source);
|
|
- g_source_remove(data->source);
|
|
+ if (data->source != NULL) {
|
|
+ VIR_DEBUG("Removed old handle source=%p", data->source);
|
|
+ g_source_destroy(data->source);
|
|
+ g_source_unref(data->source);
|
|
}
|
|
|
|
data->source = virEventGLibAddSocketWatch(
|
|
data->fd, cond, NULL, virEventGLibHandleDispatch, data, NULL);
|
|
|
|
data->events = events;
|
|
- VIR_DEBUG("Added new handle watch=%d", data->source);
|
|
+ VIR_DEBUG("Added new handle source=%p", data->source);
|
|
} else {
|
|
- if (data->source == 0)
|
|
+ if (data->source == NULL)
|
|
goto cleanup;
|
|
|
|
- VIR_DEBUG("Removed old handle watch=%d", data->source);
|
|
- g_source_remove(data->source);
|
|
- data->source = 0;
|
|
+ VIR_DEBUG("Removed old handle source=%p", data->source);
|
|
+ g_source_destroy(data->source);
|
|
+ g_source_unref(data->source);
|
|
+ data->source = NULL;
|
|
data->events = 0;
|
|
}
|
|
|
|
@@ -272,9 +274,10 @@ virEventGLibHandleRemove(int watch)
|
|
VIR_DEBUG("Remove handle data=%p watch=%d fd=%d",
|
|
data, watch, data->fd);
|
|
|
|
- if (data->source != 0) {
|
|
- g_source_remove(data->source);
|
|
- data->source = 0;
|
|
+ if (data->source != NULL) {
|
|
+ g_source_destroy(data->source);
|
|
+ g_source_unref(data->source);
|
|
+ data->source = NULL;
|
|
data->events = 0;
|
|
}
|
|
|
|
@@ -309,6 +312,22 @@ virEventGLibTimeoutDispatch(void *opaque)
|
|
return TRUE;
|
|
}
|
|
|
|
+
|
|
+static GSource *
|
|
+virEventGLibTimeoutCreate(int interval,
|
|
+ struct virEventGLibTimeout *data)
|
|
+{
|
|
+ GSource *source = g_timeout_source_new(interval);
|
|
+
|
|
+ g_source_set_callback(source,
|
|
+ virEventGLibTimeoutDispatch,
|
|
+ data, NULL);
|
|
+ g_source_attach(source, NULL);
|
|
+
|
|
+ return source;
|
|
+}
|
|
+
|
|
+
|
|
static int
|
|
virEventGLibTimeoutAdd(int interval,
|
|
virEventTimeoutCallback cb,
|
|
@@ -327,9 +346,7 @@ virEventGLibTimeoutAdd(int interval,
|
|
data->opaque = opaque;
|
|
data->ff = ff;
|
|
if (interval >= 0)
|
|
- data->source = g_timeout_add(interval,
|
|
- virEventGLibTimeoutDispatch,
|
|
- data);
|
|
+ data->source = virEventGLibTimeoutCreate(interval, data);
|
|
|
|
g_ptr_array_add(timeouts, data);
|
|
|
|
@@ -390,19 +407,20 @@ virEventGLibTimeoutUpdate(int timer,
|
|
VIR_DEBUG("Update timeout data=%p timer=%d interval=%d ms", data, timer, interval);
|
|
|
|
if (interval >= 0) {
|
|
- if (data->source != 0)
|
|
- g_source_remove(data->source);
|
|
+ if (data->source != NULL) {
|
|
+ g_source_destroy(data->source);
|
|
+ g_source_unref(data->source);
|
|
+ }
|
|
|
|
data->interval = interval;
|
|
- data->source = g_timeout_add(data->interval,
|
|
- virEventGLibTimeoutDispatch,
|
|
- data);
|
|
+ data->source = virEventGLibTimeoutCreate(interval, data);
|
|
} else {
|
|
- if (data->source == 0)
|
|
+ if (data->source == NULL)
|
|
goto cleanup;
|
|
|
|
- g_source_remove(data->source);
|
|
- data->source = 0;
|
|
+ g_source_destroy(data->source);
|
|
+ g_source_unref(data->source);
|
|
+ data->source = NULL;
|
|
}
|
|
|
|
cleanup:
|
|
@@ -448,9 +466,10 @@ virEventGLibTimeoutRemove(int timer)
|
|
VIR_DEBUG("Remove timeout data=%p timer=%d",
|
|
data, timer);
|
|
|
|
- if (data->source != 0) {
|
|
- g_source_remove(data->source);
|
|
- data->source = 0;
|
|
+ if (data->source != NULL) {
|
|
+ g_source_destroy(data->source);
|
|
+ g_source_unref(data->source);
|
|
+ data->source = NULL;
|
|
}
|
|
|
|
/* since the actual timeout deletion is done asynchronously, a timeoutUpdate call may
|
|
diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
|
|
index 178707f6b7..b7f3a8786a 100644
|
|
--- a/src/util/vireventglibwatch.c
|
|
+++ b/src/util/vireventglibwatch.c
|
|
@@ -233,17 +233,20 @@ GSource *virEventGLibCreateSocketWatch(int fd,
|
|
#endif /* WIN32 */
|
|
|
|
|
|
-guint virEventGLibAddSocketWatch(int fd,
|
|
- GIOCondition condition,
|
|
- GMainContext *context,
|
|
- virEventGLibSocketFunc func,
|
|
- gpointer opaque,
|
|
- GDestroyNotify notify)
|
|
+GSource *
|
|
+virEventGLibAddSocketWatch(int fd,
|
|
+ GIOCondition condition,
|
|
+ GMainContext *context,
|
|
+ virEventGLibSocketFunc func,
|
|
+ gpointer opaque,
|
|
+ GDestroyNotify notify)
|
|
{
|
|
- g_autoptr(GSource) source = NULL;
|
|
+ GSource *source = NULL;
|
|
|
|
source = virEventGLibCreateSocketWatch(fd, condition);
|
|
g_source_set_callback(source, (GSourceFunc)func, opaque, notify);
|
|
|
|
- return g_source_attach(source, context);
|
|
+ g_source_attach(source, context);
|
|
+
|
|
+ return source;
|
|
}
|
|
diff --git a/src/util/vireventglibwatch.h b/src/util/vireventglibwatch.h
|
|
index 2f7e61cfba..f57be1f503 100644
|
|
--- a/src/util/vireventglibwatch.h
|
|
+++ b/src/util/vireventglibwatch.h
|
|
@@ -40,9 +40,10 @@ typedef gboolean (*virEventGLibSocketFunc)(int fd,
|
|
GIOCondition condition,
|
|
gpointer data);
|
|
|
|
-guint virEventGLibAddSocketWatch(int fd,
|
|
- GIOCondition condition,
|
|
- GMainContext *context,
|
|
- virEventGLibSocketFunc func,
|
|
- gpointer opaque,
|
|
- GDestroyNotify notify);
|
|
+GSource *virEventGLibAddSocketWatch(int fd,
|
|
+ GIOCondition condition,
|
|
+ GMainContext *context,
|
|
+ virEventGLibSocketFunc func,
|
|
+ gpointer opaque,
|
|
+ GDestroyNotify notify)
|
|
+ G_GNUC_WARN_UNUSED_RESULT;
|
|
--
|
|
2.33.0
|
|
|