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)
|
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.
|
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)
|
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)
|