You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@guacamole.apache.org by JonVol <jo...@gmail.com> on 2020/08/31 20:59:28 UTC
Memory reserved during file copy via virtual channel is not freed
Hi all,
I am trying to hunt down an issue where during an RDP session via Gaucamole,
copying a file from a shared directory/drive to any location on the RDP
server reserves memory that never gets freed. This leak eventually brings
down the guac session process after ~3GB have been copied in such a way (on
a weak machine).
We are getting this with Guacamole 1.2.0 - was not happening with 1.0.0.
Using valgrind we have tracked the problem to guac_rdpdr_fs_process_read
(rdpdr-fs-messages.c). Seems that the wStream object created via the call to
guac_rdpdr_new_io_completion and is subsequently passed over to freerdp in
guac_rdp_common_svc_write (common-svc.c) never gets freed by freerdp.
I could not find an open issue or discussions on it. Has anyone encountered
this problem, or in general has any idea regarding the root cause of this
leak?
Thanks! Regards,
Jonatan
--
Sent from: http://apache-guacamole-general-user-mailing-list.2363388.n4.nabble.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@guacamole.apache.org
For additional commands, e-mail: user-help@guacamole.apache.org
Re: Memory reserved during file copy via virtual channel is not
freed
Posted by JonVol <jo...@gmail.com>.
Hi, thanks for your reply!
We are using FreeRDP 2.2.0. Meanwhile we have also seen the difference
inside between this and 2.0.0, with FreeRDP 2.0.0 using Stream_Free to free
a wStream while 2.2.0 uses a StreamPool and calls Stream_Release instead.
A slightly more in-depth overview of what we found so far to be the case
with Guacamole server 1.2.0:
As part of the file upload process, we call the function
"guac_rdpdr_fs_process_read". In this function we allocate a wStream object
using the function "guac_rdpdr_new_io_completion". This function uses
"Stream_New" in order to allocate the stream. Stream_New creates a new
stream object and allocates dedicated memory for it.
Next, in the "guac_rdpdr_fs_process_read" function we call
"guac_rdp_common_svc_write" function. This function receives the allocated
stream, and passes it to a function called "pVirtualChannelWriteEx". This
function has a comment attached to it mentioning that the wStream object
will be automatically freed by FreeRDP using Stream_Free.
As we understand, this freeing is supposed to happen in the
"drdynvc_virtual_channel_open_event_ex" in FreeRDP. However, on May the
Stream_Free was replaced by Stream_Release which expects a wStream received
from a StreamPool, and tries to return it to the pool. If the wStream was
not received from a pool (which is the case for Guacamole server), the
Stream_Release function does nothing, and does not free the allocated memory
of this stream. This causes a memory leak, where none of the memory
allocated during the file uploading is freed.
If we assume that the problem is that Guacamole server 1.2.0 is not yet
adapted to these more recent changes in FreeRDP 2, could you suggest a way
to work around it, or to make the necessary change in Guacamole?
Also, would you recommend using FreeRDP 2.0.0 at this stage rather than
FreeRDP 2.2.0?
Thanks!
Jonatan
--
Sent from: http://apache-guacamole-general-user-mailing-list.2363388.n4.nabble.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@guacamole.apache.org
For additional commands, e-mail: user-help@guacamole.apache.org
Re: Memory reserved during file copy via virtual channel is not freed
Posted by Mike Jumper <mj...@apache.org>.
On Sat, Sep 5, 2020 at 11:09 PM Mike Jumper <mj...@apache.org> wrote:
> On Thu, Sep 3, 2020 at 1:49 PM alonp <al...@cyberark.com> wrote:
>
>> mjumper wrote
>> ...
>> Do you know of any reason to avoid handling the
>> "CHANNEL_EVENT_WRITE_COMPLETE", and release the stream once it received?
>>
>
> Perhaps, but:
>
> * Care needs to be taken to ensure we remain compatible with older
> releases of FreeRDP that *do* automatically free the wStream.
> * If allocating the wStream from FreeRDP's pool would result in the
> automatic free behavior, it's probably worth considering leveraging that
> wStream pool when it's available.
> * Unless there is a reason to not handle the free
> within guac_rdp_common_svc_write(), I think that would be a better place.
> * If there is a reason to not handle the free within
> guac_rdp_common_svc_write() and we cannot rely on automatic free via
> FreeRDP's internals, I would guess that the free would also need to be done
> for CHANNEL_EVENT_WRITE_CANCELED.
>
I've opened https://issues.apache.org/jira/browse/GUACAMOLE-1181 for this.
- Mike
Re: Memory reserved during file copy via virtual channel is not freed
Posted by Mike Jumper <mj...@apache.org>.
On Thu, Sep 3, 2020 at 1:49 PM alonp <al...@cyberark.com> wrote:
> mjumper wrote
> ...
> Do you know of any reason to avoid handling the
> "CHANNEL_EVENT_WRITE_COMPLETE", and release the stream once it received?
>
Perhaps, but:
* Care needs to be taken to ensure we remain compatible with older releases
of FreeRDP that *do* automatically free the wStream.
* If allocating the wStream from FreeRDP's pool would result in the
automatic free behavior, it's probably worth considering leveraging that
wStream pool when it's available.
* Unless there is a reason to not handle the free
within guac_rdp_common_svc_write(), I think that would be a better place.
* If there is a reason to not handle the free within
guac_rdp_common_svc_write() and we cannot rely on automatic free via
FreeRDP's internals, I would guess that the free would also need to be done
for CHANNEL_EVENT_WRITE_CANCELED.
- Mike
Re: Memory reserved during file copy via virtual channel is not
freed
Posted by alonp <al...@cyberark.com>.
mjumper wrote
> It's possible that the internal behavior of pVirtualChannelWriteEx() has
> changed and the wStream is no longer automatically freed. This would be
> unfortunate, but not unprecedented. We already need a version check within
> our configure.ac to test just how much will truly be freed by
> Bitmap_Free()
> [2]. It may be that another change requires the same sort of check
> for pVirtualChannelWriteEx().
>
> - Mike
Hi Mike,
We found out that "drdynvc_virtual_channel_open_event_ex" is not being
called on this flow (and we believe this is why the relevant wStreams are
not being released).
Instead - the guac registered the function:
"guac_rdp_common_svc_handle_open_event".
This function is an event handler supposed to handle the events:
"CHANNEL_EVENT_DATA_RECEIVED", "CHANNEL_EVENT_WRITE_CANCELED" and
"CHANNEL_EVENT_WRITE_COMPLETE", however only the first one is currently
handled by it ("CHANNEL_EVENT_DATA_RECEIVED").
We believe this is the cause for the leak Jonatan mentioned above.
Once adding a code section in this function that calls "Stream_Free" on the
relevant stream when "CHANNEL_EVENT_WRITE_COMPLETE" occurs, we avoided this
leak and the memory seems to be freed.
We found no issues with this behavior, and we couldn't reproduce any flow
where this fix may be problematic.
Do you know of any reason to avoid handling the
"CHANNEL_EVENT_WRITE_COMPLETE", and release the stream once it received?
Thanks,
Alon
--
Sent from: http://apache-guacamole-general-user-mailing-list.2363388.n4.nabble.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@guacamole.apache.org
For additional commands, e-mail: user-help@guacamole.apache.org
Re: Memory reserved during file copy via virtual channel is not freed
Posted by Mike Jumper <mj...@apache.org>.
On Mon, Aug 31, 2020 at 1:59 PM JonVol <jo...@gmail.com> wrote:
> ...
> Using valgrind we have tracked the problem to guac_rdpdr_fs_process_read
> (rdpdr-fs-messages.c). Seems that the wStream object created via the call
> to
> guac_rdpdr_new_io_completion and is subsequently passed over to freerdp in
> guac_rdp_common_svc_write (common-svc.c) never gets freed by freerdp.
>
What version of FreeRDP are you building against?
From the body of the function in question [1]:
/* 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() */
It's possible that the internal behavior of pVirtualChannelWriteEx() has
changed and the wStream is no longer automatically freed. This would be
unfortunate, but not unprecedented. We already need a version check within
our configure.ac to test just how much will truly be freed by Bitmap_Free()
[2]. It may be that another change requires the same sort of check
for pVirtualChannelWriteEx().
- Mike
[1]
https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/src/protocols/rdp/channels/common-svc.c#L92-L98
[2]
https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/configure.ac#L626-L648