search

sergei-mironov / xkb-switch

Switch your X keyboard layouts from the command line
MIT License
345 stars 37 forks source link

Memory leak due to missing deallocator #31

Open sergei-mironov opened 5 years ago

sergei-mironov commented 5 years ago

Ref https://github.com/ierton/xkb-switch/pull/30

sergei-mironov commented 5 years ago

Status:

  1. Merged workaround #30
  2. Waiting for upstream reply for https://bugs.freedesktop.org/show_bug.cgi?id=110696 https://gitlab.freedesktop.org/xorg/lib/libxkbfile/issues/6

Note meaningful comment by @szaszm https://github.com/ierton/xkb-switch/pull/30#issuecomment-493545416

alexandersolovyov commented 5 years ago

Actually, all needed functions are already there. It can be done in two ways:

  1. XkbRf_getNamesProp() can receive XkbRF_VarDefsPtr instead of reference toXkbRF_VarDefsRec. Then, memory must be cleared with XkbRF_Free(vdr, True).
  2. If You are using XkbRF_GetNamesProp() with a reference toXkbRF_VarDefsRec, memory should be returned using XkbRF_Free(&vdr, False).

libxkbfile is written in C, so they can not add any destructor. It is done by analogy with X library, where function XFree() must be used to return memory for all entities created by functions of this library.

You can see it yourself, search for XkbRF_Free and XkbRf_getNamesProp in source code: XKBrules.h maprules.c

Also be attentive when functions allocates memory for other pointers ;-)

Maybe I will do next pull request to close this issue soon.

sergei-mironov commented 5 years ago

Good news, XkbRF_Free looks like a correct solution. Thank you for the information. I'll be happy to accept the PR.

alexandersolovyov commented 5 years ago

I made a mistake here. XkbRF_Free is made only for XkbRF_RulesPtr, not for XkbRF_VarDefsPtr. The best decision for freeing XkbRF_VarDefsPtr is already applied.

sergei-mironov commented 4 years ago

OK. Related sources for reference is here https://github.com/mattl/opensource.apple.com/blob/master/src/X11libs/X11libs-60.2/libxkbfile/libxkbfile-1.0.7/include/X11/extensions/XKBrules.h#L170