GUACAMOLE-1732: Prevent deadlock and enhance cleanup in RDP client. #496
Conversation
src/protocols/rdp/rdp.c
Outdated
| * 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)); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Maybe it would make sense to guard just that one assignment using the write lock? Possibly using a guac_rwlock?
Thoughts @mike-jumper?
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this comment.
Oh I just looked through the guac_rwlock api and that seems much cleaner. I can try and implement that
There was a problem hiding this 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.
aleitner
force-pushed
the
GUACAMOLE-1732-rdp-nla-timeout
branch
from
2cd233c
to
41a1308
Compare
|
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 |
aleitner
force-pushed
the
GUACAMOLE-1732-rdp-nla-timeout
branch
from
41a1308
to
e724d03
Compare
aleitner
force-pushed
the
GUACAMOLE-1732-rdp-nla-timeout
branch
from
e724d03
to
19df34b
Compare
No problem! |
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_stophas 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.