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 2021/04/08 22:41:55 UTC
[GitHub] [guacamole-server] mike-jumper opened a new pull request #336: GUACAMOLE-1283: Add synchronization around absolutely all outbound RDP messages.
mike-jumper opened a new pull request #336:
URL: https://github.com/apache/guacamole-server/pull/336
The FreeRDP library is intended to be threadsafe, but is not reliably so with respect to legacy RDP encryption and outbound messages. When outbound messages are sent by multiple threads, the encryption key used for legacy RDP encryption may not be updated correctly, resulting in a fatal connection error like:
> "ERRINFO_DECRYPT_FAILED (0x00001192):(a) Decryption using Standard RDP Security mechanisms (section 5.3.6) failed. (b) Session key creation using Standard RDP Security mechanisms (section 5.3.5) failed."
These changes add synchronization around absolutely all parts of the RDP support that may send RDP messages, including `freerdp_check_event_handles()`.
--
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
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #336: GUACAMOLE-1283: Add synchronization around absolutely all outbound RDP messages.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #336:
URL: https://github.com/apache/guacamole-server/pull/336#discussion_r612864514
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -40,8 +51,8 @@
* The arbitrary data pointer provided when the audio buffer was
* initialized.
*/
-typedef void guac_rdp_audio_buffer_flush_handler(char* buffer, int length,
- void* data);
+typedef void guac_rdp_audio_buffer_flush_handler(guac_rdp_audio_buffer* audio_buffer,
+ char* buffer, int length, void* data);
Review comment:
OK - I've removed the redundant parameters.
--
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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #336: GUACAMOLE-1283: Add synchronization around absolutely all outbound RDP messages.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #336:
URL: https://github.com/apache/guacamole-server/pull/336#discussion_r612443475
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -40,8 +51,8 @@
* The arbitrary data pointer provided when the audio buffer was
* initialized.
*/
-typedef void guac_rdp_audio_buffer_flush_handler(char* buffer, int length,
- void* data);
+typedef void guac_rdp_audio_buffer_flush_handler(guac_rdp_audio_buffer* audio_buffer,
+ char* buffer, int length, void* data);
Review comment:
Yeah, that's fine with me - I wasn't sure if there were any situations where the function would be used to process a `buffer` and `data` that wasn't already part of `audio_buffer`? If not, I'm totally good with the above - if so, we can leave it as is. No strong preference...
--
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
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #336: GUACAMOLE-1283: Add synchronization around absolutely all outbound RDP messages.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #336:
URL: https://github.com/apache/guacamole-server/pull/336#discussion_r611095698
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -40,8 +51,8 @@
* The arbitrary data pointer provided when the audio buffer was
* initialized.
*/
-typedef void guac_rdp_audio_buffer_flush_handler(char* buffer, int length,
- void* data);
+typedef void guac_rdp_audio_buffer_flush_handler(guac_rdp_audio_buffer* audio_buffer,
+ char* buffer, int length, void* data);
Review comment:
We could, sure. The same could go for `data`, which is also part of `guac_rdp_audio_buffer`. Would you prefer something like:
```c
typedef void guac_rdp_audio_buffer_flush_handler(guac_rdp_audio_buffer* audio_buffer, int length);
```
?
--
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
[GitHub] [guacamole-server] necouchman commented on a change in pull request #336: GUACAMOLE-1283: Add synchronization around absolutely all outbound RDP messages.
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #336:
URL: https://github.com/apache/guacamole-server/pull/336#discussion_r611037614
##########
File path: src/protocols/rdp/channels/audio-input/audio-buffer.h
##########
@@ -40,8 +51,8 @@
* The arbitrary data pointer provided when the audio buffer was
* initialized.
*/
-typedef void guac_rdp_audio_buffer_flush_handler(char* buffer, int length,
- void* data);
+typedef void guac_rdp_audio_buffer_flush_handler(guac_rdp_audio_buffer* audio_buffer,
+ char* buffer, int length, void* data);
Review comment:
Is there reason to expect that there are cases when `buffer` may be outside of the `audio_buffer`? Passing pointers around isn't that expensive, but seems like if we have the entire `audio_buffer` structure we could just retrieve `buffer` from there?
--
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
[GitHub] [guacamole-server] necouchman merged pull request #336: GUACAMOLE-1283: Add synchronization around absolutely all outbound RDP messages.
Posted by GitBox <gi...@apache.org>.
necouchman merged pull request #336:
URL: https://github.com/apache/guacamole-server/pull/336
--
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
[GitHub] [guacamole-server] mike-jumper commented on pull request #336: GUACAMOLE-1283: Add synchronization around absolutely all outbound RDP messages.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #336:
URL: https://github.com/apache/guacamole-server/pull/336#issuecomment-817204881
> I'm guessing that the PubSub_SubscribeChannelConnected() calls do not need to be wrapped in the mutex lock/unlock, as they're just internal to the client and don't send any messages, correct?
Right.
--
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