[Spice-devel] [PATCH 2/2] Avoid a race when receiving monitors config at startup

[Jonathon Jongsma]

When a new DisplayChannelClient is connected, we send out a monitors
config message so that the client knows the current state of the display
configuration. In some circumstances (e.g. virt-viewer fullscreen
auto-conf), the client will send a monitors config message as soon as
the vdagent is connected. This can happen before the display channel
client is connected. So we can get into a situation where the
DisplayChannelClient is sending out a stale monitors configuration
message while the guest is in the process of re-configuring its
displays. This can cause misbehavior on the guest. To avoid this race,
we keep track of whether we have received a MonitorsConfig message from
the client prior to sending out our initial MonitorsConfig message from
the server. If so, we delay our initial message to give the server a
chance to handle the re-configuration requested by the client.
---
 server/main_channel.c | 13 +++++++++++++
 server/main_channel.h |  3 +++
 server/red_worker.c   | 38 ++++++++++++++++++++++++++++++++++++--
 server/reds.c         |  1 +
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/server/main_channel.c b/server/main_channel.c
index 54718ba..111e139 100644
--- a/server/main_channel.c
+++ b/server/main_channel.c
@@ -160,6 +160,7 @@ struct MainChannelClient {
     int mig_wait_prev_try_seamless;
     int init_sent;
     int seamless_mig_dst;
+    int monitors_config_recvd;
 };
 
 enum NetTestStage {
@@ -1112,6 +1113,7 @@ static MainChannelClient *main_channel_client_create(MainChannel *main_chan, Red
     spice_assert(mcc != NULL);
     mcc->connection_id = connection_id;
     mcc->bitrate_per_sec = ~0;
+    mcc->monitors_config_recvd = FALSE;
 #ifdef RED_STATISTICS
     if (!(mcc->ping_timer = core->timer_add(ping_timer_cb, NULL))) {
         spice_error("ping timer create failed");
@@ -1364,3 +1366,14 @@ int main_channel_migrate_src_complete(MainChannel *main_chan, int success)
    }
    return semi_seamless_count;
 }
+
+void main_channel_client_received_monitors_config(MainChannelClient *mcc)
+{
+    mcc->monitors_config_recvd = TRUE;
+}
+
+int main_channel_client_did_receive_monitors_config(MainChannelClient *mcc)
+{
+    return mcc->monitors_config_recvd;
+}
+
diff --git a/server/main_channel.h b/server/main_channel.h
index c8e9ade..267bcc1 100644
--- a/server/main_channel.h
+++ b/server/main_channel.h
@@ -95,4 +95,7 @@ void main_channel_migrate_dst_complete(MainChannelClient *mcc);
 void main_channel_push_name(MainChannelClient *mcc, const char *name);
 void main_channel_push_uuid(MainChannelClient *mcc, const uint8_t uuid[16]);
 
+void main_channel_client_received_monitors_config(MainChannelClient *mcc);
+int main_channel_client_did_receive_monitors_config(MainChannelClient *mcc);
+
 #endif
diff --git a/server/red_worker.c b/server/red_worker.c
index 5deb30b..dfe8e04 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -726,6 +726,7 @@ struct DisplayChannelClient {
     int use_mjpeg_encoder_rate_control;
     uint32_t streams_max_latency;
     uint64_t streams_max_bit_rate;
+    SpiceTimer *monitors_config_timer;
 };
 
 struct DisplayChannel {
@@ -9831,11 +9832,24 @@ static int display_channel_client_wait_for_init(DisplayChannelClient *dcc)
     return FALSE;
 }
 
+static void red_push_monitors_config_internal(DisplayChannelClient *dcc);
+static void red_schedule_push_monitors_config(DisplayChannelClient *dcc)
+{
+    spice_debug("Scheduling push_monitors_config for later to avoid race...");
+    if (!dcc->monitors_config_timer)
+        dcc->monitors_config_timer = spice_timer_queue_add((SpiceTimerFunc)red_push_monitors_config_internal, dcc);
+
+    spice_assert(dcc->monitors_config_timer);
+    spice_timer_set(dcc->monitors_config_timer, 1000);
+}
+
 static void on_new_display_channel_client(DisplayChannelClient *dcc)
 {
     DisplayChannel *display_channel = DCC_TO_DC(dcc);
     RedWorker *worker = display_channel->common.worker;
     RedChannelClient *rcc = &dcc->common.base;
+    RedClient *client = red_channel_client_get_client(rcc);
+    MainChannelClient *mcc = red_client_get_main(client);
 
     red_channel_client_push_set_ack(&dcc->common.base);
 
@@ -9851,7 +9865,16 @@ static void on_new_display_channel_client(DisplayChannelClient *dcc)
         red_current_flush(worker, 0);
         push_new_primary_surface(dcc);
         red_push_surface_image(dcc, 0);
-        red_push_monitors_config(dcc);
+        if(main_channel_client_did_receive_monitors_config(mcc)) {
+            /* if we received a monitors config message from the client before
+             * we could send out a monitors config message from the server,
+             * there is a potential for a race where we send a stale monitors
+             * configuration to the client. Schedule this message for later to
+             * avoid this race*/
+            red_schedule_push_monitors_config(dcc);
+        } else {
+            red_push_monitors_config(dcc);
+        }
         red_pipe_add_verb(rcc, SPICE_MSG_DISPLAY_MARK);
         red_disply_start_streams(dcc);
     }
@@ -10464,6 +10487,7 @@ DisplayChannelClient *display_channel_client_create(CommonChannel *common,
     }
     ring_init(&dcc->palette_cache_lru);
     dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
+    dcc->monitors_config_timer = NULL;
     return dcc;
 }
 
@@ -11262,7 +11286,8 @@ static void worker_update_monitors_config(RedWorker *worker,
     memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size);
 }
 
-static void red_push_monitors_config(DisplayChannelClient *dcc)
+/* don't remove the timer */
+static void red_push_monitors_config_internal(DisplayChannelClient *dcc)
 {
     MonitorsConfig *monitors_config = DCC_TO_WORKER(dcc)->monitors_config;
 
@@ -11279,6 +11304,15 @@ static void red_push_monitors_config(DisplayChannelClient *dcc)
     red_channel_client_push(&dcc->common.base);
 }
 
+static void red_push_monitors_config(DisplayChannelClient *dcc)
+{
+    if (dcc->monitors_config_timer) {
+        spice_timer_remove(dcc->monitors_config_timer);
+        dcc->monitors_config_timer = NULL;
+    }
+    red_push_monitors_config_internal(dcc);
+}
+
 static void red_worker_push_monitors_config(RedWorker *worker)
 {
     DisplayChannelClient *dcc;
diff --git a/server/reds.c b/server/reds.c
index 4e36961..206702d 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1033,6 +1033,7 @@ void reds_on_main_agent_data(MainChannelClient *mcc, void *message, size_t size)
     case AGENT_MSG_FILTER_DISCARD:
         return;
     case AGENT_MSG_FILTER_MONITORS_CONFIG:
+        main_channel_client_received_monitors_config(mcc);
         if (red_dispatcher_use_client_monitors_config()) {
             reds_on_main_agent_monitors_config(mcc, message, size);
             return;
-- 
2.1.0