[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.)
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139
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;