From 2c86e20ab90238bc90f4979e876b3ff8a1d25e64 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 28 Oct 2020 20:23:37 -0700 Subject: [PATCH 1/3] GUACAMOLE-1181: Free wStream after send is complete/cancelled. --- src/protocols/rdp/channels/common-svc.c | 7 +++---- .../rdp/plugins/guac-common-svc/guac-common-svc.c | 13 +++++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/protocols/rdp/channels/common-svc.c b/src/protocols/rdp/channels/common-svc.c index b73dd6e1..8b076341 100644 --- a/src/protocols/rdp/channels/common-svc.c +++ b/src/protocols/rdp/channels/common-svc.c @@ -89,10 +89,9 @@ void guac_rdp_common_svc_write(guac_rdp_common_svc* svc, return; } - /* NOTE: Data sent via pVirtualChannelWriteEx MUST always be dynamically - * allocated, as it will be automatically freed using free(). If provided, - * the last parameter (user data) MUST be a pointer to a wStream, as it - * will automatically be freed by FreeRDP using Stream_Free() */ + /* NOTE: The wStream sent via pVirtualChannelWriteEx will automatically be + * freed later with a call to Stream_Free() when handling the + * corresponding write cancel/completion event. */ svc->_entry_points.pVirtualChannelWriteEx(svc->_init_handle, svc->_open_handle, Stream_Buffer(output_stream), Stream_GetPosition(output_stream), output_stream); diff --git a/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c b/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c index 91dee29b..d00af95b 100644 --- a/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c +++ b/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c @@ -28,10 +28,8 @@ #include /** - * Event handler for events which deal with data transmitted over an open SVC. - * This specific implementation of the event handler currently handles only the - * CHANNEL_EVENT_DATA_RECEIVED event, delegating actual handling of that event - * to guac_rdp_common_svc_process_receive(). + * Event handler for events which deal with data transmitted over an open SVC, + * including receipt of inbound data and completion of outbound writes. * * The FreeRDP requirements for this function follow those of the * VirtualChannelOpenEventEx callback defined within Microsoft's RDP API: @@ -83,6 +81,13 @@ static VOID guac_rdp_common_svc_handle_open_event(LPVOID user_param, DWORD open_handle, UINT event, LPVOID data, UINT32 data_length, UINT32 total_length, UINT32 data_flags) { + /* Free stream data after send is complete */ + if ((event == CHANNEL_EVENT_WRITE_CANCELLED + || event == CHANNEL_EVENT_WRITE_COMPLETE) && data != NULL) { + Stream_Free((wStream*) data, TRUE); + return; + } + /* Ignore all events except for received data */ if (event != CHANNEL_EVENT_DATA_RECEIVED) return; From 256487c95aa9c303e173324caa5a074d5fe8b7b8 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 29 Oct 2020 11:59:13 -0700 Subject: [PATCH 2/3] GUACAMOLE-1181: Only free wStream after send if FreeRDP requires this. --- configure.ac | 25 ++++++++++++++++++- .../plugins/guac-common-svc/guac-common-svc.c | 2 ++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 1873a6fb..485be581 100644 --- a/configure.ac +++ b/configure.ac @@ -623,7 +623,7 @@ then fi -# Variation in memory internal allocation/free behavior +# Variation in memory internal allocation/free behavior for bitmaps if test "x${have_freerdp2}" = "xyes" then @@ -647,6 +647,29 @@ then fi +# Variation in memory internal allocation/free behavior for channel streams +if test "x${have_freerdp2}" = "xyes" +then + + # FreeRDP 2.0.0-rc3 through 2.0.0-rc4 automatically free the wStream + # provided to pVirtualChannelWriteEx(). This changed in commit 1b78b59, and + # implementations must manually free the wStream after it is no longer + # needed (after the write is complete or cancelled). + AC_MSG_CHECKING([whether pVirtualChannelWriteEx() frees the wStream upon completion]) + AC_EGREP_CPP([\"2\\.0\\.0-(rc|dev)[3-4]\"], [ + + #include + FREERDP_VERSION_FULL + + ], + [AC_MSG_RESULT([yes])] + [AC_DEFINE([FREERDP_SVC_CORE_FREES_WSTREAM],, + [Whether pVirtualChannelWriteEx() frees the wStream upon completion])], + [AC_MSG_RESULT([no])]) + +fi + + # Glyph callback variants if test "x${have_freerdp2}" = "xyes" then diff --git a/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c b/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c index d00af95b..0a464850 100644 --- a/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c +++ b/src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c @@ -81,12 +81,14 @@ static VOID guac_rdp_common_svc_handle_open_event(LPVOID user_param, DWORD open_handle, UINT event, LPVOID data, UINT32 data_length, UINT32 total_length, UINT32 data_flags) { +#ifndef FREERDP_SVC_CORE_FREES_WSTREAM /* Free stream data after send is complete */ if ((event == CHANNEL_EVENT_WRITE_CANCELLED || event == CHANNEL_EVENT_WRITE_COMPLETE) && data != NULL) { Stream_Free((wStream*) data, TRUE); return; } +#endif /* Ignore all events except for received data */ if (event != CHANNEL_EVENT_DATA_RECEIVED) From c1ad6115a2ddf244ebb4a1b77ea211987ea3e863 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 29 Oct 2020 12:00:00 -0700 Subject: [PATCH 3/3] GUACAMOLE-1181: Warn users if the internal behavior of their version of FreeRDP cannot be tested and may be unreliable. --- configure.ac | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/configure.ac b/configure.ac index 485be581..bc283e48 100644 --- a/configure.ac +++ b/configure.ac @@ -623,6 +623,34 @@ then fi +# It is difficult or impossible to test for variations in FreeRDP behavior in +# between releases, as the change in behavior may not (yet) be associated with +# a corresponding change in version number and may not have any detectable +# effect on the FreeRDP API +if test "x${have_freerdp2}" = "xyes" +then + + AC_MSG_CHECKING([whether FreeRDP appears to be a development version]) + AC_EGREP_CPP([\"[0-9]+\\.[0-9]+\\.[0-9]+(-rc[0-9]+)?\"], [ + + #include + FREERDP_VERSION_FULL + + ], + [AC_MSG_RESULT([no])], + [AC_MSG_RESULT([yes])] + [AC_MSG_WARN([ + -------------------------------------------- + You are building against a development version of FreeRDP. Non-release + versions of FreeRDP may have differences in behavior that are impossible to + check for at build time. This may result in memory leaks or other strange + behavior. + + *** PLEASE USE A RELEASED VERSION OF FREERDP IF POSSIBLE *** + --------------------------------------------])]) + +fi + # Variation in memory internal allocation/free behavior for bitmaps if test "x${have_freerdp2}" = "xyes" then