You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by gabriel sztejnworcel <ga...@gmail.com> on 2020/09/08 21:08:50 UTC
Memory leak in RDP clipboard redirection
Hi,
I think we found a memory leak, I wanted to check with you and consult
about the fix.
We ran valgrind to check for memory leaks and noticed that each time we
copy something through the clipboard, we get a 262144 bytes memory leak in
the function guac_rdp_cliprdr_format_data_request. The leak comes from the
"output" buffer, which is allocated in the function and later passed to a
FreeRDP function through the cliprdr->ClientFormatDataResponse pointer. The
buffer is not freed, and it doesn't seem like it's supposed to be freed in
the FreeRDP function (since they accept the structure that contains the
buffer as a const pointer).
The fix I suggest (in guac_rdp_cliprdr_format_data_request):
- char* output = malloc(GUAC_RDP_CLIPBOARD_MAX_LENGTH);
+ char* buf = malloc(GUAC_RDP_CLIPBOARD_MAX_LENGTH);
+ char* output = buf; // output pointer will change so we store the
original value in buf
- return cliprdr->ClientFormatDataResponse(cliprdr, &data_response);
+ UINT result = cliprdr->ClientFormatDataResponse(cliprdr, &data_response);
+ free(buf);
+ return result;
Your thoughts?
Thanks,
Gabriel
Re: Memory leak in RDP clipboard redirection
Posted by Mike Jumper <mj...@apache.org>.
On Tue, Sep 8, 2020 at 4:07 PM Mike Jumper <mj...@apache.org> wrote:
> On Tue, Sep 8, 2020 at 2:09 PM gabriel sztejnworcel <ga...@gmail.com>
> wrote:
>
>> Hi,
>>
>> I think we found a memory leak, I wanted to check with you and consult
>> about the fix.
>>
>> We ran valgrind to check for memory leaks and noticed that each time we
>> copy something through the clipboard, we get a 262144 bytes memory leak in
>> the function guac_rdp_cliprdr_format_data_request. The leak comes from the
>> "output" buffer, which is allocated in the function and later passed to a
>> FreeRDP function through the cliprdr->ClientFormatDataResponse pointer.
>> The
>> buffer is not freed, and it doesn't seem like it's supposed to be freed in
>> the FreeRDP function (since they accept the structure that contains the
>> buffer as a const pointer).
>>
>>
> Yes, I think you're correct.
>
> The fix I suggest (in guac_rdp_cliprdr_format_data_request):
>>
>> - char* output = malloc(GUAC_RDP_CLIPBOARD_MAX_LENGTH);
>>
>> + char* buf = malloc(GUAC_RDP_CLIPBOARD_MAX_LENGTH);
>> + char* output = buf; // output pointer will change so we store the
>> original value in buf
>>
>
> There already is such a pointer:
>
>
> https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/src/protocols/rdp/channels/cliprdr.c#L356
>
> - return cliprdr->ClientFormatDataResponse(cliprdr, &data_response);
>>
>> + UINT result = cliprdr->ClientFormatDataResponse(cliprdr,
>> &data_response);
>> + free(buf);
>> + return result;
>>
>> Your thoughts?
>>
>
> I agree that we should manually free() the buffer after
> ClientFormatDataResponse() returns. It would be good to also double-check
> whether there are any other similar failures to free memory allocated for
> an outbound PDU that is handed off to FreeRDP but not actually freed
> internally by FreeRDP.
>
I've opened https://issues.apache.org/jira/browse/GUACAMOLE-1182 for this.
- Mike
Re: Memory leak in RDP clipboard redirection
Posted by Mike Jumper <mj...@apache.org>.
On Tue, Sep 8, 2020 at 2:09 PM gabriel sztejnworcel <ga...@gmail.com>
wrote:
> Hi,
>
> I think we found a memory leak, I wanted to check with you and consult
> about the fix.
>
> We ran valgrind to check for memory leaks and noticed that each time we
> copy something through the clipboard, we get a 262144 bytes memory leak in
> the function guac_rdp_cliprdr_format_data_request. The leak comes from the
> "output" buffer, which is allocated in the function and later passed to a
> FreeRDP function through the cliprdr->ClientFormatDataResponse pointer. The
> buffer is not freed, and it doesn't seem like it's supposed to be freed in
> the FreeRDP function (since they accept the structure that contains the
> buffer as a const pointer).
>
>
Yes, I think you're correct.
The fix I suggest (in guac_rdp_cliprdr_format_data_request):
>
> - char* output = malloc(GUAC_RDP_CLIPBOARD_MAX_LENGTH);
>
> + char* buf = malloc(GUAC_RDP_CLIPBOARD_MAX_LENGTH);
> + char* output = buf; // output pointer will change so we store the
> original value in buf
>
There already is such a pointer:
https://github.com/apache/guacamole-server/blob/382d72a26a04008aa936680deed00e3a31086b1e/src/protocols/rdp/channels/cliprdr.c#L356
- return cliprdr->ClientFormatDataResponse(cliprdr, &data_response);
>
> + UINT result = cliprdr->ClientFormatDataResponse(cliprdr, &data_response);
> + free(buf);
> + return result;
>
> Your thoughts?
>
I agree that we should manually free() the buffer after
ClientFormatDataResponse() returns. It would be good to also double-check
whether there are any other similar failures to free memory allocated for
an outbound PDU that is handed off to FreeRDP but not actually freed
internally by FreeRDP.
- Mike