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

[mpsuzuki]

Hi all,

Just I've grepped "n_points" and reviewed the functions
using it. There are so many functions that the length
of FT_Outline->points[] is assumed to be same with
FT_Outline->n_points, and the rest of buffer is often
overwritten or ignored during the copying of the buffer.

There are a few functions that fills FT_Outline->points[]
and update FT_Outline->n_points, without checking to
prevent buffer overflow. However, they are commented
in public header files, as the client should allocate
the sufficient buffer before calling them.
FT_Stroker_Export() and FT_Stroker_ExportBorder() are
good example.

If I search how many software using these functions,
the only example I could find was cinelerra (a non-linear
video editor, see http://cinelerra.org/). cinelerra
used FT_Stroker_Export() correctly, removing extra
allocation of FT_Outline->points[] won't make cinelerra
crashed.

So I want to fix the part allocating FT_Outline->points[]
as twiced size of FT_Outline->n_points, in next official
release. If anybody has concern, please let me know soon.

Regards,
mpsuzuki


--------------------------------------------------------
src/autofit/afhints.c
        af_glyph_hints_reload()
                extends the array if required.
                the condition is decided by n_points versus max_points.
                even if unused elements follow after points[max_points],
                they are not considered.

src/autofit/aflatin.c
        af_latin_metrics_init_widths()
        af_latin_metrics_init_blues()
                non-positive n_points is checked

src/autofit/aflatin2.c
        af_latin2_metrics_init_widths()
        af_latin2_metrics_init_blues()
                non-positive n_points is checked

src/autofit/autofit.c
        af_loader_load_g()
                use FT_GLYPHLOADER_CHECK_POINTS check buffer overrun
                and extends the buffer if required.
                when the buffer is extended, only n_points elements
                are copied. (n_points, max_points] are not copied.

                Also FT_GlyphLoader_CopyPoints() truncates the content
                the buffer after n_points, when composite outline is
                being created.

src/base/ftbbox.c
        FT_Outline_Get_BBox()
                does not scan the buffer after n_points.

src/base/ftgloader.c
        FT_GlyphLoader_Adjust_Points()
                only n_points elements are cared.

        FT_GlyphLoader_CheckPoints()
                overrun is checked by n_points, not 2 * n_points.

        FT_GlyphLoader_Add()
                the buffer after n_points are overwritten.

        FT_GlyphLoader_CopyPoints()
                only n_points elements are copied.

src/base/ftglyph.c
        ft_outline_glyph_init()
                allocate by FT_Outline_New() and write by FT_Outline_Copy()
        ft_outline_glyph_copy
                ditto.

src/base/ftoutln.c
        FT_Outline_New_Internal()
                *** allocate twice of n_points ***
        FT_Outline_Check()
                point of contours exceeds n_points, returns an error.
        FT_Outline_Copy()
                only n_points elements are copied.
        FT_Outline_Get_CBox()
                scanning of points are finished at n_points.
        FT_Outline_Translate()
                only n_points elements are translated.
                following elements are not translated.
        FT_Outline_Transform()
                only n_points elements are translated.
        FT_Outline_Get_Orientation()
                non-positive n_points is refused.

src/baset/ftstroke.c
        ft_stroke_border_export()
                the buffer after outline->points[n_points] are overwritten by 
border
                following elements are not translated.
        FT_Stroker_Export()
        FT_Stroker_ExportBorder()
                these functions take FT_Outline object with inconsistency
                between FT_Outline->n_points versus the length of 
FT_Outline->point[].
        FT_Glyph_StrokeBorder()
                measure the required size and allocate the buffer by itself.

src/cache/ftcimage.c
        ftc_inode_weight()
                evaluate the weight by the buffer size, but the buffer size
                is measured by n_points, not 2 * n_points.

src/cff/cffgload.c
        cff_builder_add_point()
                as commented, this function appends a point to outline object
                but does not check the buffer size.
        cff_builder_add_contour()
        cff_builder_close_contour()
        cff_decoder_parse_charstrings()
        cff_slot_load()
                assume the number of points is same with n_points.
                
src/gxvalid/gxvcommn.c
        gxv_ctlPoint_validate()
                assume the number of points is same with n_points.

src/pfr/pfrgload.c
        pfr_glyph_close_contour()
                assume the number of points is same with n_points.
                the point after point[n_points] are not cared.
        pfr_glyph_line_to()
                FT_GLYPHLOADER_CHECK_POINTS() extends the buffer if required.
                the point after point[n_points] is overwritten.
        pfr_glyph_curve_to()
                FT_GLYPHLOADER_CHECK_POINTS() extends the buffer if required.
                the point after point[n_points] is overwritten.
        pfr_glyph_load_rec()
                assume the number of points is same with n_points.
                the point after point[n_points] are not cared.

src/pfr/pfrobjs.c
        pfr_slot_load()
                assume the number of points is same with n_points.
                the point after point[n_points] are not cared.

src/psaux/psobjs.c
        t1_builder_add_point()
                as commented, this function appends a point to outline object
                but does not check the buffer size.
        t1_builder_add_contour()
        t1_builder_close_contour()
                assume the number of points is same with n_points.

src/psaux/t1decode.c
        t1_decoder_parse_charstrings()
                assume the number of points is same with n_points.

src/raster/ftraster.c
        ft_black_render()
                assume the number of points is same with n_points.

src/smooth/ftgrays.c
        gray_compute_cbox()
                assume the number of points is same with n_points.

src/smooth/ftsmooth.c
        ft_smooth_render_generic()
                assume the number of points is same with n_points.

src/truetype/ttgload.c
        TT_Load_Simple_Glyph()
                FT_GLYPHLOADER_CHECK_POINTS() extends the buffer if required.
        tt_prepare_zone()
                assume the number of points is same with n_points.
        TT_Hint_Glyph()
                assume the number of points is same with n_points.
        TT_Process_Simple_Glyph()
                adds 4 phantom points to current outline object,
                but does not extend the buffer.
        TT_Process_Composite_Component()
                assume the number of points is same with n_points.
        TT_Process_Composite_Glyph()
                FT_GLYPHLOADER_CHECK_POINTS() extends the buffer if required,
                and adds 4 phantom points to current outline object,
        load_truetype_glyph()   
                assume the number of points is same with n_points.

src/truetype/ttinterp.c
        Ins_SxVTL()
        Ins_GC()
        Ins_SCFS()
        Ins_MD()
        Ins_SDPVL()
        Ins_FLIPPT()
        Ins_FLIPRGON()
        Ins_FLIPRGOFF()
        Compute_Point_Displacement()
        Ins_SHP()
        Ins_SHC() # ?
        Ins_SHZ() # ?
        Ins_SHPIX()
        Ins_MSIRP()
        Ins_MDAP()
        Ins_MIAP()
        Ins_MDRP()
        Ins_MIRP()
        Ins_ALIGNRP()
        Ins_ALIGNRP()
        Ins_ISECT()
        Ins_ALIGNPTS()
        Ins_IP()
        Ins_UTP()
        Ins_DELTAP()
                assume the number of points is same with n_points.
        Ins_IUP()
                shrink the buffer to n_points.

src/truetype/ttobjs.c
        tt_size_init_bytecode()
                assume the number of twilight points is same with n_points.

src/type1/t1gload.c
        T1_Load_Glyph()
                assume the number of points is same with n_points.




On Fri, 12 Feb 2010 14:17:28 +0900
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
>>>>
>>>
>>>
>>
>
>
>_______________________________________________
>Freetype-devel mailing list
>address@hidden
>http://lists.nongnu.org/mailman/listinfo/freetype-devel