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 [email protected] 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 <[email protected]> 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 <[email protected]> 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, <[email protected]> wrote: >>> >>>> Dear Paul, >>>> >>>> On Thu, 4 Feb 2010 15:29:28 -0800 >>>> Paul Messmer <[email protected]> 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 >[email protected] >http://lists.nongnu.org/mailman/listinfo/freetype-devel _______________________________________________ Freetype-devel mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/freetype-devel
