Commits (2)
  • Oleg Oshmyan's avatar
    [base] Reject combinations of incompatible `FT_OPEN_XXX` flags. · a4c8f21a
    Oleg Oshmyan authored
    The three modes are mutually exclusive, and the documentation of the
    `FT_OPEN_XXX` constants notes this.  However, there was no check to
    validate this in the code, and the documentation on `FT_Open_Args`
    claimed that the corresponding bits were checked in a well-defined
    order, implying it was valid (if useless) to specify more than one.
    Ironically, this documented order did not agree with the actual
    code, so it could not be relied upon; hopefully, nobody did this and
    nobody will be hurt by the new validation.
    
    Even if multiple mode bits were allowed, they could cause memory
    leaks: if both `FT_OPEN_STREAM` and `stream` are set along with
    either `FT_OPEN_MEMORY` or `FT_OPEN_PATHNAME`, then `FT_Stream_New`
    allocated a new stream but `FT_Open_Face` marked it as an 'external'
    stream, so the stream object was never released.
    
    * src/base/ftobjs.c (FT_Stream_New): Reject incompatible
    `FT_OPEN_XXX` flags.
    a4c8f21a
  • Oleg Oshmyan's avatar
    [base] Fix `FT_Open_Face`'s handling of user-supplied streams. · 5d27b10f
    Oleg Oshmyan authored
    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.)
    5d27b10f
2021-07-13 Oleg Oshmyan <chortos@inbox.lv>
[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 <chortos@inbox.lv>
[base] Reject combinations of incompatible `FT_OPEN_XXX` flags.
The three modes are mutually exclusive, and the documentation of the
`FT_OPEN_XXX` constants notes this. However, there was no check to
validate this in the code, and the documentation on `FT_Open_Args`
claimed that the corresponding bits were checked in a well-defined
order, implying it was valid (if useless) to specify more than one.
Ironically, this documented order did not agree with the actual
code, so it could not be relied upon; hopefully, nobody did this and
nobody will be hurt by the new validation.
Even if multiple mode bits were allowed, they could cause memory
leaks: if both `FT_OPEN_STREAM` and `stream` are set along with
either `FT_OPEN_MEMORY` or `FT_OPEN_PATHNAME`, then `FT_Stream_New`
allocated a new stream but `FT_Open_Face` marked it as an 'external'
stream, so the stream object was never released.
* src/base/ftobjs.c (FT_Stream_New): Reject incompatible
`FT_OPEN_XXX` flags.
2021-07-12 Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
* meson.build: Fix build for other UNIX systems (e.g., FreeBSD).
......
......@@ -2113,8 +2113,7 @@ FT_BEGIN_HEADER
* Extra parameters passed to the font driver when opening a new face.
*
* @note:
* The stream type is determined by the contents of `flags` that are
* tested in the following order by @FT_Open_Face:
* The stream type is determined by the contents of `flags`:
*
* If the @FT_OPEN_MEMORY bit is set, assume that this is a memory file
* of `memory_size` bytes, located at `memory_address`. The data are not
......@@ -2127,6 +2126,9 @@ FT_BEGIN_HEADER
* Otherwise, if the @FT_OPEN_PATHNAME bit is set, assume that this is a
* normal file and use `pathname` to open it.
*
* If none of the above bits are set or if multiple are set at the same
* time, the flags are invalid and @FT_Open_Face fails.
*
* If the @FT_OPEN_DRIVER bit is set, @FT_Open_Face only tries to open
* the file with the driver whose handler is in `driver`.
*
......@@ -2299,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).
......
......@@ -197,6 +197,7 @@
FT_Error error;
FT_Memory memory;
FT_Stream stream = NULL;
FT_UInt mode;
*astream = NULL;
......@@ -208,15 +209,15 @@
return FT_THROW( Invalid_Argument );
memory = library->memory;
mode = args->flags &
( FT_OPEN_MEMORY | FT_OPEN_STREAM | FT_OPEN_PATHNAME );
if ( FT_NEW( stream ) )
goto Exit;
stream->memory = memory;
if ( args->flags & FT_OPEN_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 );
......@@ -224,33 +225,40 @@
#ifndef FT_CONFIG_OPTION_DISABLE_STREAM_SUPPORT
else if ( args->flags & FT_OPEN_PATHNAME )
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 ( ( args->flags & FT_OPEN_STREAM ) && args->stream )
else if ( ( mode == FT_OPEN_STREAM ) && args->stream )
{
/* use an existing, user-provided stream */
/* 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;
......