Re: [ft-devel] FT_Outline memory overallocation, or at least incorrect cache weighting?

[Paul Messmer]

The overallocation is an urgent issue for me, but requires no urgent action on your part.  The bar for me to make a small local fix to FreeType is lower than integrating an entirely new library release.  After looking at the code further, I'm less concerned about the possibility of access of more than n_points FT_Vectors, and I also have some testing I can exercise to give me some confidence the "fix" doesn't break anything... at least in the scope of my usage of FreeType.  

I wanted to bring this up with freetype-devel in order to get a second opinion about the situation, and so the problem could be fixed in your releases going forward.  As there doesn't appear to be a good test suite for FreeType, your exercising caution with respect to the change is a fine idea as over-allocation can easily hide a latent and non-obvious bug.

I'll keep you up to date on whether I encounter any failures after removing the factor of 2 in the allocation.  Thanks.

-- Paul

On Thu, Feb 4, 2010 at 10:35 PM, <address@hidden> wrote:

Dear Paul,

On Thu, 4 Feb 2010 15:29:28 -0800
Paul Messmer <address@hidden> wrote:
>The code above seems to believe there are n_points FT_Vectors allocated.
> However,
>
>.../base/ftoutln.c:  FT_Outline_New_Internal()
>
>    if ( FT_NEW_ARRAY( anoutline->points,   numPoints * 2L ) ||
>         FT_NEW_ARRAY( anoutline->tags,     numPoints      ) ||
>         FT_NEW_ARRAY( anoutline->contours, numContours    ) )
>      goto Fail;
>    anoutline->n_points    = (FT_UShort)numPoints;
>
>This seems to be allocating 2*n_points FT_Vectors, so there's a difference
>between how much memory is actually being used and how much it believes is
>being used.

Great thank you for finding the problem.

Tracking the history of the ftoutln.c, I think it came
from freetype-1 era (!). A "point" in TT_Outline was
described by TT_Vector, like this:

       http://cvs.savannah.gnu.org/viewvc/freetype/lib/freetype.h?revision=1.72&root=freetype&view=markup

 struct  TT_Outline_
 {
   TT_Short         n_contours;   /* number of contours in glyph   */
   TT_UShort        n_points;     /* number of points in the glyph */

   TT_Vector*       points;       /* the outline's points   */
   ...

But TT_New_Outline() allocated the buffer as the twice
of sizeof ( TT_F26Dot6 ), like this:

       http://cvs.savannah.gnu.org/viewvc/freetype/lib/ttapi.c?revision=1.55&root=freetype&view=markup

 FT_EXPORT_FUNC( TT_Error )
 TT_New_Outline( TT_UShort    numPoints,
                 TT_Short     numContours,
                 TT_Outline*  outline )
 {
   ...

   if ( ALLOC( outline->points,   numPoints*2*sizeof ( TT_F26Dot6 ) ) ||
        ALLOC( outline->flags,    numPoints  *sizeof ( Byte )       ) ||
        ALLOC( outline->contours, numContours*sizeof ( UShort )     ) )
     goto Fail;

If it were written "sizeof ( FT_Vector )", this problem might not
have occured. This "2" was carried over to FreeType2, because the
initial version of FreeType2 still specified its size by twice of
sizeof( FT_Pos ), like this:

http://cvs.savannah.gnu.org/viewvc/freetype2/src/base/ftoutln.c?revision=1.1&root=freetype&view=markup

   if ( ALLOC_ARRAY( outline->points,   numPoints * 2L, FT_Pos    ) ||
        ALLOC_ARRAY( outline->flags,    numPoints,      FT_Byte   ) ||
        ALLOC_ARRAY( outline->contours, numContours,    FT_UShort ) )
     goto Fail;

Since freetype-2.1.0, FreeType2 uses FT_NEW_ARRAY() which
automatically calculate the size of array element with
sizeof( *first_arg ), so the factor 2 is not needed anymore,
as you've pointed out. So, this problem has long life since
freetype-2.1.0. The moment how overallocation can be found
at:

http://cvs.savannah.gnu.org/viewvc/freetype2/src/base/ftoutln.c?root=freetype&r1=1.44&r2=1.45

--

I want to remove this extra factor "2". But you also mentioned
the possibility that the buffer over outline->n_points is used
in some special case, so I hesitate to remove it without careful
check. This over allocation is urgent issue for you? If not, I
want to work for this issue after next official release (expected soon).

Thank you again for detailed investigation and comment.

Regards,
mpsuzuki