Re: [PATCH] amstream: Initialize correctly AM_MEDIA_TYPE struct.

[Michael Stefaniuc]

Hello Christian,

On 11/15/2012 09:43 AM, Christian Costa wrote:
> Fixes bug 32185.
>
> dlls/amstream/mediastreamfilter.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/dlls/amstream/mediastreamfilter.c b/dlls/amstream/mediastreamfilter.c
> index 9b2bd8e..d5388e5 100644
> --- a/dlls/amstream/mediastreamfilter.c
> +++ b/dlls/amstream/mediastreamfilter.c
> @@ -132,6 +132,8 @@ static HRESULT WINAPI BasePinImp_GetMediaType(BasePin *This, int index, AM_MEDIA
>      MSPID purpose_id;
>      int i;
> - ZeroMemory(amt, sizeof(*amt));
> -

this looks "odd". There is no check if amt is NULL, at least that's what
caught my attention. So I've looked around and stumbled upon the
AM_MEDIA_TYPE structure documentation
http://msdn.microsoft.com/en-us/library/windows/desktop/dd373477%28v=vs.85%2...
  "pUnk
       Not used. Set to NULL."

Setting that to NULL in amstream's BasePinImp_GetMediaType() fixes the
crash for me. But who's responsibility is to set / check that? That
function or the caller aka test_media_streams() ?

bye
    michael

[PATCH] amstream: Initialize correctly AM_MEDIA_TYPE struct.

[Christian Costa]

Le 16/11/2012 00:24, Michael Stefaniuc a écrit :
> Hello Christian,
>
> On 11/15/2012 09:43 AM, Christian Costa wrote:
> > Fixes bug 32185.
> >
> > dlls/amstream/mediastreamfilter.c |    2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/dlls/amstream/mediastreamfilter.c b/dlls/amstream/mediastreamfilter.c
> > index 9b2bd8e..d5388e5 100644
> > --- a/dlls/amstream/mediastreamfilter.c
> > +++ b/dlls/amstream/mediastreamfilter.c
> > @@ -132,6 +132,8 @@ static HRESULT WINAPI BasePinImp_GetMediaType(BasePin *This, int index, AM_MEDIA
> >       MSPID purpose_id;
> >       int i;
> > - ZeroMemory(amt, sizeof(*amt));
> > -
>
> this looks "odd". There is no check if amt is NULL, at least that's what
> caught my attention. So I've looked around and stumbled upon the
> AM_MEDIA_TYPE structure documentation
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd373477%28v=vs.85%2...
>    "pUnk
>         Not used. Set to NULL."
>
> Setting that to NULL in amstream's BasePinImp_GetMediaType() fixes the
> crash for me. But who's responsibility is to set / check that? That
> function or the caller aka test_media_streams() ?
>
> bye
>    michael

This is an helper function called by winestrmbase. It's internal. There 
is no need to do a null check.
The crash depends of the stack content.

AM_MEDIA_TYPE amt;
while (This->enumMediaFunction(This->basePin, i,&amt) == S_OK) i++;

Where supposed to fill all the structure fields. Off course we can make 
winestrmbase function zero it before calling the helper.

Here is the entire function

static HRESULT WINAPI IEnumMediaTypesImpl_Reset(IEnumMediaTypes * iface)
{
     ULONG i;
     AM_MEDIA_TYPE amt;
     IEnumMediaTypesImpl *This = impl_from_IEnumMediaTypes(iface);

TRACE("()\n");

for (i = 0; i < This->enumMediaDetails.cMediaTypes; i++)
         if (This->enumMediaDetails.pMediaTypes[i].pbFormat)
CoTaskMemFree(This->enumMediaDetails.pMediaTypes[i].pbFormat);
     CoTaskMemFree(This->enumMediaDetails.pMediaTypes);

i = 0;
     while (This->enumMediaFunction(This->basePin, i,&amt) == S_OK) i++;

This->enumMediaDetails.cMediaTypes = i;
     This->enumMediaDetails.pMediaTypes = 
CoTaskMemAlloc(sizeof(AM_MEDIA_TYPE) * i);
     for (i = 0; i < This->enumMediaDetails.cMediaTypes; i++)
     {
         This->enumMediaFunction(This->basePin, i,&amt);
         if 
(FAILED(CopyMediaType(&This->enumMediaDetails.pMediaTypes[i], &amt)))
         {
            while (i--)
CoTaskMemFree(This->enumMediaDetails.pMediaTypes[i].pbFormat);
            CoTaskMemFree(This->enumMediaDetails.pMediaTypes);
            return E_OUTOFMEMORY;
         }
     }

This->currentVersion = This->mediaVersionFunction(This->basePin);
     This->uIndex = 0;

return S_OK;
}