SVN 5179 crash to desktop loading font

[spikethehobbit]

font is default/DejaVuSans-Bold.ttf

GG/src/Font.cpp @ 1344

// create opengl texture from buffer(s) and release buffer(s)
for (std::size_t i = 0; i < buffer_vec.size(); ++i) {
boost::shared_ptr<Texture> temp_texture(new Texture);
temp_texture->Init(X0, Y0, buffer_sizes.x, buffer_sizes.y, BUF_WIDTH, (unsigned char*)(buffer_vec), GL_LUMINANCE_ALPHA, GL_UNSIGNED_BYTE, 2);
m_textures.push_back(temp_texture);
delete [] buffer_vec; // <- crashes here
}

*** glibc detected *** ./freeorion: free(): invalid next size (normal): 0x000000000376a470 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x75b46)[0x7f001315fb46]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x6c)[0x7f001316487c]
/usr/lib/libGiGi.so(_ZN2GG4Font4InitERP11FT_FaceRec_+0xf5e)[0x7f0018c30af2]
./freeorion(_ZN2GG4FontC1IN9__gnu_cxx17__normal_iteratorIPKNS_14UnicodeCharsetESt6vectorIS4_SaIS4_EEEEEERKSsjT_SD_+0x1b0)[0x176400a]
./freeorion(_ZN2GG11FontManager11GetFontImplIN9__gnu_cxx17__normal_iteratorIPKNS_14UnicodeCharsetESt6vectorIS4_SaIS4_EEEEEEN5boost10shared_ptrINS_4FontEEERKSsjPKS7_IhSaIhEET_SL_+0x149)[0x17618e7]
./freeorion(_ZN2GG11FontManager7GetFontIN9__gnu_cxx17__normal_iteratorIPKNS_14UnicodeCharsetESt6vectorIS4_SaIS4_EEEEEEN5boost10shared_ptrINS_4FontEEERKSsjT_SH_+0x4b)[0x175f347]
./freeorion(_ZN2GG3GUI7GetFontIN9__gnu_cxx17__normal_iteratorIPKNS_14UnicodeCharsetESt6vectorIS4_SaIS4_EEEEEEN5boost10shared_ptrINS_4FontEEERKSsjT_SH_+0x48)[0x175d800]
./freeorion(_ZN8ClientUI11GetBoldFontEi+0x93)[0x1758c4f]
./freeorion(_ZN17IconTextBrowseWndC1EN5boost10shared_ptrIN2GG7TextureEEERKSsS6_+0x14b)[0x188015b]
./freeorion(_ZN11PartControlC1EPK8PartType+0x45d)[0x1930255]
./freeorion(_ZN12PartsListBox8PopulateEv+0x3dc)[0x1930e44]
./freeorion(_ZN12PartsListBox8SizeMoveERKN2GG2PtES3_+0x151)[0x1930a5d]
./freeorion(_ZN9DesignWnd11PartPalette8DoLayoutEv+0x51a)[0x19320b8]
./freeorion(_ZN9DesignWnd11PartPaletteC2EN2GG1XENS1_1YE+0x641)[0x1931a3f]
./freeorion(_ZN9DesignWndC1EN2GG1XENS0_1YE+0x554)[0x193a90c]
./freeorion(_ZN6MapWndC1Ev+0x108c)[0x18a70e2]
./freeorion(_ZN8ClientUIC1Ev+0x141)[0x1756ef5]
./freeorion(_ZN14HumanClientAppC2EPN4Ogre4RootEPNS0_12RenderWindowEPNS0_12SceneManagerEPNS0_6CameraEPNS0_8ViewportERKN5boost11filesystem34pathE+0x50d)[0x16ba245]
./freeorion(_Z19mainSetupAndRunOgrev+0x9c5)[0x16e0ede]
./freeorion(main+0x179)[0x16debc1]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xfd)[0x7f0013108ead]
./freeorion[0x168b575]

[spikethehobbit]

MALLOC_CHECK_=3 ./freeorion

segfaults at GG/src/Font.cpp @ 1318
temp_glyph_data[c] =
TempGlyphData(...)

the crash itself is in the heap check done glibc, so this appears to be a corrupted heap

[Geoff the Medio]

Does reverting this patch prevent this crash for you?

[spikethehobbit]

No, reverting that patch does not fix it. Running under valgrind only shows two errors before valgrind itself faults.

First is an overlapping memcpy in libasound. At worst this should produce corrupted audio output. It seems unlikely to be the culprit.

Second is an out-of-bounds write at GG/src/Font.cpp line 1312

Code: Select all

                for (int row = 0; row < glyph_bitmap.rows; ++row) {
                    boost::uint8_t*  src = src_start + row * glyph_bitmap.pitch;
                    boost::uint16_t* dst = dst_start + (row + Value(y_offset)) * Value(BUF_WIDTH);
                    for (int col = 0; col < glyph_bitmap.width; ++col) {
#ifdef __BIG_ENDIAN__
                        *dst++ = *src++ | (255 << 8); // big-endian uses different byte ordering
#else
                        *dst++ = (*src++ << 8) | 255; // alpha is the value from glyph_bitmap; luminance is always 100% white
#endif
                    }
                }

Should this do more than give unreadable text if I comment it out?

[Geoff the Medio]

Should this do more than give unreadable text if I comment it out?

I don't know... I didn't write and don't understand most of the GG Font code.

Do you know or can you check if this started happening with a particular revision? If it's not related to SVN 5167, then something weird and rather specific to you may be going on, as the Font code hasn't otherwise changed in at least 10 months, except being copied from the GG repository into the FreeOrion repository.

[zhur]

No, reverting that patch does not fix it. Running under valgrind only shows two errors before valgrind itself faults.

Just to be sure - did you reinstall GG after recompiling it? (Asking because happened to me once )

Edit: Also found this thread. Can it be somehow related?

[spikethehobbit]

Commenting out line 1312 erases text, but no more crash.

It worked before the GiGi merger. The fixes to building GiGi on Linux may be the culprit, since those scripts are rather hairy, and whitespace sensitive. I will go over those again.

[spikethehobbit]

@zhur that looks suspiciously similar. Heap errors can show up in odd ways.

Fixing whitespace in GG/libltdl had no effect.
It seems GiGi has a test suite, and 24 out of 200 fail. Not sure if related.
I'll try this thing under a debugger next, then start bisecting.

[Geoff the Medio]

It seems GiGi has a test suite, and 24 out of 200 fail. Not sure if related.

I guess the obvious question is whether some of those failures are also related to the GG -> FO merge. Do the same tests fail when checking out and building the latest revision in GG's own repository?

[spikethehobbit]

Test suite errors are the same with vanilla GiGi. It looks like the tests assume MS Windows widgets, but on my system it is using Gtk.

At this point I am working on the assumption that the buffer overflow in font glyph generation is causing arbitrary memory corruption. The startup code is largely deterministic, so what gets hit is consistent, but system dependent. It was just my luck to get bitten where it hurts

This could also explain the font corruption that people have been noticing.

In any event, it is a bug that needs fixing.

I will probably have time to work on it some more this evening.

[Geoff the Medio]

Test suite errors are the same with vanilla GiGi. It looks like the tests assume MS Windows widgets, but on my system it is using Gtk.

I'd be surprised if the tests assumed Windows, since the GiGi developer tzlaine worked primarily on Linux. That there were test failures doesn't necessarily mean something was unintentionally broken though, as it just might not have been gotten to yet.

This could also explain the font artifacts that people have been noticing.

It might have something to do with the variability of the font corruption, as what bit of memory was used for storing the fonts would vary from system to system and due to seemingly unrelated variation in memory usage. zhur's patch (linked above) to zero the proper range of memory seems to fix the artifacts though, and seems to make sense to me...

In any event, it is a bug that needs fixing.

I will probably have time to work on it some more this evening.

I'd appreciate that. I don't understand the GG Font setup and rendering code, and my attempts to look into it haven't been very productive.

[spikethehobbit]

This patch doesn't actually fix the root problem, but it prevents the buffer overflow. Most text still shows as before. zhur's patch seems to work correctly for what it does, but I'm still showing text corruption (dots and lines above and below text) that I believe is caused by this bug, that is glyphs are being copied with incorrect alignment into the buffer. I still don't know what 'correct' is, or how to calculate the needed vertical buffer space.

Currently, 9 glyphs in DejaVuSans-Bold.ttf are overshooting the buffer by 1-2 lines. This patch detects and discards them, then logs to stdout.

I am currently testing under valgrind, and have started loading a save game. The opening menu displays ok, except for the aforementioned text corruption. 15 minutes in at 100% CPU and no map screen yet, but no errors in any logs either.

[spikethehobbit]

This patch fixes the buffer overrun and font corruption. It supersedes the Font.cpp.diff patch submitted earlier in thread. Don't use that one.

Summary of changes:
Modifies Font::Init glyph precaching to take into account glyphs of different heights, including broken fonts that do not honour m_height.
Uses actual bitmap height instead of m_height to determine target bitmap height.
Resets max_x when creating new buffer.
Tracks max_y as glyphs may be different heights.
New buffer created if any glyph crosses the bottom, not just the first on the row.
Allows use of rightmost column and bottom row in buffer.
Traps glyphs larger than the buffer. This shouldn't happen, but a bad font could otherwise cause an infinite loop ending in OOM.
Pushes y_offset shift into GL rendering instead of bitmap cache to better pack the bitmaps. This also eliminates a possible buffer underrun due to y_offset sometimes being negative. FT_MAGIC_NUMBER removed as it is now redundant. Added field to TempGlyphData, and parameter to Glyph::Glyph.

Caveats:
I have no idea if the y_offset calculation is correct. I may have reversed the sign in the GL code, however that should have caused uneven text if it was wrong.
Removing FT_MAGIC_NUMBER may have shifted text vertically 4 pixels, although it should have cancelled.

**This was written in the wee hours during a bout of sugar induced insomnia. Unsure if I actually understood GiGi font rendering, or if it was just the sugar high. In any case, I no longer really understand what I did. Twitch.**

valgrind does not report any errors, and text appears ok. good night.

[EDIT]
/*
Patch fix_font.diff (C) 2012 SpikeTheHobbitMage
This code is released under the terms of the GNU General Public Licence GPL v2.0 or greater, or under the Lesser GNU General Public Licence LGPL v2.0 or greater.
*/
[/EDIT]

[Geoff the Medio]

Could you add a note to your post with the latest patch that you release the changes under the GPL 2.0 or later?