freetype-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Paul Messmer
Subject: Re: [ft-devel] FT_Outline memory overallocation, or at least incorrect cache weighting?
Date: Fri, 12 Feb 2010 16:36:46 -0800

My tests involved exercising both testing and production code that was using FreeType as part of laying out and rasterizing text from TrueType fonts.  The output appeared unchanged after the fix, and I didn't see crashing or other behavior suggestive of data corruption.  Memory consumption was (unsurprisingly) observed to be lower.  I ran some of the testing under valgrind, which did not notice any issues.  A quick grep shows these APIs were probably exercised:

FTC_CMapCache_Lookup
FTC_CMapCache_New
FTC_ImageCache_Lookup
FTC_ImageCache_New
FTC_Manager_Done
FTC_Manager_LookupFace
FTC_Manager_New
FT_Done_Face
FT_Done_FreeType
FT_Get_Charmap_Index
FT_Get_Kerning
FT_Glyph_Get_CBox
FT_Glyph_StrokeBorder
FT_Glyph_To_Bitmap
FT_Glyph_Transform
FT_Init_FreeType
FT_Load_Glyph
FT_Load_Sfnt_Table
FT_New_Face
FT_New_Memory_Face
FT_Open_Face
FT_Outline_Copy
FT_Outline_Done
FT_Outline_Get_CBox
FT_Outline_New
FT_Outline_Translate
FT_Set_Pixel_Sizes
FT_Stroker_Done
FT_Stroker_New
FT_Stroker_Set

-- Paul

On Thu, Feb 11, 2010 at 9:17 PM, <address@hidden> wrote:
Thank you for report. Could you tell me the coverage
of your testing (font formats, what kind of tests are
executed, etc)?

Regards,
mpsuzuki

On Tue, 9 Feb 2010 13:55:37 -0800
Paul Messmer <address@hidden> wrote:

>Testing after removing the factor of 2 isn't showing any problems for me.
>
>-- Paul
>
>On Fri, Feb 5, 2010 at 3:48 PM, Paul Messmer <address@hidden> wrote:
>
>> 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
>>>
>>
>>
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]