bugThe FreeType Project - Bugs: bug #39702, Wrong offset checks in...

 
 

bug #39702: Wrong offset checks in cff_index_get_pointers

Submitted by:  John Tytgat <joty>
Submitted on:  Mon 05 Aug 2013 06:32:09 PM UTC  
 
Severity: 3 - NormalItem Group: None
Status: FixedPrivacy: Public
Assigned to: Werner LEMBERG <wl>Open/Closed: Closed
Planned Release: 2.5.1

Add a New Comment(Rich Markup)
   

You are not logged in

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

 

Tue 06 Aug 2013 07:00:25 AM UTC, comment #1:

Thanks a lot for the patch; I've applied it to the git repository.

Note that this code is used for the new CFF engine also since loading of the glyph data hasn't been replaced.

Werner LEMBERG <wl>
Project AdministratorIn charge of this item.
Mon 05 Aug 2013 06:32:09 PM UTC, original submission:

I'm not sure if there is still interest in reporting issues in the 'old' cff code but in case there is :

The offset values in a CFF INDEX structure are being sanity checked in cff_index_get_pointers while building an equivalent pointer array.

cur_offset = idx->offsets[0] - 1;

/* sanity check */
if ( cur_offset >= idx->data_size )
{
FT_TRACE0(( "cff_index_get_pointers:"
" invalid first offset value %d set to zero\n",

cur_offset is the first offset to be found in the INDEX structure minus one. By definition this needs to be value 0 (cfr. Adobe TechNote 5176, section 5). So this test can be made stronger by checking for cur_offset != 0.

/* empty slot + two sanity checks for invalid offset tables */
if ( next_offset == 0 ||
next_offset < cur_offset ||
( next_offset >= idx->data_size && n < idx->count ) )

These tests are not 100% correct. Basically you want to protect the code using the pointer array later on so that it can rely on monotonic increasing pointer values so that cff_decoder_parse_charstrings() can always have 'ip' growing towards 'limit', cfr. its local/global subrs handling.

The first subtest 'next_offset == 0' is already covered by 'next_offset < cur_offset' as the type is unsigned, so that can be dropped. However, the third subtest is actually wrong for INDEX structures which have one or more empty entries at the end as the last non-empty entry will actually also made empty as it matches the 'next_offset >= idx->data_size && n < idx->count' subtest.

I believe these can be better replaced by:

if ( next_offset < cur_offset )
next_offset = cur_offset;
else if ( next_offset > idx->data_size )
next_offset = idx->data_size;

I.e. it just clamps the next_offset between the current offset and the maximum value it can ever have.

I've attached a diff covering those changes and also a subset font which wrong triggers the 'next_offset >= idx->data_size && n < idx->count' subtest for its gid 66 glyph.

John Tytgat <joty>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #28764:  cff_index_get_pointers.patch added by joty (1KiB - application/octet-stream)
file #28765:  DT106250 added by joty (5KiB - application/octet-stream)

 

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 joty (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 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 06 Aug 2013 07:00:25 AM UTCwlStatusNone=>Fixed
      Assigned toNone=>wl
      Open/ClosedOpen=>Closed
      Planned ReleaseNone=>2.5.1
    Mon 05 Aug 2013 06:32:09 PM UTCjotyAttached File-=>Added cff_index_get_pointers.patch, #28764
      Attached File-=>Added DT106250, #28765

    Back to the top


    Powered by Savane 3.1-cleanup1