Re: [Devel] Possible bug in ftxpost.c ?

[Antoine Leca]

Re-bonjour Jean-Christophe,

En Jean-Christophe Dubacq va escriure:
> 
> OK, I now post a patch. I know it's for freetype1, but lots of
> applications still use this (ttf2{tfm,pk} among them, gfontview also...
> in fact, all the font utilities I used).

See below about FT2.

 
> I worked a bit, and I found a fix. It is a very simple patch to
> ftxpost.c, in lib/extend. (see at end of mail).

I disagree with the patch.

What we did (wrongly) on first instance was to "populate" the
post20->glyphNames array with the additional strings in the same order as
there was a request for an additional name, indicated by index greater than
258. Obviously this is wrong when indexes came out of order (as is the
case in the fix to Impact done for the Euro).

What you do is again to "populate" the  post20->glyphNames  array as
early as possible, but while storing at the correct index. I agree this
fixes the problem with Impact (and similarly "improved" fonts).
However, it still fails with some badly behaving fonts, such as those
where a PSname occurs multiple times (more than one glyph refers to the
same name! and I know them to exist, even if I do not have one at hand
with the problem occuring outside the standard names): in such a case,
we still could encounter a overrun problem with your patch.

 
> I checked freetype2, and the fix is almost exactly the same. There is a
> trick a bit more complicated to use the least possible memory, but it
> didn't matter muchi (equivalent function is in src/sfnt/ttpost.c).

Did you really check? It appears to me that Freetype 2 has the problem
already fixed (probably as part of the "enhancement" to spare memory).
As a result, here is an extract of running testnames against IMPACT:
  Glyph 188  name fraction -6 40 7 19 41
  Glyph 189  name Euro -1 40 27 26 41
  Glyph 190  name guilsinglleft 1 31 10 8 28
<...>
  Glyph 246  name threequarters 1 40 35 33 40
  Glyph 247  name francXXX 2 40 37 34 40
  Glyph 248  name Gbreve 2 48 28 24 49
  Glyph 249  name gbreve 2 42 27 23 48
  Glyph 250  name Idotaccent 2 47 14 10 47
  Glyph 251  name Scedilla 1 40 26 24 49
  Glyph 252  name scedilla 1 33 25 23 42
  Glyph 253  name Cacute 2 47 28 24 48
  Glyph 254  name cacute 1 41 24 22 42
  Glyph 255  name Ccaron 2 47 28 24 48
  Glyph 256  name ccaron 1 41 24 22 42
  Glyph 257  name dmacron 1 39 27 25 40
  Glyph 258  name overscore 0 43 28 28 2
  Glyph 259  name middot 14 19 17 7 6
  Glyph 260  name foursuperior 0 40 16 15 20
  Glyph 261  name commaaccent1 6 -3 17 5 10
  Glyph 262  name Amacron 0 46 26 26 46
  Glyph 263  name amacron 1 39 26 23 40
  Glyph 264  name Abreve 0 48 26 26 48
  Glyph 265  name abreve 1 42 26 23 43
  Glyph 266  name Aogonek 0 38 26 26 46
  Glyph 267  name aogonek 1 32 25 23 40
  Glyph 268  name Ccircumflex 2 47 28 24 48
  Glyph 269  name ccircumflex 1 41 24 22 42
  Glyph 270  name Cdot 2 47 28 24 48
  Glyph 271  name cdot 1 41 24 22 42
<...>

Which looks like OK to me (but I wait for aggreement from you).

So instead of applying your patch, I shall prefer to apply the
FT2 code instead. See patch included (after the patch from J.-C.)
Please test (and please Werner/David/whoever knows, review the
areas marked FIXME, I am unsure about some tests)

One potential problem of this patch, though, is that I have to
touch the .h, thus potentially asking for a new revision scheme.
Werner please comment about the incidence (or lack of).

The patch is NOT applied to the current CVS tree. I shall commit it
as soon as Jean-Christophe agrees it works for him.

 
> How difficult is it to port an application from freetype1 to freetype2 ?

This question you should better ask in a separate thread, in
address@hidden

 
> --- ftxpost.c.old       Thu Jul  4 16:26:20 2002
> +++ ftxpost.c   Thu Jul  4 16:24:39 2002
> @@ -126,7 +126,7 @@
>    {
>      DEFINE_LOAD_LOCALS( input->stream );
> 
> -    UShort  nameindex, n, num;
> +    UShort  nameindex, n, k, num;
>      Byte    len;
> 
> 
> @@ -177,10 +177,10 @@
> 
>      /* Now we can read the glyph names which are stored in */
>      /* Pascal string format.                               */
> -    for ( n = 0; n < num; n++ )
> +    for ( k=0, n = 0; n < num; n++ )
>      {
>        nameindex = post20->glyphNameIndex[n];
> -
> +      /* nameindex is only useful to count the glyphs */
>        if ( nameindex < 258 )
>          ;                               /* default Mac glyph, do nothing */
>        else
> @@ -192,13 +192,14 @@
> 
>          FORGET_Frame();
> 
> -        if ( ALLOC_ARRAY( post20->glyphNames[nameindex - 258],
> +        if ( ALLOC_ARRAY( post20->glyphNames[k],
>                            len + 1, Char ) ||
> -             FILE_Read( post20->glyphNames[nameindex - 258], len ) )
> +             FILE_Read( post20->glyphNames[k], len ) )
>            goto Fail1;
> 
>          /* we make a C string */
> -        post20->glyphNames[nameindex - 258][len] = '\0';
> +        post20->glyphNames[k][len] = '\0';
> +        k++;
>        }
>      }

--- ftxpost.h.org       Mon Dec 24 00:11:44 2001
+++ ftxpost.h   Mon Jul  8 17:12:24 2002
@@ -42,6 +42,7 @@
   struct TT_Post_20_
   {
     TT_UShort   numGlyphs;
+    TT_UShort   numNames;
     TT_UShort*  glyphNameIndex;
     TT_Char**   glyphNames;
   };
--- ftxpost.c.org       Mon Dec 24 00:12:20 2001
+++ ftxpost.c   Mon Jul  8 17:23:04 2002
@@ -126,14 +126,14 @@
   {
     DEFINE_LOAD_LOCALS( input->stream );
 
-    UShort  nameindex, n, num;
+    UShort  n, num_glyphs, num_names;
     Byte    len;
 
 
     if ( ACCESS_Frame( 2L ) )
       return error;
 
-    num = GET_UShort();
+    num_glyphs = GET_UShort();
 
     FORGET_Frame();
 
@@ -142,71 +142,90 @@
 
     /* There already exist fonts which have more than 32768 glyph names */
     /* in this table, so the test for this threshold has been dropped.  */
-    if ( num > input->numGlyphs )
+
+    if ( num_glyphs > input->numGlyphs )
       return TT_Err_Invalid_Post_Table;
 
-    post20->numGlyphs = num;
+    post20->numGlyphs = num_glyphs;
+
+    /* load the indices */
 
-    if ( ALLOC_ARRAY( post20->glyphNameIndex, num, TT_UShort ) )
+    if ( ALLOC_ARRAY( post20->glyphNameIndex, num_glyphs, TT_UShort ) )
       return error;
 
-    if ( ACCESS_Frame( num * 2L ) )
+    if ( ACCESS_Frame( num_glyphs * 2L ) )
       goto Fail;
 
-    for ( n = 0; n < num; n++ )
+    for ( n = 0; n < num_glyphs; n++ )
     {
       post20->glyphNameIndex[n] = GET_UShort();
+    }
+
+    FORGET_Frame();
+
+    /* compute number of names stored in table */
+    num_names = 0;
+
+    for ( n = 0; n < num_glyphs; n++ )
+    {
+      UShort  idx;
+
 
-      if ( post20->glyphNameIndex[n] > 258 + num )
+      idx = post20->glyphNameIndex[n];
+      if ( idx >= 258 )
       {
-        FORGET_Frame();
-        error = TT_Err_Invalid_Post_Table;
-        goto Fail;
+        idx -= 257;
+#if 0 /* FIXME This test has been dropped in Freetype 2. Why? */
+        if ( idx > num_glyphs )
+        {
+          error = TT_Err_Invalid_Post_Table;
+          goto Fail;
+        }
+#endif
+        if ( idx > num_names )
+          num_names = idx;
       }
     }
 
-    FORGET_Frame();
+    post20->numNames = num_names;
 
-    if ( ALLOC_ARRAY( post20->glyphNames, num, Char* ) )
+    if ( num_names == 0 )                       /* nothing more to do */
+      return TT_Err_Ok;
+
+    /* now load the name strings */
+    if ( ALLOC_ARRAY( post20->glyphNames, num_names, Char* ) )
       goto Fail;
 
     /* We must initialize the glyphNames array for proper */
     /* deallocation.                                      */
-    for ( n = 0; n < num; n++ )
+    /* FIXME  is it still really needed?                  */
+    for ( n = 0; n < num_names; n++ )
       post20->glyphNames[n] = NULL;
 
     /* Now we can read the glyph names which are stored in */
     /* Pascal string format.                               */
-    for ( n = 0; n < num; n++ )
+    for ( n = 0; n < num_names; n++ )
     {
-      nameindex = post20->glyphNameIndex[n];
-
-      if ( nameindex < 258 )
-        ;                               /* default Mac glyph, do nothing */
-      else
-      {
-        if ( ACCESS_Frame( 1L ) )
-          goto Fail1;
+      if ( ACCESS_Frame( 1L ) )
+        goto Fail1;
 
-        len = GET_Byte();
+      len = GET_Byte();
 
-        FORGET_Frame();
+      FORGET_Frame();
 
-        if ( ALLOC_ARRAY( post20->glyphNames[nameindex - 258],
-                          len + 1, Char ) ||
-             FILE_Read( post20->glyphNames[nameindex - 258], len ) )
-          goto Fail1;
+      if ( ALLOC_ARRAY( post20->glyphNames[n], len + 1, Char ) ||
+           FILE_Read( post20->glyphNames[n], len ) )
+        goto Fail1;
 
-        /* we make a C string */
-        post20->glyphNames[nameindex - 258][len] = '\0';
-      }
+      /* we make a C string */
+      post20->glyphNames[n][len] = '\0';
     }
 
     return TT_Err_Ok;
 
 
   Fail1:
-    for ( n = 0; n < num; n++ )
+    for ( n = 0; n < num_names; n++ )
       if ( post20->glyphNames[n] )
         FREE( post20->glyphNames[n] );
 
@@ -311,10 +330,13 @@
         break;
 
       case 0x00020000:
-        for ( n = 0; n < post->p.post20.numGlyphs; n++ )
-          if ( post->p.post20.glyphNames[n] )
-            FREE( post->p.post20.glyphNames[n] );
-        FREE( post->p.post20.glyphNames );
+        if ( post->p.post20.numNames )
+        {
+          for ( n = 0; n < post->p.post20.numNames; n++ )
+            if ( post->p.post20.glyphNames[n] )
+              FREE( post->p.post20.glyphNames[n] );
+          FREE( post->p.post20.glyphNames );
+        }
         FREE( post->p.post20.glyphNameIndex );
         break;