[Git][freetype/freetype][master] 3 commits: [base] Fix leak of internal

Werner Lemberg pushed to branch master at FreeType / FreeType

Commits:

13983b05

by Ben Wagner at 2023-01-17T08:48:33+01:00

[base] Fix leak of internal stream marked external.

`open_face_from_buffer` allocates a new `FT_Stream` to pass to
`ft_open_face_internal`.  Because this is an `FT_OPEN_STREAM`,
`ft_open_face_internal` will mark this as an 'external stream', which the
caller must free.  However, `open_face_from_buffer` cannot directly free it
because the stream must last as long as the face.  There is currently an
attempt at this by clearing the 'external stream' bit after
`open_face_from_buffer` returns successfully.  However, this is too late as
the original stream may have already been closed and the stream on the face
may not be the same stream as originally passed.

It is tempting to use `FT_OPEN_MEMORY` and let `ft_open_face_internal`
create the stream internally.  However, with this method there is no means
to pass through a 'close' function to the created stream to free the
underlying data, which must be owned by the stream.

A possibility is to check on success if the stream of the face is the same
as the original stream.  If it is then unset the external flag.  If not,
then free the original stream.  Unfortunately, while no current
implementation does so, it is possible that the face still has the original
stream somewhere other than as the `FT_FaceRec::stream`.  The stream needs
to remain available for the life of the face or until it is closed,
whichever comes earlier.

The approach taken here is to let the stream own itself.  When the stream is
closed it will free itself.

* src/base/ftobjs.c (memory_stream_close): Free `stream`.
(open_face_from_buffer): Simplify error handling, since
`ft_open_face_internal` always closes `args.stream` on any error.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=54930

0d4f887c

by Ben Wagner at 2023-01-17T08:54:11+01:00

[base] Always close user-provided stream.

The `FT_Open_Face` documentation states

> 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`).

However, if the user provides a stream in `args.stream` with
`FT_OPEN_STREAM` set and a `close` function, but then for some reason passes
NULL for `aface` and a non-negative `face_index`, the error
`Invalid_Argument` is returned but the `close` callback will not be called
on the user-provided stream.  This may cause resource leaks if the caller is
depending on the `close` callback to free resources.

The difficulty is that a user may fill out a `FT_StreamRec` and pass its
address as `args.stream`, but the stream isn't really 'live' until
`FT_Stream_New` is called on it (and `memory` is set).  In particular, it
cannot really be cleaned up properly in `ft_open_face_internal` until the
stream pointer has been copied into the `stream` local variable.

* src/base/ftobj.c (ft_open_face_internal): Ensure that user-provided
`args.stream.close` is called even with early errors.

29f83d1d

by Ben Wagner at 2023-01-17T08:59:25+01:00

[base] 'close' callback may not use `stream->memory`.

The documentation for `FT_StreamRec::memory` states that it 'shouldn't be
touched by stream implementations'.  This is true even for internal
implementations of the 'close' callback, since it is not guaranteed that
`memory` will even be set when the 'close' callback occurs.

* src/base/ftobjs.c (new_memory_stream): stash current `memory` in
`stream->descriptor`.
(memory_stream_close): Use it.

1 changed file:

src/base/ftobjs.c

Changes:

src/base/ftobjs.c

@@ -1672,13 +1672,13 @@
    static void
    memory_stream_close( FT_Stream  stream )
+   {
 -    FT_Memory  memory = stream->memory;
 +    FT_Memory  memory = (FT_Memory)stream->descriptor.pointer;
      FT_FREE( stream->base );
+-
      stream->size  = 0;
      stream->close = NULL;
 +    FT_FREE( stream );
+   }
@@ -1709,7 +1709,8 @@
      FT_Stream_OpenMemory( stream, base, size );
 -    stream->close = close;
 +    stream->descriptor.pointer = memory;
 +    stream->close              = close;
      *astream = stream;
@@ -1734,6 +1735,7 @@
      FT_Memory     memory = library->memory;
 +    /* `memory_stream_close` also frees the stream object. */
      error = new_memory_stream( library,
                                 base,
                                 size,
@@ -1763,21 +1765,7 @@
        face_index &= 0x7FFF0000L; /* retain GX data */
  #endif
 -    error = ft_open_face_internal( library, &args, face_index, aface, 0 );
+-
 -    if ( !error )
 -      (*aface)->face_flags &= ~FT_FACE_FLAG_EXTERNAL_STREAM;
 -    else
 -#ifdef FT_MACINTOSH
 -      FT_Stream_Free( stream, 0 );
 -#else
 -    {
 -      FT_Stream_Close( stream );
 -      FT_FREE( stream );
 -    }
 -#endif
+-
 -    return error;
 +    return ft_open_face_internal( library, &args, face_index, aface, 0 );
+   }
@@ -2557,7 +2545,7 @@
      /* test for valid `library' delayed to `FT_Stream_New' */
 -    if ( ( !aface && face_index >= 0 ) || !args )
 +    if ( !args )
        return FT_THROW( Invalid_Argument );
      external_stream = FT_BOOL( ( args->flags & FT_OPEN_STREAM ) &&
@@ -2568,6 +2556,14 @@
      if ( error )
        goto Fail3;
 +    /* Do this error check after `FT_Stream_New` to ensure that the */
 +    /* 'close' callback is called.                                  */
 +    if ( !aface && face_index >= 0 )
 +    {
 +      error = FT_THROW( Invalid_Argument );
 +      goto Fail3;
 +    }
++
      memory = library->memory;
      /* If the font driver is specified in the `args' structure, use */

From:	Werner Lemberg (@wl)
Subject:	[Git][freetype/freetype][master] 3 commits: [base] Fix leak of internal stream marked external.
Date:	Tue, 17 Jan 2023 08:01:12 +0000