[PATCH 2/2] [base] Ensure FT_Open_Face always closes user-supplied stream on error

[Oleg Oshmyan]

This was already true (though undocumented) most of the time, but not
if a FT_NEW inside FT_Stream_New failed or if the FT_OPEN flags were bad.

* include/freetype/freetype.h (FT_Open_Face): Note explicitly that any
user-supplied stream is closed if an error is returned.

* src/base/ftobjs.c (FT_Stream_New): Don't allocate unnecessary buffer.
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 illegal combination of multiple FT_OPEN mode flags or a clean
FT_OPEN_STREAM flag on a FreeType build that lacks stream support.
---
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`.

But currently, `FT_Stream_New` allocates a new stream even if it is
already given one by the user. And 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's 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`
flags are unsupported, which can mean either an illegal 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.

 include/freetype/freetype.h |  4 ++++
 src/base/ftobjs.c           | 31 ++++++++++++++++++-------------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/include/freetype/freetype.h b/include/freetype/freetype.h
index 9230401a8..b651e4c62 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 4783a09c7..4d8ce3db3 100644
--- a/src/base/ftobjs.c
+++ b/src/base/ftobjs.c
@@ -211,14 +211,12 @@
     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 ( 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 );
@@ -229,8 +227,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 )
     {
@@ -238,21 +240,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;
-- 
2.32.0