[PR] GUACAMOLE-2341: Correct disconnect race vs. RDP channel cleanup. [guacamole-server]

["mike-jumper (via GitHub)" <gi...@apache.org>]

mike-jumper opened a new pull request, #726:
URL: https://github.com/apache/guacamole-server/pull/726

   This change corrects cases where handling of RDP channels might race with resource cleanup at disconnect, leading to segfaults. Prior to the addition of the watchdog, a segfault during cleanup could lead to failure to reap the connection process (one of the possible causes of GUACAMOLE-2118).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GUACAMOLE-2341: Correct disconnect race vs. RDP channel cleanup. [guacamole-server]

["necouchman (via GitHub)" <gi...@apache.org>]

necouchman commented on code in PR #726:
URL: https://github.com/apache/guacamole-server/pull/726#discussion_r4352936610


##########
src/protocols/rdp/channels/audio-input/audio-buffer.c:
##########
@@ -241,22 +304,41 @@ static void* guac_rdp_audio_buffer_flush_thread(void* data) {
                 guac_rdp_audio_buffer_duration(&audio_buffer->out_format, audio_buffer->bytes_written),
                 audio_buffer->bytes_written);
 
-        /* Only actually invoke if defined */
-        if (audio_buffer->flush_handler) {
-            guac_rdp_audio_buffer_schedule_flush(audio_buffer);
-            audio_buffer->flush_handler(audio_buffer,
-                    audio_buffer->packet_size);
+        guac_rdp_audio_buffer_schedule_flush(audio_buffer);
+
+        /* NOTE: Sending the audio packet requires the RDP message lock, which
+         * must not be acquired after the audio buffer lock. Doing otherwise
+         * would be a lock order violation and could lead to deadlock.
+         *
+         * Here, the packet is copied out of the audio buffer so that nothing
+         * within the audio buffer is in use once unlocked. */
+
+        size_t length = audio_buffer->packet_size;
+        if (length > packet_buffer_size) {

Review Comment:
   Are there any conditions under which `length`/`packet_size` here could be zero that aren't caught prior to this? Any reason to check that?



##########
src/protocols/rdp/rdp.c:
##########
@@ -962,31 +964,24 @@ static int guac_rdp_handle_connection(guac_client* client) {
     if (rdp_client->active_job != NULL) {
         guac_rdp_print_job_kill(rdp_client->active_job);
         guac_rdp_print_job_free(rdp_client->active_job);
+        rdp_client->active_job = NULL;
     }
 
     /* Disconnect client and channels */
     pthread_mutex_lock(&(rdp_client->message_lock));
     freerdp_disconnect(rdp_inst);
     pthread_mutex_unlock(&(rdp_client->message_lock));
 
-    /* Stop render loop */
-    guac_display_render_thread_destroy(rdp_client->render_thread);
-    rdp_client->render_thread = NULL;
-
-    /* Ensure all background rendering processes are stopped before freeing
-     * underlying memory */
-    guac_display_stop(rdp_client->display);
-
-    /* Clean up FreeRDP internal GDI implementation (this must be done BEFORE
-     * freeing the guac_display, as freeing the GDI will free objects like
-     * rdpPointer that will attempt to free associated guac_display_layer
-     * instances during cleanup) */

Review Comment:
   Did this get moved somewhere else? Or is it, for some reason, no longer required?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org