I have a VTK application that segfaults in libfreetype, when text in rendered. I tried to look for details with gdb, and the problem seems to be in the inlined code of FTC_CACHE_LOOKUP_CMP, called just before ftccmap.c:382. The crash occurs when optimization is enabled. In this case, node is stored in a register, and for an unknown reason, node is reset to zero before being dereferenced. The code following the _Ok label, in the macro definition of FTC_CACHE_LOOKUP_CMP _pnode = (FTC_Node*)(void*)&(node); *_pnode = _node; seems equivalent to this simpler version : node = _node; And the app no longer crashes with this fix. I'll attach the simple VTK app that generates the crash for me. Compile it, with vtk-devel installed, with : gcc -c AppliPlanes.cxx -I/usr/include/vtk gcc -o AppliPlanes AppliPlanes.o -lvtkRendering
Created attachment 355001 [details] sample vtk app trigerring the crash AppliPlanes.cxx
Created attachment 355002 [details] sample vtk app trigerring the crash (header) AppliPlanes.h
Created attachment 355003 [details] workaround patch
Yeah, a classic aliasing violation. Behdad: ping?
Thanks. Building.
freetype-2.3.9-5.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/freetype-2.3.9-5.fc11
What about F12? And F10?
I reported the issue upstream, so I'm hoping to get a new release with the fix for F12. Feel free to build for F10.
freetype-2.3.9-5.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report.
Applied to FreeType git, thanks.
Bah, you mean you have to post bugs with _patches_ to get any interest around here? :) Werner lets me know that https://bugzilla.redhat.com/show_bug.cgi?id=506840 is likely the same as this. I will test and confirm. -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
*** Bug 506840 has been marked as a duplicate of this bug. ***
Note that the fix proposed above makes compilation fail with C++. Any idea how to correct that?
Is ftccache.h a public header? If it's not, why does it have to be buildable as C++?
Some platforms use the C++ compiler even for compiling C code. We always try to make FreeType compile without warnings (and errors) for both C and C++. Nelson Beebe, who is maintaining a real zoo of different Unix machines, sometimes provides reports like this: http://lists.gnu.org/archive/html/freetype-devel/2003-04/msg00054.html
confirming the fix/workaround patch in this report fixes my bug 506840. Thanks, Fabrice. -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
Hmmm, the problem is that even with that aliasing issue fixed, you're still casting pointers of different types, which is both a likely violation of aliasing rules and invalid C++. You have a structure like this: /* the cmap cache node */ typedef struct FTC_CMapNodeRec_ { FTC_NodeRec node; FTC_FaceID face_id; FT_UInt cmap_index; FT_UInt32 first; /* first character in node */ FT_UInt16 indices[FTC_CMAP_INDICES_MAX]; /* array of glyph indices */ } FTC_CMapNodeRec, *FTC_CMapNode; and then you're trying to use an FTC_CMapNode (an FTC_CMapNodeRec *) as an FTC_Node (an FTC_NodeRec *). The GCC C frontend has special rules to allow this type of "poor man's inheritance", in C++ you can't do that without actual inheritance. You should be using &(node->node) rather than just node where you're passing your FTC_CMapNode to functions which expect an FTC_Node.
Hmmm, &(node->node) isn't going to work in that context because you're using it as an lvalue.
I think the only fix which is likely to work with C++ is to edit FTC_CMapCache_Lookup in ftccmap.c to add a variable: FTC_Node ftcnode; and to change the FTC_CACHE_LOOKUP_CMP call to: FTC_CACHE_LOOKUP_CMP( cache, ftc_cmap_node_compare, hash, &query, ftcnode, error ); node = (FTC_CMapNode) ftcnode; If FTC_CACHE_LOOKUP_CMP is also used in other places, they'll need the same type of fix. This is still a somehow unsafe cast, mind you, but at least this one should compile and not trigger an aliasing violation in GCC. (I still think the idea of writing code which compiles as both C and C++ is insane though. ;-) )
Would it be too much to ask if this could be patched in Rawhide while we wait for a new upstream release? It would help me get my navit package review done, because that app fails to run entirely unless this bug is fixed, so my review for it will be stuck in limbo until this bug is fixed. thanks! -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
Building.
Thanks a bunch, Behdad - I really appreciate it. -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
As Kevin mentioned already, proposed fix has a side effect that GCC finds a cast between incompatible pointers. Although current git head of FreeType2 includes it, I want to revert it. In my investigation, the crashing behaviour was supposed to be introduced by Richard Guenther's inline optimizer for GCC-4.4 branch, committed on 2008-08-09. For detail, see https://savannah.nongnu.org/bugs/index.php?27441 I propose another fix to pass the problem, aslike: diff --git a/src/cache/ftccache.h b/src/cache/ftccache.h index 2082bc4..5e932b7 100644 --- a/src/cache/ftccache.h +++ b/src/cache/ftccache.h @@ -206,7 +206,7 @@ FT_BEGIN_HEADER \ \ error = 0; \ - node = NULL; \ + /* node = NULL; */ \ _idx = _hash & _cache->mask; \ if ( _idx < _cache->p ) \ _idx = _hash & ( _cache->mask*2 + 1 ); \ @@ -246,7 +246,8 @@ FT_BEGIN_HEADER error = FTC_Cache_NewNode( _cache, _hash, query, &_node ); \ \ _Ok: \ - node = _node; \ + _pnode = (FTC_Node*)(void*)&(node); \ + *_pnode = _node; \ FT_END_STMNT #else /* !FTC_INLINE */ I want to revert the fix that current FC11 uses and apply this patch. Anybody can test if this fix works well?
if you throw out a scratch build, I will check it. -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
Created attachment 362615 [details] source rpm including the patch proposed by mpsuzuki Here is the source rpm including the patch proposed in https://bugzilla.redhat.com/show_bug.cgi?id=513582#c23
my incarnation of this problem (navit failing to run) does not happen with the build from comment #25, so I have no problem with this change. -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
Thank you very much for testing! I will replace previous fix for ftccache.h by new one.
As far as I understand, the proposed fix still violates C aliasing rules, even if gcc currently is not smart enough for it. One fix I can imagine is to introduce a union that has both pointer types as members, the assing to one, read from the other.
Behdad is right, mpsuzuki's "fix" is NOT correct, please DO NOT use this! Every "fix" using that pnode hack will always be an aliasing rule violation. Even if it happens to work with the current GCC, it may break at any point in the future. Please read comment #19 if you really want to be able to compile this C code with C++ (which I don't give a darn about, and which shouldn't take priority over making it actually VALID C – that aliasing violation is not valid C++ either, by the way; you'd use inheritance to do this properly in C++, if this were actually C++ code, which it isn't).
Thanks Behdad & Kevin, My fix in #25 just disables the problematic optimization in GCC-4.4.x, I know it does not fix the strict aliasing issue (I thought it was different issue). I will try another fix following to comment #19.
Created attachment 362757 [details] Patch to introduce the unions to the parts violating strict aliasing. Behdad, Following is what you suggested? Attached is a patch to do similar modifications for all part warned as violating strict aliasing. diff --git a/src/cache/ftccmap.c b/src/cache/ftccmap.c index 50a6189..8fb3387 100644 --- a/src/cache/ftccmap.c +++ b/src/cache/ftccmap.c @@ -283,11 +283,15 @@ { FTC_Cache cache = FTC_CACHE( cmap_cache ); FTC_CMapQueryRec query; - FTC_CMapNode node; FT_Error error; FT_UInt gindex = 0; FT_UInt32 hash; FT_Int no_cmap_change = 0; + union + { + FTC_Node node; + FTC_CMapNode cmap; + } ftc_unode; if ( cmap_index < 0 ) @@ -371,20 +375,20 @@ #if 1 FTC_CACHE_LOOKUP_CMP( cache, ftc_cmap_node_compare, hash, &query, - node, error ); + ftc_unode.node, error ); #else - error = FTC_Cache_Lookup( cache, hash, &query, (FTC_Node*) &node ); + error = FTC_Cache_Lookup( cache, hash, &query, &(ftc_unode.node) ); #endif if ( error ) goto Exit; - FT_ASSERT( (FT_UInt)( char_code - node->first ) < FTC_CMAP_INDICES_MAX ); + FT_ASSERT( (FT_UInt)( char_code - ftc_unode.cmap->first ) < FTC_CMAP_INDICES_MAX ); /* something rotten can happen with rogue clients */ - if ( (FT_UInt)( char_code - node->first >= FTC_CMAP_INDICES_MAX ) ) + if ( (FT_UInt)( char_code - ftc_unode.cmap->first >= FTC_CMAP_INDICES_MAX ) ) return 0; - gindex = node->indices[char_code - node->first]; + gindex = ftc_unode.cmap->indices[char_code - ftc_unode.cmap->first]; if ( gindex == FTC_CMAP_UNKNOWN ) { FT_Face face; @@ -392,7 +396,7 @@ gindex = 0; - error = FTC_Manager_LookupFace( cache->manager, node->face_id, &face ); + error = FTC_Manager_LookupFace( cache->manager, ftc_unode.cmap->face_id, &face ); if ( error ) goto Exit; @@ -413,7 +417,7 @@ FT_Set_Charmap( face, old ); } - node->indices[char_code - node->first] = (FT_UShort)gindex; + ftc_unode.cmap->indices[char_code - ftc_unode.cmap->first] = (FT_UShort)gindex; } Exit:
Created attachment 362758 [details] source rpm including the 2nd patch proposed by mpsuzuki Here is new source rpm including my 2nd patch.
This one should now work with GCC and other real-world compilers. It's still not valid C because the C standard explicitly does not require a union to be 100% overlapping storage (if you assign one member, the value of the other is undefined), in fact handling "union" as "struct" would still be a compliant implementation of "union". But GCC *does* guarantee that you can use "union" that way and other compilers in practical use do as well.
Thank you very quick reply. Hmm, I should rework. You suggested to pass FTC_Node variable to FTC_CACHE_LOOKUP_CMP() and cast it to FTC_CMapNode by caller. It is safer than using union?
Created attachment 362766 [details] Patch to cast the pointers allocated by FTC_XXX_LOOKUP_CMP() in the callers. Kevin, This is what you suggested in comment #19? diff --git a/src/cache/ftccmap.c b/src/cache/ftccmap.c index 50a6189..428cebb 100644 --- a/src/cache/ftccmap.c +++ b/src/cache/ftccmap.c @@ -283,7 +283,7 @@ { FTC_Cache cache = FTC_CACHE( cmap_cache ); FTC_CMapQueryRec query; - FTC_CMapNode node; + FTC_Node node; FT_Error error; FT_UInt gindex = 0; FT_UInt32 hash; @@ -373,18 +373,18 @@ FTC_CACHE_LOOKUP_CMP( cache, ftc_cmap_node_compare, hash, &query, node, error ); #else - error = FTC_Cache_Lookup( cache, hash, &query, (FTC_Node*) &node ); + error = FTC_Cache_Lookup( cache, hash, &query, &node ); #endif if ( error ) goto Exit; - FT_ASSERT( (FT_UInt)( char_code - node->first ) < FTC_CMAP_INDICES_MAX ); + FT_ASSERT( (FT_UInt)( char_code - FTC_CMAP_NODE( node )->first ) < FTC_CMAP_INDICES_MAX ); /* something rotten can happen with rogue clients */ - if ( (FT_UInt)( char_code - node->first >= FTC_CMAP_INDICES_MAX ) ) + if ( (FT_UInt)( char_code - FTC_CMAP_NODE( node )->first >= FTC_CMAP_INDICES_MAX ) ) return 0; - gindex = node->indices[char_code - node->first]; + gindex = FTC_CMAP_NODE( node )->indices[char_code - FTC_CMAP_NODE( node )->first]; if ( gindex == FTC_CMAP_UNKNOWN ) { FT_Face face; @@ -392,7 +392,7 @@ gindex = 0; - error = FTC_Manager_LookupFace( cache->manager, node->face_id, &face ); + error = FTC_Manager_LookupFace( cache->manager, FTC_CMAP_NODE( node )->face_id, &face ); if ( error ) goto Exit; @@ -413,7 +413,7 @@ FT_Set_Charmap( face, old ); } - node->indices[char_code - node->first] = (FT_UShort)gindex; + FTC_CMAP_NODE( node )->indices[char_code - FTC_CMAP_NODE( node )->first] = (FT_UShort)gindex; } Exit:
Created attachment 362768 [details] source rpm including the 3rd patch proposed by mpsuzuki The source rpm including patch in comment #35.
Yeah, that was my idea and I think that's the safest solution.
Adam, Could you test the new source rpm in comment #36?
yup, seems OK, navit runs without errors, nothing else seems to have any problems. -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers
Thank you! I will "add" the patch proposed in comment #35 to next release of freetype, 2.3.0.
The patch in comment #35 is applied to git head. freetype-2.3.10 will include it. Thanks to everybody helped me.