[gnome-remote-desktop] clipboard-rdp: Prevent various race conditions due to g_timeout_add()

[Jonas Ådahl <jadahl src gnome org>]

commit 81c2b0919fd2aaced6e88699dfa3dc07c66cca8d
Author: Pascal Nowack <Pascal Nowack gmx de>
Date:   Thu Feb 25 16:03:15 2021 +0100

    clipboard-rdp: Prevent various race conditions due to g_timeout_add()
    
    g_timeout_add() is racy. Technically, it is safe to call from any
    thread, when creating and attaching the new source.
    However, this does not apply for the source id.
    To be clear: The following can happen in a worse case:
    
    1. g_timeout_add() is called
    2. The source is created and attached to the main context
    3. The main context directly picks up the source and runs its source
       function.
    4. The source id is set to 0 and the source is removed from the main
       context.
    5. g_timeout_add() returns the id.
    
    In this situation, the id is not valid any more.
    
    To ensure that we don't run into this situation any more, add some
    synchronization.
    This can be easily done with the already existing WinPR events.
    
    An alternative would be to use GSources directly.
    However, even in that case, we would still need the WinPR events, so
    this would just add an unnecessary amount of code lines.
    
    The WinPR events to add the synchronization are:
    - format_list_received_event
    - format_list_response_received_event
    - format_data_request_received_event
    
    WinPR events have two states: signaled and non-signaled.
    Each of these three events is created in the non-signaled state.
    
    When queueing the update task (e.g. for format_list_received_event (a
    new FormatList was received)), then the event is signaled using
    SetEvent() after g_timeout_add() was called.
    The main thread can already pick up the task and execute it, in case of
    g_timeout_add() didn't return the source id yet.
    The source function won't however be able to set the source id to 0
    yet.
    Instead, it waits in WaitForSingleObject (event, max_wait_time).
    WaitForSingleObject() will first check the event, whether it was
    signalled.
    If it was, then the function will directly return WAIT_OBJECT_0.
    The return value will however be discarded here, as we don't need it.
    In our case, it will always be WAIT_OBJECT_0, as the max_wait_time is
    set to INFINITE.
    If the event was not signalled, then WaitForSingleObject() will wait
    until either the event is signalled (using SetEvent()) or until the
    max_wait_time has passed.
    For the synchronization, use INFINITE as max_wait_time, since we
    definitely know that the event will be signalled (after g_timeout_add()
    returned the source id).
    
    After WaitForSingleObject(event, INFINITE) returns for our event, the
    source id can be safely removed (by setting the id to 0 and return
    G_SOURCE_REMOVE), as it is now set by g_timeout_add() and therefore
    won't be overwritten any more with an now invalid one.
    
    If another call happens for the same type of update task (e.g. another
    FormatList was received) during the current source function run, then
    that call will wait until the current source function completes.
    This is realized with the completed_format_list_event.
    This event already exists (with the initial state being signalled) and
    will be reset before g_timeout_add() is called.
    It will be set after the format_list_received_event was reset.
    Using WaitForMultipleObjects() the cliprdr thread either waits for the
    source function to end (completed_format_list_event is signalled) or
    the session ends (stop_event is signalled).
    This pattern ensures that only one update task can be queued to the
    main context before another one can run.
    This is not only necessary to ensure that the task is removed (using
    g_source_remove()) when the session is stopped), but to also ensure
    that all tasks run in the desired order.
    This part is important, as otherwise gnome-remote-desktop would be
    unable to respond to the client with the correct messages, as it needs,
    among other things, to be able to always track the current state, the
    current advertised formats or whether the client can now retrieve the
    content for a specific mime type.
    
    After the source id is set to 0, the event will be reset using
    ResetEvent() to return into the initial state and the source function
    returns.
    
    WinPR events are thread safe, so it is completely fine to call
    SetEvent()/ResetEvent() from multiple threads, although this practice
    is not used here.
    The same applies to WaitForSingleObject()/WaitForMultipleObjects().
    Both check-and-wait calls can run at the same time without running into
    problems where only one of those calls would return when a tracked
    event is signalled.
    
    So, the synchronization using the WinPR events follows a specific
    pattern:
    1. Wait until the current task completes (in the cliprdr thread)
    2. Queue task using g_timeout_add() and set the associated event using
       SetEvent() in the cliprdr thread.
    3. Before the source function returns, wait for the associated event
       to be signalled, in case of the event is not signalled yet (main
       thread)
    4. Reset the associated event.
    
    In addition to the synchronization task, these WinPR events have
    another purpose.
    For example, the format_list_received_event also aborts
    FormatDataRequests, when a new FormatList is received, as the client
    won't send a FormatDataResponse for the request any more.
    
    So, add the synchronization using WinPR events to prevent any race
    conditions that can happen with g_timeout_add(), when using multiple
    threads.

 src/grd-clipboard-rdp.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
---
diff --git a/src/grd-clipboard-rdp.c b/src/grd-clipboard-rdp.c
index d2edaf5..4f6dc3d 100644
--- a/src/grd-clipboard-rdp.c
+++ b/src/grd-clipboard-rdp.c
@@ -80,6 +80,7 @@ struct _GrdClipboardRdp
   HANDLE completed_format_data_request_event;
   HANDLE format_list_received_event;
   HANDLE format_list_response_received_event;
+  HANDLE format_data_request_received_event;
   unsigned int pending_server_formats_drop_id;
   unsigned int server_format_list_update_id;
   unsigned int server_format_data_request_id;
@@ -775,9 +776,10 @@ update_server_format_list (gpointer user_data)
 
   g_free (update_context);
 
+  WaitForSingleObject (clipboard_rdp->format_list_received_event, INFINITE);
   clipboard_rdp->server_format_list_update_id = 0;
-  SetEvent (clipboard_rdp->completed_format_list_event);
   ResetEvent (clipboard_rdp->format_list_received_event);
+  SetEvent (clipboard_rdp->completed_format_list_event);
 
   return G_SOURCE_REMOVE;
 }
@@ -917,7 +919,6 @@ cliprdr_client_format_list (CliprdrServerContext      *cliprdr_context,
   if (WaitForSingleObject (clipboard_rdp->stop_event, 0) == WAIT_OBJECT_0)
     return CHANNEL_RC_OK;
 
-  SetEvent (clipboard_rdp->format_list_received_event);
   ResetEvent (clipboard_rdp->completed_format_list_event);
   update_context = g_malloc0 (sizeof (ServerFormatListUpdateContext));
   update_context->clipboard_rdp = clipboard_rdp;
@@ -925,6 +926,7 @@ cliprdr_client_format_list (CliprdrServerContext      *cliprdr_context,
 
   clipboard_rdp->server_format_list_update_id =
     g_timeout_add (0, update_server_format_list, update_context);
+  SetEvent (clipboard_rdp->format_list_received_event);
 
   return CHANNEL_RC_OK;
 }
@@ -941,6 +943,8 @@ handle_format_list_response (gpointer user_data)
   g_clear_handle_id (&clipboard_rdp->pending_server_formats_drop_id,
                      g_source_remove);
 
+  WaitForSingleObject (clipboard_rdp->format_list_response_received_event,
+                       INFINITE);
   clipboard_rdp->client_format_list_response_id = 0;
   ResetEvent (clipboard_rdp->format_list_response_received_event);
 
@@ -1108,7 +1112,10 @@ request_server_format_data (gpointer user_data)
   g_free (dst_data);
   g_free (request_context);
 
+  WaitForSingleObject (clipboard_rdp->format_data_request_received_event,
+                       INFINITE);
   clipboard_rdp->server_format_data_request_id = 0;
+  ResetEvent (clipboard_rdp->format_data_request_received_event);
   SetEvent (clipboard_rdp->completed_format_data_request_event);
 
   return G_SOURCE_REMOVE;
@@ -1212,6 +1219,7 @@ cliprdr_client_format_data_request (CliprdrServerContext              *cliprdr_c
 
       clipboard_rdp->server_format_data_request_id =
         g_timeout_add (0, request_server_format_data, request_context);
+      SetEvent (clipboard_rdp->format_data_request_received_event);
 
       return CHANNEL_RC_OK;
     }
@@ -1519,6 +1527,8 @@ grd_clipboard_rdp_dispose (GObject *object)
   g_clear_pointer (&clipboard_rdp->fuse_mount_path, g_free);
   g_clear_pointer (&clipboard_rdp->format_data_cache, g_hash_table_unref);
   g_clear_pointer (&clipboard_rdp->allowed_server_formats, g_hash_table_destroy);
+  g_clear_pointer (&clipboard_rdp->format_data_request_received_event,
+                   CloseHandle);
   g_clear_pointer (&clipboard_rdp->format_list_response_received_event,
                    CloseHandle);
   g_clear_pointer (&clipboard_rdp->format_list_received_event, CloseHandle);
@@ -1583,6 +1593,8 @@ grd_clipboard_rdp_init (GrdClipboardRdp *clipboard_rdp)
     CreateEvent (NULL, TRUE, FALSE, NULL);
   clipboard_rdp->format_list_response_received_event =
     CreateEvent (NULL, TRUE, FALSE, NULL);
+  clipboard_rdp->format_data_request_received_event =
+    CreateEvent (NULL, TRUE, FALSE, NULL);
 
   clipboard_rdp->allowed_server_formats = g_hash_table_new (NULL, NULL);
   clipboard_rdp->format_data_cache = g_hash_table_new (NULL, NULL);