Re: [PATCH 1/5] winegstreamer: Fix multiple leaks in failure pathes.

[Zebediah Figura]

On 9/1/21 4:05 PM, Derek Lesho wrote:
> Signed-off-by: Derek Lesho
>
> dlls/winegstreamer/media_source.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)

Typo in the title...
> diff --git a/dlls/winegstreamer/media_source.c b/dlls/winegstreamer/media_source.c
> index 01ab626254a..383afcfce75 100644
> --- a/dlls/winegstreamer/media_source.c
> +++ b/dlls/winegstreamer/media_source.c
> @@ -733,6 +733,9 @@ static HRESULT new_media_stream(struct media_source *source,
>       object->IMFMediaStream_iface.lpVtbl = &media_stream_vtbl;
>       object->ref = 1;
> - if (FAILED(hr = MFCreateEventQueue(&object->event_queue)))
> -
>    goto fail;
> -
> IMFMediaSource_AddRef(&source->IMFMediaSource_iface);
> object->parent_source = source;
> object->stream_id = stream_id;
>
> @@ -741,9 +744,6 @@ static HRESULT new_media_stream(struct media_source *source,
>       object->eos = FALSE;
>       object->wg_stream = wg_stream;
> -
>
> if (FAILED(hr = MFCreateEventQueue(&object->event_queue)))
> -
>    goto fail;
> -
> TRACE("Created stream object %p.\n", object);
>
> *out_stream = object;

I don't particularly want to nitpick mfplat code, but,

(1) you're using a goto label only once, and

(2) it devolves into a free(), which seems easier and clearer.

Personally I tend to prefer making cleanup explicit in initialization 
functions. It's less fragile; you can be sure that you won't break 
anything by changing your destroy method. It also tends to require less 
conditionals in the destroy method.

This would also end up making most of the rest of the hunks in this 
patch go away.
> @@ -1211,7 +1211,8 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)
>   source->state = SOURCE_SHUTDOWN;
>
> - unix_funcs->wg_parser_disconnect(source->wg_parser);
> -
>
> if (source->stream_count)
> -
>    unix_funcs->wg_parser_disconnect(source->wg_parser);
>
> if (source->read_thread)
> {
>
> @@ -1231,6 +1232,9 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)
>       {
>           struct media_stream *stream = source->streams[i];
> -
>    if (!stream)
> -
>        continue;
> -
>     stream->state = STREAM_SHUTDOWN;
>
>     if (stream->event_queue)
>
> @@ -1245,7 +1249,7 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface)
>   unix_funcs->wg_parser_destroy(source->wg_parser);
>
> - if (source->stream_count)
> -
>
> if (source->streams)
>      free(source->streams);
>
> if (source->async_commands_queue)
>
> @@ -1353,7 +1357,6 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_
>           if (FAILED(hr = media_stream_init_desc(object->streams[i])))
>           {
>               ERR("Failed to finish initialization of media stream %p, hr %x.\n", object->streams[i], hr);
> -
>        IMFMediaStream_Release(&object->streams[i]->IMFMediaStream_iface);
>         goto fail;
>     }
> }
>
> @@ -1392,6 +1395,8 @@ static HRESULT media_source_constructor(IMFByteStream *bytestream, struct media_
>       fail:
>       WARN("Failed to construct MFMediaSource, hr %#x.\n", hr);
> - if (object->wg_parser)
> -
>    IMFMediaSource_Shutdown(&object->IMFMediaSource_iface);

A moot point if you take my advice, but, media_source_Release() already 
does this. Not to mention that conditional looks really janky.
>   free(descriptors);
>   IMFMediaSource_Release(&object->IMFMediaSource_iface);
>   return hr;