Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NXdixfonts.c: fix memory leak #949

Merged
merged 1 commit into from Nov 3, 2020

Conversation

uli42
Copy link
Member

@uli42 uli42 commented Oct 18, 2020

Please have a closer look at this before merging!

==15332== 2,500 (96 direct, 2,404 indirect) bytes in 6 blocks are definitely lost in loss record 324 of 342
==15332== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15332== by 0x5748B9E: FontFileStartListFonts (in /usr/lib/x86_64-linux-gnu/libXfont.so.1.4.1)
==15332== by 0x5748C4A: FontFileStartListFontsAndAliases (in /usr/lib/x86_64-linux-gnu/libXfont.so.1.4.1)
==15332== by 0x42859A: nxdoListFontsAndAliases (NXdixfonts.c:1163)
==15332== by 0x42C0E0: nxOpenFont (NXdixfonts.c:1541)
==15332== by 0x43392E: ProcOpenFont (NXdispatch.c:902)
==15332== by 0x434585: Dispatch (NXdispatch.c:482)
==15332== by 0x40EF77: main (main.c:355)

FontFileStartListFontsAndAliases() allocates some private data. This
data is used by subsequent calls of FontFileListNextFontOrAlias() in a
loop. (Only) the last call to that function will free() the private
data and return with BadFontName. FontFileListNextFontOrAlias() is
the only libXfont function that free()s the private data.

In nxagent the loop is exited as soon as a font exists both locally
and remote. Therefore the private data would never be free()d.

Solution: do not break the loop but store the first matching result
and let the loop run to the end, ignoring all following results.

Disadvantage: this can mean hundreds of extra iterations for
nothing. I have done no investigation of the time penalty this might
cause.

Unfortunately this is the only clean way I have found so far.

An unclean solution has also been implemented. It can be activated by
defining BREAK_XFONT_LOOP. In that case the private data is handled in
nxagent by taking assumptions about its structure (taken from the
libXfont source). That will break if libXfont changes its internal
handling of the private. Therefore it is discouraged.

An third alternative would be to drop using libXfont from the
system. Instead fork libXfont to the nx-libs tree, add some patches
link to that library statically.

Fixes #586

@uli42 uli42 requested review from sunweaver and Ionic October 18, 2020 00:14
@uli42 uli42 changed the title NXdixfonts.c: free memory leak NXdixfonts.c: fix memory leak Oct 18, 2020
@uli42 uli42 force-pushed the pr/font_memleak branch 2 times, most recently from b70a74c to 215f496 Compare October 18, 2020 00:34
Copy link
Member

@sunweaver sunweaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, indeed. And good analysis of the possible solutions for fixing it.

I have researched a little in libXfont (master) Git and found that this struct has been added in 2003 and was never changed since then:

^153e8da (Kaleb Keithley     2003-11-14 15:54:40 +0000  793) typedef struct _LFWIData {
^153e8da (Kaleb Keithley     2003-11-14 15:54:40 +0000  794)     FontNamesPtr    names;
^153e8da (Kaleb Keithley     2003-11-14 15:54:40 +0000  795)     int                   current;
^153e8da (Kaleb Keithley     2003-11-14 15:54:40 +0000  796) } LFWIDataRec, *LFWIDataPtr;
commit 153e8da44452905ae04a0e20ad0d85f40399b4ca (tag: XORG-MAIN)
Author: Kaleb Keithley <kaleb@freedesktop.org>
Date:   Fri Nov 14 15:54:40 2003 +0000

    R6.6 is the Xorg base-line

So, I think we are safe to go the "define BREAK_XFONT_LOOP" pathway...

I will merge this PR now, shall I hard-code the '#define'?

==15332== 2,500 (96 direct, 2,404 indirect) bytes in 6 blocks are definitely lost in loss record 324 of 342
==15332==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15332==    by 0x5748B9E: FontFileStartListFonts (in /usr/lib/x86_64-linux-gnu/libXfont.so.1.4.1)
==15332==    by 0x5748C4A: FontFileStartListFontsAndAliases (in /usr/lib/x86_64-linux-gnu/libXfont.so.1.4.1)
==15332==    by 0x42859A: nxdoListFontsAndAliases (NXdixfonts.c:1163)
==15332==    by 0x42C0E0: nxOpenFont (NXdixfonts.c:1541)
==15332==    by 0x43392E: ProcOpenFont (NXdispatch.c:902)
==15332==    by 0x434585: Dispatch (NXdispatch.c:482)
==15332==    by 0x40EF77: main (main.c:355)

FontFileStartListFonts[AndAliases]() allocates some private data. This
data is used by subsequent calls of FontFileListNextFontOrAlias() in a
loop. (Only) the last call to that function will free() the private
data and return with BadFontName.  FontFileListNextFontOrAlias() is
the only libXfont function that free()s the private data.

In nxagent the loop is exited as soon as a font exists both locally
and remote. Therefore the private data would never be free()d.

Solution: do not break the loop but store the first matching result
and let the loop run to the end, ignoring all following results.

Disadvantage: this can mean hundreds of extra iterations for
nothing. I have done no investigation of the time penalty this might
cause.

Unfortunately this is the only clean way I have found so far.

An unclean solution has also been implemented. It can be activated by
defining BREAK_XFONT_LOOP. In that case the private data is handled in
nxagent by taking assumptions about its structure (taken from the
libXfont source). That will break if libXfont changes its internal
handling of the private. Therefore it is discouraged.

An third alternative would be to drop using libXfont from the
system. Instead fork libXfont to the nx-libs tree, add some patches
link to that library statically.

Fixes ArcticaProject#586
@sunweaver sunweaver merged commit ced973e into ArcticaProject:3.6.x Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

memleak in font handling
2 participants