On Tue, Sep 8, 2020 at 4:07 PM Mike Jumper <[email protected]> wrote:
> On Tue, Sep 8, 2020 at 2:09 PM gabriel sztejnworcel <[email protected]> > 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
