bugThe FreeType Project - Bugs: bug #31040, Invalid read

 
 

bug #31040: Invalid read

Submitted by:  Werner LEMBERG <wl>
Submitted on:  Wed 15 Sep 2010 05:05:17 AM UTC  
 
Severity: 3 - NormalItem Group: Crash
Status: FixedPrivacy: Public
Assigned to: suzuki toshiya <mpsuzuki>Open/Closed: Closed
Planned Release: 2.4.3

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 01 Oct 2010 06:16:52 AM UTC, comment #11:

I've applied it to the git repository, thanks.

Werner LEMBERG <wl>
Project Administrator
Mon 20 Sep 2010 08:16:32 AM UTC, comment #10:

Sorry for my misunderstanding the original post.
The issue you raised was the invalid pointer
dereference (valgrind report as "invalid read"),
but I was working for FT_Stream's invalid reading.

My fix for #30798 was designed to free the buffer
in CUR->size, I expected its effect is only to
size-related functions and they are aware of
cvt_ready or bytecode_ready flags. But some
pointers in CUR->size are copied to CUR.
tt_loader_init() invokes tt_size_ready_bytecode()
and copies size->context to current context.

1888 if ( !size->cvt_ready )
1889 {
1890 FT_Error error = tt_size_ready_bytecode( size );
1891 if ( error )
1892 return error;
1893 }
1894
1895 /* query new execution context */
1896 exec = size->debug ? size->context
1897 : ( (TT_Driver)FT_FACE_DRIVER( face ) )->context;
1898 if ( !exec )
1899 return TT_Err_Could_Not_Find_Context;
1900
1901 grayscale =
1902 FT_BOOL( FT_LOAD_TARGET_MODE( load_flags ) != FT_RENDER_MODE_MONO );
1903
1904 TT_Load_Context( exec, face, size );

Therefore, if the buffers in size are freed, such
restoration is needed. Although I feel it is unsafe
to reuse the buffers in size which might be garbled,
free/realloc the buffers in size and replace the
pointers in the current context could have some
side effect.

Thus, I remove the code to free the buffers and
unset the flag. As tt_size_reset() unsets cvt_ready
flag only, I leave bytecode_ready flag. Still it can
prevent the crash in #30798.
# If I unset bytecode_ready, new bytecode buffer is
# allocated and so the pointer replacement is essential.

(file #21506)

suzuki toshiya <mpsuzuki>
Project AdministratorIn charge of this item.
Sun 19 Sep 2010 07:01:28 PM UTC, comment #9:

Hmm, valgrind still shows the `Invalid read' error (see attached file). I believe that my original remark is still valid...

(file #21499)

Werner LEMBERG <wl>
Project Administrator
Sun 19 Sep 2010 04:37:30 PM UTC, comment #8:

Just I've committed my patches to GIT, thank you for review.

suzuki toshiya <mpsuzuki>
Project AdministratorIn charge of this item.
Sun 19 Sep 2010 08:11:26 AM UTC, comment #7:

Looks good! Thanks for your work.

Werner LEMBERG <wl>
Project Administrator
Fri 17 Sep 2010 03:47:29 PM UTC, comment #6:

Just I've fixed SEGV issue in GIT.
Here I attach the updated patch
including the fix to prevent glyf
and post overrunning.

(file #21479)

suzuki toshiya <mpsuzuki>
Project AdministratorIn charge of this item.
Fri 17 Sep 2010 01:37:49 PM UTC, comment #5:

About SEGV issue, it was a bug in my free_buffers_in_size().
FT_FREE( ptr ) overwrites ptr by NULL after free( ptr )
to prevent careless reference of the freed buffer.
free_buffers_in_size() copies size->twilight to local storage
twilight, and free its members. Thus, the original pointers
in size->twilight are not overwritten. If free_buffers_in_size()
is invoked after the normal free, the original pointers in
size->twilight are still non-NULL values and free_buffers_in_size()
tries to free them and cause double free crash. The fix will be
following.

diff --git a/src/truetype/ttinterp.c b/src/truetype/ttinterp.c
index 6401009..a083bbb 100644
--- a/src/truetype/ttinterp.c
+++ b/src/truetype/ttinterp.c
@@ -7364,9 +7364,8 @@
static void
free_buffer_in_size( TT_ExecContext exc )
{
- FT_Memory memory = exc->memory;
- TT_Size size = exc->size;
- TT_GlyphZoneRec twilight;
+ FT_Memory memory = exc->memory;
+ TT_Size size = exc->size;

if ( !size )
@@ -7381,18 +7380,16 @@
if ( size->storage )
FT_FREE( size->storage );

- twilight = size->twilight;
-
- if ( twilight.org )
- FT_FREE( twilight.org );
- if ( twilight.cur )
- FT_FREE( twilight.cur );
- if ( twilight.orus )
- FT_FREE( twilight.orus );
- if ( twilight.tags )
- FT_FREE( twilight.tags );
- if ( twilight.contours )
- FT_FREE( twilight.contours );
+ if ( size->twilight.org )
+ FT_FREE( size->twilight.org );
+ if ( size->twilight.cur )
+ FT_FREE( size->twilight.cur );
+ if ( size->twilight.orus )
+ FT_FREE( size->twilight.orus );
+ if ( size->twilight.tags )
+ FT_FREE( size->twilight.tags );
+ if ( size->twilight.contours )
+ FT_FREE( size->twilight.contours );
}

I will commit to GIT soon.

suzuki toshiya <mpsuzuki>
Project AdministratorIn charge of this item.
Thu 16 Sep 2010 10:17:50 AM UTC, comment #4:

The stream related errors I found are:
1) invalid offsets in loca table (some offsets exceeds the end of glyf table)
2) too many entries declared in post table header (post is too short to include all of them)

Following patch checks the end of glyf & post table and
prevents the attempt to read by invalid offset.

diff --git a/src/truetype/ttpload.c b/src/truetype/ttpload.c
index a311b03..c531733 100644
--- a/src/truetype/ttpload.c
+++ b/src/truetype/ttpload.c
@@ -203,6 +203,26 @@
}
}

+ /* Check broken location data */
+ if ( pos1 >= face->glyf_len )
+ {
+ FT_ERROR(( "tt_face_get_location:"
+ " too large offset=0x%08lx found for gid=0x%04lx,"
+ " exceeding the end of glyf table (0x%08lx)\n",
+ pos1, gindex, face->glyf_len ));
+ *asize = 0;
+ return 0;
+ }
+
+ if ( pos2 >= face->glyf_len )
+ {
+ FT_ERROR(( "tt_face_get_location:"
+ " too large offset=0x%08lx found for gid=0x%04lx,"
+ " truncate at the end of glyf table (0x%08lx)\n",
+ pos2, gindex + 1, face->glyf_len ));
+ pos2 = face->glyf_len;
+ }
+
/* The `loca' table must be ordered; it refers to the length of */
/* an entry as the difference between the current and the next */
/* position. However, there do exist (malformed) fonts which */
diff --git a/src/sfnt/ttpost.c b/src/sfnt/ttpost.c
index aa0bf1e..5059fd5 100644
--- a/src/sfnt/ttpost.c
+++ b/src/sfnt/ttpost.c
@@ -153,7 +153,8 @@

static FT_Error
load_format_20( TT_Face face,
- FT_Stream stream )
+ FT_Stream stream,
+ FT_ULong post_limit )
{
FT_Memory memory = stream->memory;
FT_Error error;
@@ -230,8 +231,29 @@
FT_UInt len;

- if ( FT_READ_BYTE ( len ) ||
- FT_NEW_ARRAY( name_strings[n], len + 1 ) ||
+ FT_TRACE7(( "load_format_20: %d byte left in post table\n",
+ post_limit - FT_STREAM_POS() ));
+
+ if ( FT_STREAM_POS() >= post_limit )
+ {
+ FT_ERROR(( "load_format_20:"
+ " all entries in post table is already parsed,"
+ " put NULL name for gid=%d\n", n ));
+ len = 0;
+ }
+ else if ( FT_READ_BYTE( len ) )
+ goto Fail1;
+
+ if ( len > 0 && FT_STREAM_POS() + len > post_limit )
+ {
+ FT_ERROR(( "load_format_20:"
+ " too large string length (%d)"
+ " truncate at the end of post table (%d byte left)\n",
+ len, post_limit - FT_STREAM_POS() ));
+ len = FT_MAX( 0, post_limit - FT_STREAM_POS() );
+ }
+
+ if ( FT_NEW_ARRAY( name_strings[n], len + 1 ) ||
FT_STREAM_READ ( name_strings[n], len ) )
goto Fail1;

@@ -271,7 +293,8 @@

static FT_Error
load_format_25( TT_Face face,
- FT_Stream stream )
+ FT_Stream stream,
+ FT_ULong post_limit )
{
FT_Memory memory = stream->memory;
FT_Error error;
@@ -338,16 +361,19 @@
FT_Stream stream;
FT_Error error;
FT_Fixed format;
+ FT_ULong post_len, post_limit;

/* get a stream for the face's resource */
stream = face->root.stream;

/* seek to the beginning of the PS names table */
- error = face->goto_table( face, TTAG_post, stream, 0 );
+ error = face->goto_table( face, TTAG_post, stream, &post_len );
if ( error )
goto Exit;

+ post_limit = FT_STREAM_POS() + post_len;
+
format = face->postscript.FormatType;

/* go to beginning of subtable */
@@ -356,9 +382,9 @@

/* now read postscript table */
if ( format == 0x00020000L )
- error = load_format_20( face, stream );
+ error = load_format_20( face, stream, post_limit );
else if ( format == 0x00028000L )
- error = load_format_25( face, stream );
+ error = load_format_25( face, stream, post_limit );
else
error = SFNT_Err_Invalid_File_Format;

By the combination of the patch to prevent double-free
(attached file includes all of them), I got no crash, no FT_Stream
related warning, and no memory leak. Is this expected solution?

(file #21471)

suzuki toshiya <mpsuzuki>
Project AdministratorIn charge of this item.
Thu 16 Sep 2010 07:08:11 AM UTC, comment #3:

I guess `invalid read' is caused by broken loca entry (pointing the buffer
exceeding the end of glyf table), now I insert boundary check in
tt_face_get_location() and testing.

suzuki toshiya <mpsuzuki>
Project AdministratorIn charge of this item.
Wed 15 Sep 2010 08:02:06 AM UTC, comment #2:

Thanks. However, this doesn't fix the `invalid read' issue, as you've certainly noticed.

Werner LEMBERG <wl>
Project Administrator
Wed 15 Sep 2010 07:56:02 AM UTC, comment #1:

glibc reports that the crash might be caused by double-free.
I was thinking FT_FREE() changes the pointer to NULL after
freeing the buffer and double-free won't be occur, it would
be my mistake. I've inserted the checks of the flags that were
set when the buffers are allocated before freeing them, and
the crash disappears. I attached the patch for temporal fix,
but I have to check more what occurs before the crash and
write official fix. Please wait.

diff --git a/builds/unix/install-sh b/builds/unix/install-sh
old mode 100644
new mode 100755
diff --git a/src/truetype/ttinterp.c b/src/truetype/ttinterp.c
index a3eeaa6..68391f4 100644
--- a/src/truetype/ttinterp.c
+++ b/src/truetype/ttinterp.c
@@ -7372,27 +7372,39 @@
if ( !size )
return;

- if ( size->function_defs )
- FT_FREE( size->function_defs );
- if ( size->instruction_defs )
- FT_FREE( size->instruction_defs );
- if ( size->cvt )
- FT_FREE( size->cvt );
- if ( size->storage )
- FT_FREE( size->storage );
+ /* free buffers allocated by tt_size_init_bytecode(),
+ * tt_size_init_bytecode() sets `bytecode_ready' flag.
+ */
+ if ( size->bytecode_ready )
+ {
+ if ( size->function_defs )
+ FT_FREE( size->function_defs );
+ if ( size->instruction_defs )
+ FT_FREE( size->instruction_defs );
+ if ( size->cvt )
+ FT_FREE( size->cvt );
+ if ( size->storage )
+ FT_FREE( size->storage );
+ }

twilight = size->twilight;

- if ( twilight.org )
- FT_FREE( twilight.org );
- if ( twilight.cur )
- FT_FREE( twilight.cur );
- if ( twilight.orus )
- FT_FREE( twilight.orus );
- if ( twilight.tags )
- FT_FREE( twilight.tags );
- if ( twilight.contours )
- FT_FREE( twilight.contours );
+ /* free buffers allocated by tt_size_ready_bytecode(),
+ * tt_size_ready_bytecode() sets `cvt_ready' flag.
+ */
+ if ( size->cvt_ready )
+ {
+ if ( twilight.org )
+ FT_FREE( twilight.org );
+ if ( twilight.cur )
+ FT_FREE( twilight.cur );
+ if ( twilight.orus )
+ FT_FREE( twilight.orus );
+ if ( twilight.tags )
+ FT_FREE( twilight.tags );
+ if ( twilight.contours )
+ FT_FREE( twilight.contours );
+ }
}

(file #21463)

suzuki toshiya <mpsuzuki>
Project AdministratorIn charge of this item.
Wed 15 Sep 2010 05:05:17 AM UTC, original submission:

This bug is probably related to #30798; I think we need to re-execute the CVT for subglyphs also in case an error has occurred.

Attached is the valgrind output of

FT2_DEBUG=any:7 valgrind ftview -f 137 20 1.ttf

together with the font itself.

Toshiya-san, can you have a look, please?

Werner LEMBERG <wl>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #21499:  1.tff.log.xz added by wl (107KiB - application/octet-stream)
file #21460:  1.ttf added by wl (201KiB - application/x-font-ttf)
file #21461:  1.ttf.log.xz added by wl (97KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mpsuzuki (Updated the item)
  • -unavailable- added by wl (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 16 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 01 Oct 2010 06:16:52 AM UTCwlStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Mon 20 Sep 2010 08:16:32 AM UTCmpsuzukiAttached File-=>Added bug31040_mps20100920a.diff, #21506
    Sun 19 Sep 2010 07:01:28 PM UTCwlAttached File-=>Added 1.tff.log.xz, #21499
      StatusFixed=>In Progress
      Open/ClosedClosed=>Open
    Sun 19 Sep 2010 04:38:10 PM UTCmpsuzukiOpen/ClosedOpen=>Closed
    Sun 19 Sep 2010 04:37:30 PM UTCmpsuzukiStatusReady For Test=>Fixed
      Planned ReleaseNone=>2.4.3
    Fri 17 Sep 2010 03:47:29 PM UTCmpsuzukiAttached File-=>Added bug31040_mps20100917a.diff, #21479
      StatusIn Progress=>Ready For Test
    Thu 16 Sep 2010 10:17:50 AM UTCmpsuzukiAttached File-=>Added bug31040_mps20100916a.diff, #21471
    Wed 15 Sep 2010 07:56:02 AM UTCmpsuzukiAttached File-=>Added savannah-31040_20100915a.diff, #21463
      StatusConfirmed=>In Progress
    Wed 15 Sep 2010 05:05:17 AM UTCwlAttached File-=>Added 1.ttf, #21460
      Attached File-=>Added 1.ttf.log.xz, #21461

    Back to the top


    Powered by Savane 3.1-cleanup1