bugThe FreeType Project - Bugs: bug #51833, ft_raster1_render() calls...

 
 

bug #51833: ft_raster1_render() calls ft_mem_realloc() with item and item_size and new_count swapped

Submitted by:  Patrick J. Daly <pjdaly>
Submitted on:  Wed 23 Aug 2017 10:58:24 PM UTC  
 
Severity: 3 - NormalItem Group: Incorrect behaviour
Status: FixedPrivacy: Public
Assigned to: NoneOpen/Closed: Closed
Planned Release: None

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 29 Sep 2017 05:01:25 PM UTC, comment #8:

src/smooth/ftsmooth.c now also uses FT_ALLOC_MULT. We are done.

Alexei Podtelezhnikov <podtelez>
Project Member
Fri 25 Aug 2017 02:03:03 PM UTC, comment #7:

I'll work on it. I see three instances.

src/base/ftbitmap.c:229: if ( FT_QALLOC_MULT( buffer, new_pitch, bitmap->rows + ypixels ) )
src/winfonts/winfnt.c:1098: if ( FT_ALLOC_MULT( bitmap->buffer, pitch, bitmap->rows ) )
src/raster/ftrend1.c:197: if ( FT_ALLOC_MULT( bitmap->buffer, pitch, height ) )

I will also see if this one can be converted.

src/smooth/ftsmooth.c:282: if ( FT_ALLOC( bitmap->buffer, (FT_ULong)( pitch * height ) ) )

Alexei Podtelezhnikov <podtelez>
Project Member
Fri 25 Aug 2017 01:54:17 PM UTC, comment #6:

OK, I'm setting the status to `postponed'. Please report back any new findings or patches :-)

Werner LEMBERG <wl>
Project Administrator
Fri 25 Aug 2017 01:29:53 PM UTC, comment #5:

Oh dear... we are not set up to build and run the latest. We are running with 2.5.0. This is in a product so moving up takes some porting time and rigorous QA and of course management sign-offs. Maybe they'll let me do the next port/up-rev but for now I won't be able to provide a patch to the latest and I don't feel comfortable with &quot;blind edits&quot;, i.e. build/test with 2.5 then submit a 2.8 patch. It's sort of a trivial change change and &quot;it should work&quot; but we all know Murphy's Law applies. ;-)

Patrick J. Daly <pjdaly>
Fri 25 Aug 2017 07:58:35 AM UTC, comment #4:

Patrick, can you provide a patch? There are some other places in FreeType where I can see similar issues with swapped arguments to FT_ALLOC_MULT.

Alexei, I guess the main reasons for not using FT_ALLOC_MULT are that contributors either don't know that this macro exists, or they don't care and directly multiply :-)

Werner LEMBERG <wl>
Project Administrator
Fri 25 Aug 2017 02:12:14 AM UTC, comment #3:

I kind of agree with you that it is strange: the FreeType bitmaps are stored row-by-row, yet this one takes the column-by-column view. FT_REALLOC_MULT zeros out additional "columns", luckily it is never used.

I also wonder why FT_ALLOC_MULT was not used in the smooth rasterizer or other places.

Alexei Podtelezhnikov <podtelez>
Project Member
Thu 24 Aug 2017 07:05:16 PM UTC, comment #2:

You can close this if you want. I was debating whether I should even mention it because I expected to hear that it doesn't matter.

We're taking a hard look at memory allocations because substituting FT2 for our usual font renderer is taking 1/3 more time for a particular test file and 74% more allocation calls.

The crash was in my hacked-in temp/throw-away debug code to see what sort of allocations the FT2 code is doing. It was just an uninit variable on my part. But since my attention was already heavily focused on allocations, I paused when I saw 63 as an item size.

It made me think perhaps the argument was clobbered or uninit because other than seeing 1, 2, 4, or 8 for byte, short, pointers I expected to see nicely rounded structures allocated and 63 ain't.

So again, feel free to close it. Thanks.

Patrick J. Daly <pjdaly>
Thu 24 Aug 2017 05:45:08 PM UTC, comment #1:

The FT_(RE)ALLOC_MULT macros check for multiplication overflows. The order does not seem to matter with respect to the overflow check.

Why would you want to change it?

Alexei Podtelezhnikov <podtelez>
Project Member
Wed 23 Aug 2017 10:58:24 PM UTC, original submission:

This is minor, I think, but I noticed it when looking for a crash and saw a funny item_size 63 with new_count 12.
I had some debug code that was checking the arguments to realloc and it found the item_size 63.

There is a macro FT_ALLOC_MULT( ptr, count, item_size ) that is called from ft_raster1_render() like so:

pitch = ( ( width + 15 ) >> 4 ) << 1;
bitmap->pixel_mode = FT_PIXEL_MODE_MONO;

bitmap->width = width;
bitmap->rows = height;
bitmap->pitch = (int)pitch;

if ( FT_ALLOC_MULT( bitmap->buffer, pitch, height ) )
^^^^^^^^^^^^^ swapped
goto Exit;

(gdb) print pitch
$13 = 12
(gdb) print height
$14 = 63

#0 ft_mem_qrealloc (memory=0xefc14f18, item_size=63, cur_count=0, new_count=12, block=0x0, p_error=0xffffc3dc) at ../freetype2/ftutil.c:228
#1 0x082541e9 in ft_mem_realloc (memory=0xefc14f18, item_size=63, cur_count=0, new_count=12, block=0x0,
^^^^^^^^^^^^^ ^^^^^^^^^^^^
p_error=0xffffc454) at ../freetype2/ftutil.c:189
#2 0x0829367b in ft_raster1_render (render=0xefc236c8, slot=0xefcb2558, mode=FT_RENDER_MODE_MONO, origin=0x0) at ../freetype2/ftrend1.c:259

Patrick J. Daly <pjdaly>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by wl (Posted a comment)
  • -unavailable- added by podtelez (Posted a comment)
  • -unavailable- added by pjdaly (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 29 Sep 2017 05:01:25 PM UTCpodtelezStatusPostponed=>Fixed
      Open/ClosedOpen=>Closed
    Fri 25 Aug 2017 01:54:17 PM UTCwlStatusNone=>Postponed

    Back to the top


    Powered by Savane 3.1-cleanup1