[freetype2-iu-dev] setenv() crashes rxvt-unicode (and maybe other things)

[estar]

In 03-aflatin.c.patch and 05-ftobjs.c.patch, setenv() is used to set CUR_WIDTH. This code ends up in the Freetype library and should not be using setenv(), because applications linked with libfreetype.so may alter the environment using their own functions, in ways incompatible with setenv(). Cf. the POSIX specification:

If the application modifies environ or the pointers to which it points, the behavior of setenv() is undefined.

In rxvt-unicode, the daemon program urxvtd changes (resets, in fact) **environ using its own data structures and functions in order to spawn terminal windows, so it is no longer possible to use setenv() in this context. Doing so – as happens when using urxvtd linked against a patched libfreetype.so – leads to eventual heap corruption in urxvtd, causing a crash. The same behaviour is likely in any other program that makes heavy use of **environ.

Why is it necessary to set an environment variable (as admitted in the patches, ‘a shameful hack’)? Wouldn’t an int freetype_cur_width; in ftsmooth.c with extern int freetype_cur_width; in ftobjs.c and aflatin.c suffice?

This is an issue affecting multiple users, cf. [https://bugs.gentoo.org/show_bug.cgi?id=435732](Gentoo bug 435732), [http://www.infinality.net/forum/viewtopic.php?f=2&t=324](a thread on infinality.net) and [https://bbs.archlinux.org/viewtopic.php?id=156748](a thread in the Arch Linux forum).

[goddesse]

There's no reason it needs to use an environment variable. I think infinality just didn't know about extern and static. I'll make the change unless @estar is planning on submitting a pull request.

[bohoomil]

@estar Thank you for reminding us of this. As an Urxvt user, I've been affected by the bug for quite a while.

@goddesse I'd appreciate your contribution. As soon as there's a proposed fixed, I'll update upstream patch and start testing it.

[estar]

Thanks to both of you for taking care of this. @goddesse, a note on commit c42f9912fba6df83314f03facd6affc032073c3b: is the code which formerly effected CUR_WIDTH=0 in ftobjs.c not needed at all? Just wondering that you removed it entirely.

[goddesse]

All that did was define and initialize the CUR_WIDTH environment variable to zero. I do that all in aflatin.h and aflatin.c now so it's no longer needed.

The new global might logically belong in a different place, honestly. The only places that used CUR_WIDTH were aflatin and ftsmooth (which already includes aflatin) so I opted to keep the current dependencies the same.

[estar]

Yes, but it did that at every FT_Load_Glyph(); your code only initialises the variable once, at library load, and does not reset it between FT_Load_Glyph() invocations unless the code in aflatin.c is actually called, as far as I understand. Is that safe for the code in ftsmooth.c when loading glyphs of different kinds?

Also, on a general note, I’m guessing the entire mechanism isn’t thread-safe, but Freetype is supposed to be… But that’ll depend on finding a place where to pass this value in a saner manner.

[goddesse]

You're right. I'll add back the code to re-zero it in ftobj's FT_Load_Glyph so it won't interfere with rendering for non-latin glyphs.

[bohoomil]

I updated the dev branch, including the upstream patch, and the library builds seamlessly. @estar & @goddesse -- thanks once again for your contribution.