Segmentation fault when using g15 driver

[toams]

I got a segmentation fault when trying to run lcdproc on my logitech g15 keyboard.
I compiled lcdproc from latest master
I traced back the issue to this commit: 71a3c09

backtrace:

Backtrace: /lib/x86_64-linux-gnu/libg15daemon_client.so.1(+0x1393) [0x7ff17ad18393]
Backtrace: /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7ff17ab56210]
Backtrace: /lib/x86_64-linux-gnu/libg15render.so.1(g15r_renderG15Glyph+0x1e) [0x7ff17ad11dae]
Backtrace: server/drivers/g15.so(g15_chr+0xaa) [0x7ff17ad1e9e1]
Backtrace: server/drivers/g15.so(g15_string+0x3b) [0x7ff17ad1ea27]
Backtrace: server/LCDd(drivers_string+0x73) [0x565001fdcefc]
Backtrace: server/LCDd(+0xfdee) [0x565001fd9dee]
Backtrace: server/LCDd(+0x10763) [0x565001fda763]
Backtrace: server/LCDd(render_screen+0xbd) [0x565001fda8eb]
Backtrace: server/LCDd(main+0x6af) [0x565001fd4145]
Backtrace: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7ff17ab370b3]
Backtrace: server/LCDd(_start+0x2e) [0x565001fd1a2e
End of Backtrace.

[ethandicks]

I peeked at that commit - looks like they refactored the rendering routine. I do not have a G15 so I cannot test.

Are you able to do some digging to narrow down what section it's failing on?

[haraldg]

@toams how did you verify, that this commit causes the problem?
@jwrdegoede since you are the author, can you comment on this?

[jwrdegoede]

I just double checked the commit and I cannot see anything wrong with it.

I suspect this really is an issue with libg15render, @toams where does your libg15render come from? Note that both Fedora and Debian are shipping a svn snapshot of svn revision 316 which they are calling version 1.3.0 because upstream libg15render never did a proper 1.3.0 release before basically stopping all development.

Ah I remember something now. And I think I know what is going on. The definition of the g15canvas struct looks like this:

/** \brief This structure holds the data need to render objects to the LCD screen.*/
  typedef struct g15canvas
  {
/** g15canvas::buffer[] is a buffer holding the pixel data to be sent to the LCD.*/
    unsigned char buffer[G15_BUFFER_LEN];
/** g15canvas::mode_xor determines whether xor processing is used in g15r_setPixel.*/
    int mode_xor;
/** g15canvas::mode_cache can be used to determine whether caching should be used in an applic
    int mode_cache;
/** g15canvas::mode_reverse determines whether color values passed to g15r_setPixel are revers
    int mode_reverse;
#ifdef TTF_SUPPORT
    FT_Library ftLib;
    FT_Face ttf_face[G15_MAX_FACE][sizeof (FT_Face)];
    int ttf_fontsize[G15_MAX_FACE];
#endif
  } g15canvas;

Notice the #ifdef TTF_SUPPORT in there. This header actually depends on the project you are building defining this (which is stupid and the libg15render code / distro-packages really should be fixed to not depend on this).

In commit a9c0472 (which is older then commit 71a3c09) I added the following to lcdproc's server/drivers/g15.c file to deal with this:

/*
 * If we have freetype2, assume libg15render is build with TTF support,
 * the TTF_SUPPORT define makes the size of the g15 struct bigger, if we do
 * not set this define while libg15render is build with TTF support we get
 * heap corruption. The other way around does not matter, then we just alloc
 * a little bit too much memory (the TTF related variables live at the end
 * of the struct).
 */
#ifdef HAVE_FT2
#define TTF_SUPPORT
#endif
#include <libg15render.h>

So if you are using a pre-build lib15grender from your distro that likely has TTF support enabled and if your local system does not have the necessary headers installed for lcdproc's ./configure to set HAVE_FT2, then this is the likely problem.

Please check config,h in the directory where you are building lcdproc and check that it contains: #define HAVE_FT2 1 if that is not set, then that is the problem.

Malloc will typically leave some free space after the allocated object, where as by packing them into the struct there is very little free space if any behind the g15canvas struct, increasing the chances of memory corruption when TTF_SUPPORT is not defined when including the header.

Note that during my original work on this I hit a heap corruption issues because of this before the refactoring to not malloc these 2 structs. I guess the refactoring makes this problem easier to hit, which is causing you to think that commit 71a3c09 is the problem, but in reality reverting that commit really just papers over the problem and leads to crashes later.

[jwrdegoede]

@toams if the above comment does not help, then please provide some more info about your setup to help debug this, like:

1. What distribution and architecture you are seeing this on
2. Where your libg15render is coming from and which version it is
3. What keyboard you are using
4. You say that you are building lcdproc from master, can you share your config.log and config.h files please ?
5. Can you share your LCDd.conf and lcdproc.conf files please
6. Are you using any other LCDd clients other then lcdproc?
7. And anything else which might be relevant for me to try and reproduce the problem?

[jwrdegoede]

@toams, one more remark if my hunch about #define HAVE_FT2 1 not being set is wrong, also please make sure that the libg15render.h file being included and the libg15render.so file being used for linking belong to each other. You may have a distro version installed in say /usr/lib, with a locally build version in say /usr/local/lib and the build could end up using one for the .h and then ld.so may end up picking the other when runtime linking the program. The best way to avoid this is to make sure you only have one copy of each (and then the 2 files must be from the same build).

[toams]

1. KDE neon based on ubuntu 20.04
2. libg15render is installed from standard repository, version is 1.3.0~svn316-3. Only one libg15render.h and libg15render.so are found on my system. I purged all files and reinstalled them to make sure all of them are coming from standard repo and not from a local build
3. I'm using g15 keyboard version one; lsusb: Bus 001 Device 006: ID 046d:c222 Logitech, Inc. G15 Keyboard / LCD
4. config.log, config.h
5. I've made no changes yet to LCDd.conf and lcdproc.conf. I start LCDd by running: sudo LCDd -f -d g15
6. no clients yet
7. I was able to run LCDd without hitting the segfault after reverting 71a3c09. Git complained about a merge conflict, and gcc was unable to compile due to some issues with pointers, but i was able to resolve them. (I must admit I'm no programmer so I might have done something else wrong, but the code runs without crashing) see here

Reverting the commit is probably not the correct way to solve this but it might give a hint to what is wrong

I can't find any mention of have_ft2 in the config.log and the config.h but I have libfreetype-dev installed. Version 2.10.1-2ubuntu0.1. Maybe here is something wrong?

I tried debugging with gdb:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7fb2dae in g15r_renderG15Glyph () from /lib/x86_64-linux-gnu/libg15render.so.1

This makes me believe the problem is indeed with libg15render

[jwrdegoede]

If you look at your config.h you will see that HAVE_FT2 is not defined there, as I already suspected.

This means that your system is lacking the freetype2 headers / .pc files please make sure you have those installed; and then rebuild everything from scratch.

As long as you don't have HAVE_FT2 defined in your config.h you will get libg15render crashes, reverting 71a3c09 means that you are now malloc-ing a too small struct instead of embedding it in another struct, which means that you are now corrupting your heap which usually takes a bit longer to cause a crash; but you are still using the wrong size for the struct.

The only fix for this is to make sure your build environment matches the one used to build libg15render, which means that HAVE_FT2 should be defined in your config.h .

Quoting from your config.log:

configure:8125: checking if freetype support has been enabled
configure:8137: result: yes
configure:8141: WARNING: freetype does not seem to be installed

You need to fix that WARNING and then check that HAVE_FT2 is present in config.log after fixing the warning.

[haraldg]

@jwrdegoede thanks for clearing this up. I agree that libg15render does something very stupid and should be fixed.

Regarding the upstream situation there seems to be a fork now that does regular releases:
https://gitlab.com/menelkir/libg15render

However I also wonder whether we can work around this problem on the lcdproc side: Is there anything stopping us from defining TTF_SUPPORT unconditionally before including the header file?

[jwrdegoede]

@haraldg wrote:

However I also wonder whether we can work around this problem on the lcdproc side: Is there anything stopping us from defining TTF_SUPPORT unconditionally before including the header file?

Unfortunately yes there is, 2 of the extra struct g15canvas members have types defined in the freetype2 headers, so defining TTF_SUPPORT also actives including the freetype2 headers, quoting from libg15render.h :

#ifdef TTF_SUPPORT
#include <ft2build.h>
#include FT_FREETYPE_H
#include FT_BITMAP_H
#endif

And this will fail on systems without the freetype2 headers installed, that is why I made the lcdproc g15 code do things like this:

#ifdef HAVE_FT2
#define TTF_SUPPORT
#endif

The proper fix for this would be to give libg15render a libg15render.h.in which gets processed by its ./configure script and which then generates a libg15render.h without the #ifdef-s which matches the compile time config of libg15render. As such it is good to know that there is a new upstream. I'll file an issue for this with the new upstream. But since most distros ship the svn316 snapshot we will still need the workaround on the lcdproc side.

[jwrdegoede]

Done: https://gitlab.com/menelkir/libg15render/-/issues/1

[haraldg]

Thanks for the clarification.

Can we figure out the compile time config of libg15render in ./configure? If yes, then maybe we can just use this to set TTF_SUPPORT for the build?

Otherwise I think the g15 driver has an undeclared build dependency on freetype2. We could work around the issue by making the dependency explicit in autoconf.

[jwrdegoede]

Can we actually figure out the compile time config of libg15render in ./configure? If yes, then maybe we can just use this to set TTF_SUPPORT for the build?

No I'm afraid we cannot, once the upstream issue is fixed, then we should be able to detect this because then code doing #include <libg15render.h> will fail to build if the header needs the freetype2 headers and those are not installed, but with the current libg15render.h out there, there is no way to detect this.

Oh wait, I'm wrong there actually is a way, sort of. libg15render has a helper program called g15fontconvert, which to my surprise even gets build when there is no TTF support, but then the entire code of the program is this;

int main(char *argv) {
    printf("g15fontconvert - libg15render has no FreeType support compiled in.  This is requir
    return -1;
}

So we could use this, but that depends on this helper being installed, which I'm not sure all distro packages do, also see below.

Otherwise I think the g15 driver has an undeclared build dependency on freetype2. We could work around the issue by making the dependency explicit in autoconf.

Yes I agree that this would be the best thing to do, at least all distro packages of libg15render have freetype support enabled AFAIK,

Note I'm afraid I don't really have time to write a patch to add the freetype2 builddep myself, so I hope that you can take care of that.

[haraldg]

Nobody ever wants to work on our autoconf code. I myself can't do much more than intelligent copy&paste and trial&error. Also I don't really understand, why your safeguard isn't enough: If the distros compile libg15render against freetype2, don't they make libg15render-dev depend on libfreetype2-dev, so that HAVE_FT2 should always be defined when building the g15 driver?

But yes, I can provide a PR for you and @toams to test and improve upon.

[toams]

The thing that confuses me right now is that autoconf doesnt detect freetype being installed on my system. I searched for freetype.h and freetype.pc and they are both installed.:

/usr/include/freetype2/freetype/freetype.h
/usr/lib/x86_64-linux-gnu/pkgconfig/freetype2.pc

but ./configure doesnt detect them

I will try #190 later this evening

[toams]

Nice to see this being resolved so quickly,
I didn't expect this project to be so quick to respond and so eager to solve this issue.
Many thanks to all involved!