NoakPalander commented 2 years ago

Running, built with gcc/std=C99

context_t context;
context.xdo = xdo_new(NULL); // (1)
// initialize the other fields, none of them are heap allocated

xdo_free(context.xdo); // !(1)

which I assume is supposedly correct yields the following with asan:

=================================================================
==59319==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 72 byte(s) in 1 object(s) allocated from:
    #0 0x7f97ddd03fb9 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x7f97ddb62ec5 in XkbGetMap (/usr/lib/libX11.so.6+0x90ec5)

SUMMARY: AddressSanitizer: 72 byte(s) leaked in 1 allocation(s).

is this an xdo-specific issue or is it an internal leak by X11 that is beyond this library to deal with?

FascinatedBox commented 2 years ago

This is caused by xdo.c using XkbFreeClientMap instead of XkbFreeKeyboard. Merging #190 will fix it.

jordansissel commented 2 years ago

190 merged 👍

or is it an internal leak by X11

It's hard to say. The documentation for XkbGetMap() says that calling XkbFreeClientMap is the correct way to free the memory, but as you report, it doesn't seem to behave this way. #190 should fix this and is now merged.

jordansissel commented 2 years ago

@NoakPalander I believe #190 fixes this problem. As mentioned, I think this might be a bug in the X11 xkbcommon library even though xdotool can work around it. Here's my sample code that isolates it to just X11/xkbcommon:

This program will call XkbGetMap and attempt to release resources using XkbFreeClientMap or XkbFreeKeyboard depending on the first command-line argument:

#include <X11/Xlib.h>
#include <stdio.h>
#include <xkbcommon/xkbcommon.h>
#include <X11/XKBlib.h>
#include <strings.h>

int main(int argc, char *argv[]) {
  Display *dpy = XOpenDisplay(NULL);

  XkbDescPtr desc = XkbGetMap(dpy, XkbAllClientInfoMask, XkbUseCoreKbd);

  if (argc > 1 && strcasecmp(argv[1], "xkbfreekeyboard") == 0) {
    printf("Using XkbFreeKeyboard\n");
    XkbFreeKeyboard(desc, 0, 1);
  } else {
    printf("Using XkbFreeClientMap\n");
    XkbFreeClientMap(desc, 0, 1);
  }

  XCloseDisplay(dpy);
}

Compiling:

% gcc -o xkbgetmaptest -g xkbgetmap.c -lX11 -lxkbcommon

Test with valgrind:

# Using XkbFreeClientMap
% valgrind --leak-check=full ./xkbgetmaptest
Using XkbFreeClientMap
...
==2625== 72 bytes in 1 blocks are definitely lost in loss record 1 of 1
==2625==    at 0x484DA83: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2625==    by 0x48F5264: XkbGetMap (in /usr/lib/x86_64-linux-gnu/libX11.so.6.4.0)
==2625==    by 0x10923F: main (xkbgetmap.c:10)

# Using XkbFreeKeyboard
% valgrind --leak-check=full ./xkbgetmaptest xkbfreekeyboard
Using XkbFreeKeyboard
==2638== HEAP SUMMARY:
==2638==     in use at exit: 0 bytes in 0 blocks
==2638==   total heap usage: 112 allocs, 112 frees, 87,031 bytes allocated
==2638==
==2638== All heap blocks were freed -- no leaks are possible

jordansissel commented 2 years ago

Ahh. Older versions of the XkbGetMap documentation erroneously stated that XkbFreeClietnMap() should be invoked to free things, but 7-8 years ago the X11 documentation was updated to fix this.

The original patch (also about 7 years ago) adding the call to XkbFreeClientMap was b8f01098 likely used the upstream X11 documentation which told us to use the wrong function. Oops!

Reference: https://gitlab.freedesktop.org/xorg/lib/libx11/-/commit/d6bd988bc00494914b38b95ee5df77ac4f32f19f

jordansissel / xdotool

Small leakage in xdo or in X11? #385

190 merged 👍