From 8f1147b4119f920b69eb9c577121cbd5ac1e1d70 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <freddy77@gmail.com>
Date: Mon, 10 Aug 2020 15:27:09 +0100
Subject: [PATCH 28/31] channel-main: Use heap and reference counting for
spice_migrate
Don't use the stack, it will potentially disappear (see mig
variable in main_migrate_connect).
For instance channels use this structure when they are freed. As
the free is done in delayed mode the initial coroutine could be
ended releasing the stack and causing a segmentation fault.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1867564.
Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
Acked-by: Uri Lublin <uril@redhat.com>
---
src/channel-main.c | 110 ++++++++++++++++++++++++++++++++-------------
1 file changed, 78 insertions(+), 32 deletions(-)
diff --git a/src/channel-main.c b/src/channel-main.c
index 79fe63c..8caf727 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -123,6 +123,7 @@ struct spice_migrate {
struct coroutine *from;
SpiceMigrationDstInfo *info;
SpiceSession *session;
+ int ref_count;
guint nchannels;
SpiceChannel *src_channel;
SpiceChannel *dst_channel;
@@ -175,8 +176,8 @@ static void channel_set_handlers(SpiceChannelClass *klass);
static void agent_send_msg_queue(SpiceMainChannel *channel);
static void agent_free_msg_queue(SpiceMainChannel *channel);
static void migrate_channel_event_cb(SpiceChannel *channel, SpiceChannelEvent event,
- gpointer data);
-static gboolean main_migrate_handshake_done(gpointer data);
+ spice_migrate *mig);
+static gboolean main_migrate_handshake_done(spice_migrate *mig);
static void spice_main_channel_send_migration_handshake(SpiceChannel *channel);
static void file_xfer_flushed(SpiceMainChannel *channel, gboolean success);
static void file_xfer_read_async_cb(GObject *source_object,
@@ -193,6 +194,7 @@ static void file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
GError *error,
gpointer userdata);
static void file_transfer_operation_send_progress(SpiceFileTransferTask *xfer_task);
+static void spice_migrate_unref(spice_migrate *mig);
/* ------------------------------------------------------------------ */
@@ -387,6 +389,7 @@ static void spice_main_channel_finalize(GObject *obj)
{
SpiceMainChannelPrivate *c = SPICE_MAIN_CHANNEL(obj)->priv;
+ spice_migrate_unref(c->migrate_data);
g_free(c->agent_msg_data);
agent_free_msg_queue(SPICE_MAIN_CHANNEL(obj));
@@ -2242,11 +2245,50 @@ static void main_handle_agent_token(SpiceChannel *channel, SpiceMsgIn *in)
agent_send_msg_queue(SPICE_MAIN_CHANNEL(channel));
}
+static spice_migrate*
+spice_migrate_ref(spice_migrate *mig)
+{
+ if (mig != NULL) {
+ mig->ref_count++;
+ }
+ return mig;
+}
+
+static void
+spice_migrate_unref(spice_migrate *mig)
+{
+ if (mig != NULL && --mig->ref_count == 0) {
+ g_free(mig);
+ }
+}
+
+static inline void
+spice_migrate_idle_add(gboolean (*func)(spice_migrate *mig), spice_migrate *mig)
+{
+ g_idle_add_full(G_PRIORITY_DEFAULT_IDLE, (GSourceFunc) func, spice_migrate_ref(mig),
+ (GDestroyNotify) spice_migrate_unref);
+}
+
+static void
+spice_migrate_closure_unref(spice_migrate *mig, GClosure *closure)
+{
+ spice_migrate_unref(mig);
+}
+
+static gulong
+spice_migrate_signal_connect(gpointer instance, const gchar *detailed_signal,
+ GCallback func, spice_migrate *mig)
+{
+ return g_signal_connect_data(instance, detailed_signal, func, spice_migrate_ref(mig),
+ (GClosureNotify) spice_migrate_closure_unref,
+ (GConnectFlags) 0);
+}
+
/* main context */
-static void migrate_channel_new_cb(SpiceSession *s, SpiceChannel *channel, gpointer data)
+static void migrate_channel_new_cb(SpiceSession *s, SpiceChannel *channel, spice_migrate *mig)
{
- g_signal_connect(channel, "channel-event",
- G_CALLBACK(migrate_channel_event_cb), data);
+ spice_migrate_signal_connect(channel, "channel-event",
+ G_CALLBACK(migrate_channel_event_cb), mig);
}
static void
@@ -2267,7 +2309,7 @@ static void spice_main_channel_send_migration_handshake(SpiceChannel *channel)
if (!spice_channel_test_capability(channel, SPICE_MAIN_CAP_SEAMLESS_MIGRATE)) {
c->migrate_data->do_seamless = false;
- g_idle_add(main_migrate_handshake_done, c->migrate_data);
+ spice_migrate_idle_add(main_migrate_handshake_done, c->migrate_data);
} else {
SpiceMsgcMainMigrateDstDoSeamless msg_data;
SpiceMsgOut *msg_out;
@@ -2282,13 +2324,12 @@ static void spice_main_channel_send_migration_handshake(SpiceChannel *channel)
/* main context */
static void migrate_channel_event_cb(SpiceChannel *channel, SpiceChannelEvent event,
- gpointer data)
+ spice_migrate *mig)
{
- spice_migrate *mig = data;
SpiceChannelPrivate *c = SPICE_CHANNEL(channel)->priv;
g_return_if_fail(mig->nchannels > 0);
- g_signal_handlers_disconnect_by_func(channel, migrate_channel_event_cb, data);
+ g_signal_handlers_disconnect_by_func(channel, migrate_channel_event_cb, mig);
switch (event) {
case SPICE_CHANNEL_OPENED:
@@ -2299,7 +2340,8 @@ static void migrate_channel_event_cb(SpiceChannel *channel, SpiceChannelEvent ev
c->state = SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE;
mig->dst_channel = channel;
- main_priv->migrate_data = mig;
+ spice_migrate_unref(main_priv->migrate_data);
+ main_priv->migrate_data = spice_migrate_ref(mig);
} else {
c->state = SPICE_CHANNEL_STATE_MIGRATING;
mig->nchannels--;
@@ -2332,9 +2374,8 @@ static void migrate_channel_event_cb(SpiceChannel *channel, SpiceChannelEvent ev
}
/* main context */
-static gboolean main_migrate_handshake_done(gpointer data)
+static gboolean main_migrate_handshake_done(spice_migrate *mig)
{
- spice_migrate *mig = data;
SpiceChannelPrivate *c = SPICE_CHANNEL(mig->dst_channel)->priv;
g_return_val_if_fail(c->channel_type == SPICE_CHANNEL_MAIN, FALSE);
@@ -2348,9 +2389,8 @@ static gboolean main_migrate_handshake_done(gpointer data)
}
/* main context */
-static gboolean migrate_connect(gpointer data)
+static gboolean migrate_connect(spice_migrate *mig)
{
- spice_migrate *mig = data;
SpiceChannelPrivate *c;
int port, sport;
const char *host;
@@ -2393,8 +2433,8 @@ static gboolean migrate_connect(gpointer data)
g_object_set(mig->session, "host", host, NULL);
spice_session_set_port(mig->session, port, FALSE);
spice_session_set_port(mig->session, sport, TRUE);
- g_signal_connect(mig->session, "channel-new",
- G_CALLBACK(migrate_channel_new_cb), mig);
+ spice_migrate_signal_connect(mig->session, "channel-new",
+ G_CALLBACK(migrate_channel_new_cb), mig);
g_signal_emit(mig->src_channel, signals[SPICE_MIGRATION_STARTED], 0,
mig->session);
@@ -2414,50 +2454,56 @@ static void main_migrate_connect(SpiceChannel *channel,
{
SpiceMainChannelPrivate *main_priv = SPICE_MAIN_CHANNEL(channel)->priv;
int reply_type = SPICE_MSGC_MAIN_MIGRATE_CONNECT_ERROR;
- spice_migrate mig = { 0, };
+ spice_migrate *mig;
SpiceMsgOut *out;
SpiceSession *session;
- mig.src_channel = channel;
- mig.info = dst_info;
- mig.from = coroutine_self();
- mig.do_seamless = do_seamless;
- mig.src_mig_version = src_mig_version;
+ mig = spice_new0(spice_migrate, 1);
+ mig->ref_count = 1;
+ mig->src_channel = channel;
+ mig->info = dst_info;
+ mig->from = coroutine_self();
+ mig->do_seamless = do_seamless;
+ mig->src_mig_version = src_mig_version;
CHANNEL_DEBUG(channel, "migrate connect");
session = spice_channel_get_session(channel);
- mig.session = spice_session_new_from_session(session);
- if (mig.session == NULL)
+ mig->session = spice_session_new_from_session(session);
+ if (mig->session == NULL) {
goto end;
- if (!spice_session_set_migration_session(session, mig.session))
+ }
+ if (!spice_session_set_migration_session(session, mig->session)) {
goto end;
+ }
- main_priv->migrate_data = &mig;
+ spice_migrate_unref(main_priv->migrate_data);
+ main_priv->migrate_data = spice_migrate_ref(mig);
/* no need to track idle, call is sync for this coroutine */
- g_idle_add(migrate_connect, &mig);
+ spice_migrate_idle_add(migrate_connect, mig);
/* switch to main loop and wait for connections */
coroutine_yield(NULL);
- if (mig.nchannels != 0) {
+ if (mig->nchannels != 0) {
CHANNEL_DEBUG(channel, "migrate failed: some channels failed to connect");
spice_session_abort_migration(session);
} else {
- if (mig.do_seamless) {
+ if (mig->do_seamless) {
SPICE_DEBUG("migration (seamless): connections all ok");
reply_type = SPICE_MSGC_MAIN_MIGRATE_CONNECTED_SEAMLESS;
} else {
SPICE_DEBUG("migration (semi-seamless): connections all ok");
reply_type = SPICE_MSGC_MAIN_MIGRATE_CONNECTED;
}
- spice_session_start_migrating(session, mig.do_seamless);
+ spice_session_start_migrating(session, mig->do_seamless);
}
end:
CHANNEL_DEBUG(channel, "migrate connect reply %d", reply_type);
out = spice_msg_out_new(channel, reply_type);
spice_msg_out_send(out);
+ spice_migrate_unref(mig);
}
/* coroutine context */
@@ -2489,7 +2535,7 @@ static void main_handle_migrate_dst_seamless_ack(SpiceChannel *channel, SpiceMsg
g_return_if_fail(c->state == SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE);
main_priv->migrate_data->do_seamless = true;
- g_idle_add(main_migrate_handshake_done, main_priv->migrate_data);
+ spice_migrate_idle_add(main_migrate_handshake_done, main_priv->migrate_data);
}
static void main_handle_migrate_dst_seamless_nack(SpiceChannel *channel, SpiceMsgIn *in)
@@ -2501,7 +2547,7 @@ static void main_handle_migrate_dst_seamless_nack(SpiceChannel *channel, SpiceMs
g_return_if_fail(c->state == SPICE_CHANNEL_STATE_MIGRATION_HANDSHAKE);
main_priv->migrate_data->do_seamless = false;
- g_idle_add(main_migrate_handshake_done, main_priv->migrate_data);
+ spice_migrate_idle_add(main_migrate_handshake_done, main_priv->migrate_data);
}
/* main context */
--
2.28.0