You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "aleitner (via GitHub)" <gi...@apache.org> on 2024/03/09 01:24:03 UTC
[PR] GUACAMOLE-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
aleitner opened a new pull request, #496:
URL: https://github.com/apache/guacamole-server/pull/496
Resolve authentication timeout and deadlock issues. There was a timeout occurring approximately 12 seconds into an RDP connection when using NLA and resources were not properly being cleaned up.
To address this, `guac_argv_stop` has been introduced. It ensures that when a client disconnects, any threads waiting for user input (such as username or password), can effectively abort their wait, preventing the connection from hanging indefinitely.
A deadlock has also been mitigated by modifying the lock state to allow read access to other threads when it is safe to do so.
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#issuecomment-1992729261
Hey @aleitner this looks like a good candidate for the upcoming 1.5.5 release. It's definitely a bug fix. Want to target it at `staging/1.5.5` instead?
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#discussion_r1520207158
##########
src/protocols/rdp/rdp.c:
##########
@@ -525,12 +525,24 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Set default pointer */
guac_common_cursor_set_pointer(rdp_client->display->cursor);
+ /*
+ * Downgrade the lock to allow for concurrent read access
+ * Access to read locks need to be made available for other processes to use
+ * while we await credentials from the user.
+ */
+ pthread_rwlock_unlock(&(rdp_client->lock));
Review Comment:
Maybe it would make sense to guard just that one assignment using the write lock? Possibly using a `guac_rwlock`?
Thoughts @mike-jumper?
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#discussion_r1520514157
##########
src/protocols/rdp/rdp.c:
##########
@@ -525,12 +525,24 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Set default pointer */
guac_common_cursor_set_pointer(rdp_client->display->cursor);
+ /*
+ * Downgrade the lock to allow for concurrent read access
+ * Access to read locks need to be made available for other processes to use
+ * while we await credentials from the user.
+ */
+ pthread_rwlock_unlock(&(rdp_client->lock));
Review Comment:
Sorry, I was unclear here. What I meant was using `guac_rwlock` to convert the entire `rdp_client->lock` into a reentrant rwlock, so you could keep the current read lock around the `freerdp_connect()` call, and then additionally grab the write lock, but only when the `rdp_client->audio_input` (or any assignment) is done.
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "aleitner (via GitHub)" <gi...@apache.org>.
aleitner commented on code in PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#discussion_r1520517030
##########
src/protocols/rdp/rdp.c:
##########
@@ -525,12 +525,24 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Set default pointer */
guac_common_cursor_set_pointer(rdp_client->display->cursor);
+ /*
+ * Downgrade the lock to allow for concurrent read access
+ * Access to read locks need to be made available for other processes to use
+ * while we await credentials from the user.
+ */
+ pthread_rwlock_unlock(&(rdp_client->lock));
Review Comment:
Oh I just looked through the guac_rwlock api and that seems much cleaner. I can try and implement that
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "aleitner (via GitHub)" <gi...@apache.org>.
aleitner commented on code in PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#discussion_r1520512042
##########
src/protocols/rdp/rdp.c:
##########
@@ -525,12 +525,24 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Set default pointer */
guac_common_cursor_set_pointer(rdp_client->display->cursor);
+ /*
+ * Downgrade the lock to allow for concurrent read access
+ * Access to read locks need to be made available for other processes to use
+ * while we await credentials from the user.
+ */
+ pthread_rwlock_unlock(&(rdp_client->lock));
Review Comment:
On one hand I like the idea of adding a separate lock for around setting the audio handler, but on the other hand I'm not keen on nesting more locks
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#discussion_r1522242112
##########
src/protocols/rdp/rdp.c:
##########
@@ -525,12 +525,24 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Set default pointer */
guac_common_cursor_set_pointer(rdp_client->display->cursor);
+ /*
+ * Downgrade the lock to allow for concurrent read access
+ * Access to read locks need to be made available for other processes to use
+ * while we await credentials from the user.
+ */
+ pthread_rwlock_unlock(&(rdp_client->lock));
Review Comment:
Cool, looks good. I realize now that this doesn't _really_ require reentrancy (acquiring the write lock while holding the read lock, as you're doing now is equivalent to just releasing the read lock and acquiring the write lock).
But it certainly won't hurt to use the reentrant locks either.
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner commented on code in PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#discussion_r1520205304
##########
src/protocols/rdp/rdp.c:
##########
@@ -525,12 +525,24 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Set default pointer */
guac_common_cursor_set_pointer(rdp_client->display->cursor);
+ /*
+ * Downgrade the lock to allow for concurrent read access
+ * Access to read locks need to be made available for other processes to use
+ * while we await credentials from the user.
+ */
+ pthread_rwlock_unlock(&(rdp_client->lock));
Review Comment:
I like this approach, but I think there's at least one assignment that should probably still be guarded by the write lock - namely:
https://github.com/apache/guacamole-server/blob/main/src/protocols/rdp/rdp.c#L112
The lock itself is a bit unclear on exactly what it's guarding, and it _might_ be ok to have this particular assignment unguarded since it's only set once, so the only stale value that any other thread/process could read would be `NULL`.
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "jmuehlner (via GitHub)" <gi...@apache.org>.
jmuehlner merged PR #496:
URL: https://github.com/apache/guacamole-server/pull/496
--
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-1732: Prevent deadlock and enhance cleanup in RDP client. [guacamole-server]
Posted by "aleitner (via GitHub)" <gi...@apache.org>.
aleitner commented on PR #496:
URL: https://github.com/apache/guacamole-server/pull/496#issuecomment-1992751549
> Hey @aleitner this looks like a good candidate for the upcoming 1.5.5 release. It's definitely a bug fix. Want to target it at `staging/1.5.5` instead?
No problem!
--
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