[freetype2] master 5d27b10 2/2: [base] Fix `FT_Open_Face`'s handling of user-supplied streams.

[Werner Lemberg]

branch: master
commit 5d27b10f4c6c8e140bd48a001b98037ac0d54118
Author: Oleg Oshmyan <chortos@inbox.lv>
Commit: Werner Lemberg <wl@gnu.org>

    [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 5a40b09..c8a0795 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,51 @@
 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
diff --git a/include/freetype/freetype.h b/include/freetype/freetype.h
index 598abd8..566f56d 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 f7b2b3f..0ded244 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;