You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/02/12 14:21:20 UTC
[GitHub] [guacamole-server] lyind opened a new pull request #257:
GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
lyind opened a new pull request #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
URL: https://github.com/apache/guacamole-server/pull/257
Fix for GUACAMOLE-958.
Two separate problems:
1. Race condition may lead to unwarranted wait and then timeout:
`guacd_timed_client_free()` raced with `guacd_client_free_thread()` for signalling on `free_operation.completed_cond`
2. Potential deadlock/hang of the thread `client_free_thread` if `pthread_cond_timedwait()` failed early or `guacd_client_free_thread()` took longer to reach it's signalling part than the timeout. The reason is that `pthread_cond_timedwait()` re-acquires the mutex before returning 0 or ETIMEDOUT and doesn't ever release it on other errors. Please refer to the man page.
Reference:
http://man7.org/linux/man-pages/man3/pthread_cond_timedwait.3p.html
----------------------------------------------------------------
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
[GitHub] [guacamole-server] lyind commented on a change in pull request
#257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
lyind 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_r378807355
##########
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:
I also noted that statement on the man page and thought about it carefully.
I think that the while-loop is not necessary for our use-case:
`pthread_mutex_lock()` (and a few other functions) are implicit memory barriers which will enforce synchronization of the state variable (`free_operation.completed`).
Thus I chose to avoid introducing unnecessary code complexity.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] lyind commented on a change in pull request
#257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
lyind 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_r378808997
##########
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:
<del>
Looking at the code again I believe we should put the `free_operation->completed = 1;` above the mutex lock in `guacd_client_free_thread()` to make sure our load/store barrier works.
In the unlikely case where the free-thread sets completed to 1 after a timeout, we don't care about a possible race-condition (returning a late success is better than returning a timeout).
</del>
Reading this:
https://stackoverflow.com/questions/3208060/does-guarding-a-variable-with-a-pthread-mutex-guarantee-its-also-not-cached
The initial commimt was fine. I will revert the second one and force-push.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] mike-jumper merged pull request #257:
GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
mike-jumper merged pull request #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
URL: https://github.com/apache/guacamole-server/pull/257
----------------------------------------------------------------
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
[GitHub] [guacamole-server] lyind commented on a change in pull request
#257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
lyind 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_r378808997
##########
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:
<del>
Looking at the code again I believe we should put the `free_operation->completed = 1;` above the mutex lock in `guacd_client_free_thread()` to make sure our load/store barrier works.
In the unlikely case where the free-thread sets completed to 1 after a timeout, we don't care about a possible race-condition (returning a late success is better than returning a timeout).
</del>
Reading this:
https://stackoverflow.com/questions/3208060/does-guarding-a-variable-with-a-pthread-mutex-guarantee-its-also-not-cached
The initial commit was fine. I will revert the second one and force-push.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by 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
[GitHub] [guacamole-server] lyind edited a comment on issue #257:
GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
lyind edited a comment on issue #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
URL: https://github.com/apache/guacamole-server/pull/257#issuecomment-585336284
I run guacd with this patch and observed no issues so far.
Guacd/client termination timing seems to be more predictable now.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] mike-jumper commented on a change in pull
request #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by 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_r379014428
##########
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:
I don't think the issue with `pthread_cond_timedwait()` and using a loop deals specifically with memory barriers, but rather with the potential for the wait to complete before the expected change is actually made:
>
> ### Condition Wait Semantics
>
> It is important to note that when `pthread_cond_wait() and `pthread_cond_timedwait()` return without error, the associated predicate may still be false. Similarly, when `pthread_cond_timedwait()` returns with the timeout error, the associated predicate may be true due to an unavoidable race between the expiration of the timeout and the predicate state change.
>
> The application needs to recheck the predicate on any return because it cannot be sure there is another thread waiting on the thread to handle the signal, and if there is not then the signal is lost. The burden is on the application to check the predicate.
>
> Some implementations, particularly on a multi-processor, may sometimes cause multiple threads to wake up when the condition variable is signaled simultaneously on different processors.
>
> In general, whenever a condition wait returns, the thread has to re-evaluate the predicate associated with the condition wait to determine whether it can safely proceed, should wait again, or should declare a timeout. A return from the wait does not imply that the associated predicate is either true or false.
>
> It is thus recommended that a condition wait be enclosed in the equivalent of a "while loop" that checks the predicate.
>
That said, I think I agree that it's not needed in our case, as the requirement centers around multiple threads waiting on the condition, whereas in our case we can be absolutely certain that there is only ever one such thread.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] lyind commented on a change in pull request
#257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
lyind 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_r378808997
##########
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:
Looking at the code again I believe we should put the `free_operation->completed = 1;` above the mutex lock in `guacd_client_free_thread()` to make sure our load/store barrier works.
In the unlikely case where the free-thread sets completed to 1 after a timeout, we don't care about a possible race-condition (returning a late success is better than returning a timeout).
----------------------------------------------------------------
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
[GitHub] [guacamole-server] lyind commented on issue #257: GUACAMOLE-958:
Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
lyind commented on issue #257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
URL: https://github.com/apache/guacamole-server/pull/257#issuecomment-585336284
I run guacd with this patch and observed no issues so far.
Guacd/client termination timing seems to be more deterministic now.
----------------------------------------------------------------
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
[GitHub] [guacamole-server] lyind commented on a change in pull request
#257: GUACAMOLE-958: Avoid race/deadlock in guacd_timed_client_free()
Posted by GitBox <gi...@apache.org>.
lyind 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_r378808997
##########
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:
<del>
Looking at the code again I believe we should put the `free_operation->completed = 1;` above the mutex lock in `guacd_client_free_thread()` to make sure our load/store barrier works.
In the unlikely case where the free-thread sets completed to 1 after a timeout, we don't care about a possible race-condition (returning a late success is better than returning a timeout).
</del>
Reading these:
https://stackoverflow.com/questions/3208060/does-guarding-a-variable-with-a-pthread-mutex-guarantee-its-also-not-cached
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
The initial commit was fine. I will revert the second one and force-push.
----------------------------------------------------------------
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