Re: [PATCH] winegstreamer: Fix memory leak in amt_from_gst_caps_video.

[Andrew Eikum]

This is an improvement, but I think we should go further and fix leaks
in all of the failure cases. The problem is we're using the
amt_from_gst_caps_video return value inconsistently. In
accept_caps_sink, we unconditionally free the resulting AM_MEDIA_TYPE.
But in setcaps_sink, we only free it, via the pin->pmt assignment, if
amt_from_gst_caps_video succeeds.

I think we should always free the allocated memory in the failure
cases in amt_from_gst_caps_video, and then only free the resulting AMT
in the calling functions if amt_from_gst_caps_video succeeded. The
resulting AMT should never be used or freed if amt_from_gst_caps_video
fails.

Note that amt_from_gst_caps_audio has the same inconsistent return
type handling. I would accept a separate patch to fix this handling,
too. (Unlike for video, it doesn't currently matter in practice
because of amt_from_gst_caps_audio's implementation.)

Does that make sense?

Andrew

On Mon, Nov 14, 2016 at 11:43:21PM -0700, Alex Henrie wrote:
> Cc: Andrew Eikum
>
> Coverity #713781, "Variable vih going out of scope leaks the storage it
> points to."
> Signed-off-by: Alex Henrie
>
> dlls/winegstreamer/gstdemux.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/winegstreamer/gstdemux.c b/dlls/winegstreamer/gstdemux.c
> index a6a41ab..5d97510 100644
> --- a/dlls/winegstreamer/gstdemux.c
> +++ b/dlls/winegstreamer/gstdemux.c
> @@ -179,18 +179,21 @@ static gboolean amt_from_gst_caps_audio(GstCaps *caps, AM_MEDIA_TYPE *amt)
>
> static gboolean amt_from_gst_caps_video(GstCaps *caps, AM_MEDIA_TYPE *amt)
>  {
> - VIDEOINFOHEADER *vih = CoTaskMemAlloc(sizeof(*vih));
> - BITMAPINFOHEADER *bih = &vih->bmiHeader;
> -
>
> VIDEOINFOHEADER *vih;
> -
>
> BITMAPINFOHEADER *bih;
> gint32 width = 0, height = 0, nom = 0, denom = 0;
> GstVideoInfo vinfo;
>
> if (!gst_video_info_from_caps (&vinfo, caps))
>     return FALSE;
> width = vinfo.width;
> height = vinfo.height;
> nom = vinfo.fps_n;
> denom = vinfo.fps_d;
> -
>
> vih = CoTaskMemAlloc(sizeof(*vih));
> -
>
> bih = &vih->bmiHeader;
> -
>
> amt->formattype = FORMAT_VideoInfo;
>    amt->pbFormat = (BYTE*)vih;
>    amt->cbFormat = sizeof(*vih);
>
> -- 
> 2.10.2

[PATCH] winegstreamer: Fix memory leak in amt_from_gst_caps_video.

[Alex Henrie]

2016-11-15 7:08 GMT-07:00 Andrew Eikum :
> This is an improvement, but I think we should go further and fix leaks
> in all of the failure cases. The problem is we're using the
> amt_from_gst_caps_video return value inconsistently. In
> accept_caps_sink, we unconditionally free the resulting AM_MEDIA_TYPE.
> But in setcaps_sink, we only free it, via the pin->pmt assignment, if
> amt_from_gst_caps_video succeeds.
>
> I think we should always free the allocated memory in the failure
> cases in amt_from_gst_caps_video, and then only free the resulting AMT
> in the calling functions if amt_from_gst_caps_video succeeded. The
> resulting AMT should never be used or freed if amt_from_gst_caps_video
> fails.
>
> Note that amt_from_gst_caps_audio has the same inconsistent return
> type handling. I would accept a separate patch to fix this handling,
> too. (Unlike for video, it doesn't currently matter in practice
> because of amt_from_gst_caps_audio's implementation.)
>
> Does that make sense?

Yes, that makes sense. I'll send new patches tonight, unless you beat
me to it ;-)

-Alex