Bug 513582 - segfault in FTC_CMapCache_Lookup()
Summary: segfault in FTC_CMapCache_Lookup()
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: freetype
Version: 11
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Behdad Esfahbod
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 506840 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-07-24 11:24 UTC by Fabrice Bellet
Modified: 2009-09-29 17:48 UTC (History)
7 users (show)

Fixed In Version: 2.3.9-5.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-07-27 21:27:32 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
sample vtk app trigerring the crash (2.18 KB, text/plain)
2009-07-24 11:26 UTC, Fabrice Bellet
no flags Details
sample vtk app trigerring the crash (header) (403 bytes, text/plain)
2009-07-24 11:27 UTC, Fabrice Bellet
no flags Details
workaround patch (627 bytes, patch)
2009-07-24 11:29 UTC, Fabrice Bellet
no flags Details | Diff
source rpm including the patch proposed by mpsuzuki (1.58 MB, application/x-redhat-package-manager)
2009-09-25 06:07 UTC, mpsuzuki
no flags Details
Patch to introduce the unions to the parts violating strict aliasing. (11.93 KB, patch)
2009-09-26 12:02 UTC, mpsuzuki
no flags Details | Diff
source rpm including the 2nd patch proposed by mpsuzuki (1.58 MB, application/x-redhat-package-manager)
2009-09-26 12:05 UTC, mpsuzuki
no flags Details
Patch to cast the pointers allocated by FTC_XXX_LOOKUP_CMP() in the callers. (10.27 KB, patch)
2009-09-26 16:36 UTC, mpsuzuki
no flags Details | Diff
source rpm including the 3rd patch proposed by mpsuzuki (1.58 MB, application/x-redhat-package-manager)
2009-09-26 16:44 UTC, mpsuzuki
no flags Details

Description Fabrice Bellet 2009-07-24 11:24:36 UTC
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

Comment 1 Fabrice Bellet 2009-07-24 11:26:07 UTC
Created attachment 355001 [details]
sample vtk app trigerring the crash

AppliPlanes.cxx

Comment 2 Fabrice Bellet 2009-07-24 11:27:05 UTC
Created attachment 355002 [details]
sample vtk app trigerring the crash (header)

AppliPlanes.h

Comment 3 Fabrice Bellet 2009-07-24 11:29:03 UTC
Created attachment 355003 [details]
workaround patch

Comment 4 Kevin Kofler 2009-07-24 13:29:08 UTC
Yeah, a classic aliasing violation.

Behdad: ping?

Comment 5 Behdad Esfahbod 2009-07-25 20:06:42 UTC
Thanks.  Building.

Comment 6 Fedora Update System 2009-07-25 20:24:14 UTC
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

Comment 7 Kevin Kofler 2009-07-25 21:59:05 UTC
What about F12? And F10?

Comment 8 Behdad Esfahbod 2009-07-26 03:31:02 UTC
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.

Comment 9 Fedora Update System 2009-07-27 21:27:27 UTC
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.

Comment 10 Werner Lemberg 2009-07-29 09:49:04 UTC
Applied to FreeType git, thanks.

Comment 11 Adam Williamson 2009-07-29 16:05:28 UTC
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

Comment 12 Kevin Kofler 2009-07-29 16:54:55 UTC
*** Bug 506840 has been marked as a duplicate of this bug. ***

Comment 13 Werner Lemberg 2009-07-29 17:29:50 UTC
Note that the fix proposed above makes compilation fail with C++.  Any idea how to correct that?

Comment 14 Kevin Kofler 2009-07-29 17:51:42 UTC
Is ftccache.h a public header? If it's not, why does it have to be buildable as C++?

Comment 15 Werner Lemberg 2009-07-29 18:09:38 UTC
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

Comment 16 Adam Williamson 2009-07-29 18:40:42 UTC
confirming the fix/workaround patch in this report fixes my bug 506840. Thanks, Fabrice.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 17 Kevin Kofler 2009-07-29 19:14:23 UTC
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.

Comment 18 Kevin Kofler 2009-07-29 19:15:42 UTC
Hmmm, &(node->node) isn't going to work in that context because you're using it as an lvalue.

Comment 19 Kevin Kofler 2009-07-29 19:21:42 UTC
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. ;-) )

Comment 20 Adam Williamson 2009-07-30 16:38:54 UTC
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

Comment 21 Behdad Esfahbod 2009-07-30 18:05:21 UTC
Building.

Comment 22 Adam Williamson 2009-07-30 22:43:47 UTC
Thanks a bunch, Behdad - I really appreciate it.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 23 mpsuzuki 2009-09-25 05:39:16 UTC
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?

Comment 24 Adam Williamson 2009-09-25 05:43:41 UTC
if you throw out a scratch build, I will check it.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 25 mpsuzuki 2009-09-25 06:07:54 UTC
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

Comment 26 Adam Williamson 2009-09-25 18:06:17 UTC
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

Comment 27 mpsuzuki 2009-09-25 18:34:00 UTC
Thank you very much for testing!
I will replace previous fix for ftccache.h by new one.

Comment 28 Behdad Esfahbod 2009-09-26 02:21:01 UTC
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.

Comment 29 Kevin Kofler 2009-09-26 06:23:48 UTC
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).

Comment 30 mpsuzuki 2009-09-26 06:44:39 UTC
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.

Comment 31 mpsuzuki 2009-09-26 12:02:58 UTC
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:

Comment 32 mpsuzuki 2009-09-26 12:05:08 UTC
Created attachment 362758 [details]
source rpm including the 2nd patch proposed by mpsuzuki

Here is new source rpm including my 2nd patch.

Comment 33 Kevin Kofler 2009-09-26 12:10:24 UTC
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.

Comment 34 mpsuzuki 2009-09-26 12:24:05 UTC
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?

Comment 35 mpsuzuki 2009-09-26 16:36:37 UTC
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:

Comment 36 mpsuzuki 2009-09-26 16:44:22 UTC
Created attachment 362768 [details]
source rpm including the 3rd patch proposed by mpsuzuki

The source rpm including patch in comment #35.

Comment 37 Kevin Kofler 2009-09-26 18:35:17 UTC
Yeah, that was my idea and I think that's the safest solution.

Comment 38 mpsuzuki 2009-09-26 23:05:31 UTC
Adam,
Could you test the new source rpm in comment #36?

Comment 39 Adam Williamson 2009-09-28 17:37:17 UTC
yup, seems OK, navit runs without errors, nothing else seems to have any problems.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 40 mpsuzuki 2009-09-28 17:50:27 UTC
Thank you!
I will "add" the patch proposed in comment #35 to next release of freetype, 2.3.0.

Comment 41 mpsuzuki 2009-09-29 17:48:21 UTC
The patch in comment #35 is applied to git head. freetype-2.3.10 will include it.
Thanks to everybody helped me.


Note You need to log in before you can comment on or make changes to this bug.