From 5d27b10f4c6c8e140bd48a001b98037ac0d54118 Mon Sep 17 00:00:00 2001 From: Oleg Oshmyan Date: Tue, 13 Jul 2021 10:59:32 +0200 Subject: [PATCH] [base] Fix `FT_Open_Face`'s handling of user-supplied streams. This was already true (though undocumented) most of the time, but not if `FT_NEW` inside `FT_Stream_New` failed or if the `FT_OPEN_XXX` flags were bad. Normally, `FT_Open_Face` calls `FT_Stream_New`, which returns the user-supplied stream unchanged, and in case of any subsequent error in `FT_Open_Face`, the stream is closed via `FT_Stream_Free`. Up to now, however, `FT_Stream_New` allocates a new stream even if it is already given one by the user. If this allocation fails, the user-supplied stream is not returned to `FT_Open_Face` and never closed. Moreover, the user cannot detect this situation: all they see is that `FT_Open_Face` returns `FT_Err_Out_Of_Memory`, but that can also happen after a different allocation fails within the main body of `FT_Open_Face`, when the user's stream has already been closed by `FT_Open_Face`. It is plausible that the user stream's `close` method frees memory allocated for the stream object itself, so the user cannot defensively free it upon `FT_Open_Face` failure lest it ends up doubly freed. All in all, this ends up leaking the memory/resources used by user's stream. Furthermore, `FT_Stream_New` simply returns an error if the `FT_OPEN_XXX` flags are unsupported, which can mean either an invalid combination of flags or a perfectly innocent `FT_OPEN_STREAM` on a FreeType build that lacks stream support. With this patch, the user-supplied stream is closed even in these cases, so the user can be sure that if `FT_Open_Face` failed, the stream is definitely closed. * src/base/ftobjs.c (FT_Stream_New): Don't allocate a buffer unnecessarily. Move error-handling code to make the control flow more obvious. Close user-supplied stream if the flags are unsupported. `FT_Stream_Open` always sets `pathname.pointer`, so remove the redundant (re)assignment. None of the `FT_Stream_Open...` functions uses `stream->memory`, so keep just one assignment at the end, shared among all possible control flow paths. ('Unsupported flags' that may need a stream closure can be either an invalid combination of multiple `FT_OPEN_XXX` mode flags or a clean `FT_OPEN_STREAM` flag on a FreeType build that lacks stream support.) --- ChangeLog | 46 +++++++++++++++++++++++++++++++++++++ include/freetype/freetype.h | 4 ++++ src/base/ftobjs.c | 31 ++++++++++++++----------- 3 files changed, 68 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5a40b09c8..c8a079568 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,49 @@ +2021-07-13 Oleg Oshmyan + + [base] Fix `FT_Open_Face`'s handling of user-supplied streams. + + This was already true (though undocumented) most of the time, but + not if `FT_NEW` inside `FT_Stream_New` failed or if the + `FT_OPEN_XXX` flags were bad. + + Normally, `FT_Open_Face` calls `FT_Stream_New`, which returns the + user-supplied stream unchanged, and in case of any subsequent error + in `FT_Open_Face`, the stream is closed via `FT_Stream_Free`. + + Up to now, however, `FT_Stream_New` allocates a new stream even if + it is already given one by the user. If this allocation fails, the + user-supplied stream is not returned to `FT_Open_Face` and never + closed. Moreover, the user cannot detect this situation: all they + see is that `FT_Open_Face` returns `FT_Err_Out_Of_Memory`, but that + can also happen after a different allocation fails within the main + body of `FT_Open_Face`, when the user's stream has already been + closed by `FT_Open_Face`. It is plausible that the user stream's + `close` method frees memory allocated for the stream object itself, + so the user cannot defensively free it upon `FT_Open_Face` failure + lest it ends up doubly freed. All in all, this ends up leaking the + memory/resources used by user's stream. + + Furthermore, `FT_Stream_New` simply returns an error if the + `FT_OPEN_XXX` flags are unsupported, which can mean either an + invalid combination of flags or a perfectly innocent + `FT_OPEN_STREAM` on a FreeType build that lacks stream support. + With this patch, the user-supplied stream is closed even in these + cases, so the user can be sure that if `FT_Open_Face` failed, the + stream is definitely closed. + + * src/base/ftobjs.c (FT_Stream_New): Don't allocate a buffer + unnecessarily. + Move error-handling code to make the control flow more obvious. + Close user-supplied stream if the flags are unsupported. + `FT_Stream_Open` always sets `pathname.pointer`, so remove the + redundant (re)assignment. None of the `FT_Stream_Open...` functions + uses `stream->memory`, so keep just one assignment at the end, + shared among all possible control flow paths. + ('Unsupported flags' that may need a stream closure can be either an + invalid combination of multiple `FT_OPEN_XXX` mode flags or a clean + `FT_OPEN_STREAM` flag on a FreeType build that lacks stream + support.) + 2021-07-13 Oleg Oshmyan [base] Reject combinations of incompatible `FT_OPEN_XXX` flags. diff --git a/include/freetype/freetype.h b/include/freetype/freetype.h index 598abd8c9..566f56d7b 100644 --- a/include/freetype/freetype.h +++ b/include/freetype/freetype.h @@ -2301,6 +2301,10 @@ FT_BEGIN_HEADER * See the discussion of reference counters in the description of * @FT_Reference_Face. * + * If `FT_OPEN_STREAM` is set in `args->flags`, the stream in + * `args->stream` is automatically closed before this function returns + * any error (including `FT_Err_Invalid_Argument`). + * * @example: * To loop over all faces, use code similar to the following snippet * (omitting the error handling). diff --git a/src/base/ftobjs.c b/src/base/ftobjs.c index f7b2b3f18..0ded2440f 100644 --- a/src/base/ftobjs.c +++ b/src/base/ftobjs.c @@ -212,14 +212,12 @@ mode = args->flags & ( FT_OPEN_MEMORY | FT_OPEN_STREAM | FT_OPEN_PATHNAME ); - if ( FT_NEW( stream ) ) - goto Exit; - - stream->memory = memory; - if ( mode == FT_OPEN_MEMORY ) { /* create a memory-based stream */ + if ( FT_NEW( stream ) ) + goto Exit; + FT_Stream_OpenMemory( stream, (const FT_Byte*)args->memory_base, (FT_ULong)args->memory_size ); @@ -230,8 +228,12 @@ else if ( mode == FT_OPEN_PATHNAME ) { /* create a normal system stream */ + if ( FT_NEW( stream ) ) + goto Exit; + error = FT_Stream_Open( stream, args->pathname ); - stream->pathname.pointer = args->pathname; + if ( error ) + FT_FREE( stream ); } else if ( ( mode == FT_OPEN_STREAM ) && args->stream ) { @@ -239,21 +241,24 @@ /* in this case, we do not need to allocate a new stream object */ /* since the caller is responsible for closing it himself */ - FT_FREE( stream ); stream = args->stream; + error = FT_Err_Ok; } #endif else + { error = FT_THROW( Invalid_Argument ); + if ( ( args->flags & FT_OPEN_STREAM ) && args->stream ) + FT_Stream_Close( args->stream ); + } - if ( error ) - FT_FREE( stream ); - else - stream->memory = memory; /* just to be certain */ - - *astream = stream; + if ( !error ) + { + stream->memory = memory; + *astream = stream; + } Exit: return error;