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. - Mike
