[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
URL: https://github.com/apache/guacamole-server/pull/257#discussion_r378482869
 
 

 ##########
 File path: src/guacd/proc.c
 ##########
 @@ -267,24 +267,24 @@ static int guacd_timed_client_free(guac_client* client, int timeout) {
         .tv_nsec = current_time.tv_usec * 1000
     };
 
-    /* Free the client in a separate thread, so we can time the free operation */
-    if (pthread_create(&client_free_thread, NULL,
-                guacd_client_free_thread, &free_operation))
-        return 1;
-
     /* The mutex associated with the pthread conditional and flag MUST be
      * acquired before attempting to wait for the condition */
     if (pthread_mutex_lock(&free_operation.completed_mutex))
         return 1;
 
-    /* Wait a finite amount of time for the free operation to finish */
-    if (pthread_cond_timedwait(&free_operation.completed_cond,
-                &free_operation.completed_mutex, &deadline))
-        return 1;
+    /* Free the client in a separate thread, so we can time the free operation */
+    if (!pthread_create(&client_free_thread, NULL,
+                guacd_client_free_thread, &free_operation)) {
+
+        /* Wait a finite amount of time for the free operation to finish */
+        (void) pthread_cond_timedwait(&free_operation.completed_cond,
 
 Review comment:
   The documentation for `pthread_cond_timedwait()` that you've linked to emphasizes that the return value of the function and the predicate should be rechecked in a loop, as the wait may complete before the timeout occurs and before a signal is sent.
   
   That may cause non-deterministic free behavior, as well, and should probably be corrected here.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services