Re: icmstream mem leak fix

[Mike McCormack]

Hi James,

That looks kind of wrong, both the before and after...
Interfaces should be released, not free'd...

Secondly, freeing the interface if the Query works is almost definitely 
wrong, because ppv will likely point to the same memory as pstream.

Mike

James Hawkins wrote:
> hr = IAVIStream_QueryInterface((IAVIStream*)pstream, riid, ppv);
> - if (FAILED(hr))
> - LocalFree((HLOCAL)pstream);
> -
> -
>
> LocalFree((HLOCAL)pstream);
>
> return hr;
>
> }

icmstream mem leak fix

[James Hawkins]

Hey Mike,

Ignoring the change that I made, what is wrong with the current code? 
I kindof figured that pstream was tied into ppv, but I wanted to make
sure.  If that is the case, then my acmstream patch should be ignored
as well.  I thought to call LocalFree on pstream because it was
allocated with LocalAlloc.  Can you explain what should be written
instead so I don't make the same mistake?

On Tue, 30 Nov 2004 16:07:18 +0900, Mike McCormack  wrote:
> Hi James,
>
> That looks kind of wrong, both the before and after...
> Interfaces should be released, not free'd...
>
> Secondly, freeing the interface if the Query works is almost definitely
> wrong, because ppv will likely point to the same memory as pstream.
>
> Mike
>
> James Hawkins wrote:
> > hr = IAVIStream_QueryInterface((IAVIStream*)pstream, riid, ppv);
> > - if (FAILED(hr))
> > - LocalFree((HLOCAL)pstream);
> > -
> > -
> >
> > LocalFree((HLOCAL)pstream);
> >
> > return hr;
> >
> > }

icmstream mem leak fix

[Mike McCormack]

James Hawkins wrote:
> Ignoring the change that I made, what is wrong with the current code? 
> I kindof figured that pstream was tied into ppv, but I wanted to make
> sure.  If that is the case, then my acmstream patch should be ignored
> as well.  I thought to call LocalFree on pstream because it was
> allocated with LocalAlloc.  Can you explain what should be written
> instead so I don't make the same mistake?

Actually, the current code will work correctly.  My nitpick is only that 
you should use Release() instead of free() on an interface pointer.

Mike

Index: dlls/avifil32/acmstream.c
===================================================================
RCS file: /home/wine/wine/dlls/avifil32/acmstream.c,v
retrieving revision 1.14
diff -u -r1.14 acmstream.c
--- dlls/avifil32/acmstream.c	5 Oct 2004 18:10:21 -0000	1.14
+++ dlls/avifil32/acmstream.c	30 Nov 2004 08:21:29 -0000
@@ -118,10 +118,10 @@
     return AVIERR_MEMORY;

pstream->lpVtbl = &iacmst;
+  pstream->ref = 1;

hr = IAVIStream_QueryInterface((IAVIStream*)pstream, riid, ppv);
-  if (FAILED(hr))
-    LocalFree((HLOCAL)pstream);
+  IAVIStream_Release((IAVIStream*)pstream);

return hr;
 }

icmstream mem leak fix

[James Hawkins]

On Tue, 30 Nov 2004 16:32:59 +0900, Mike McCormack  wrote:
> James Hawkins wrote:
> > Ignoring the change that I made, what is wrong with the current code?
> > I kindof figured that pstream was tied into ppv, but I wanted to make
> > sure.  If that is the case, then my acmstream patch should be ignored
> > as well.  I thought to call LocalFree on pstream because it was
> > allocated with LocalAlloc.  Can you explain what should be written
> > instead so I don't make the same mistake?
>
> Actually, the current code will work correctly.  My nitpick is only that
> you should use Release() instead of free() on an interface pointer.
>
> Mike
> Index: dlls/avifil32/acmstream.c
>
> RCS file: /home/wine/wine/dlls/avifil32/acmstream.c,v
> retrieving revision 1.14
> diff -u -r1.14 acmstream.c
> --- dlls/avifil32/acmstream.c   5 Oct 2004 18:10:21 -0000       1.14
> +++ dlls/avifil32/acmstream.c   30 Nov 2004 08:21:29 -0000
> @@ -118,10 +118,10 @@
>      return AVIERR_MEMORY;
>
> pstream->lpVtbl = &iacmst;
> -
>
> pstream->ref = 1;
>
> hr = IAVIStream_QueryInterface((IAVIStream*)pstream, riid, ppv);
> - if (FAILED(hr))
> - LocalFree((HLOCAL)pstream);
> -
>
> IAVIStream_Release((IAVIStream*)pstream);
>
> return hr;
>
> }

I just want to make sure I've got this.  Do we want to release even if
the call to QueryInterface suceeds?

icmstream mem leak fix

[Mike McCormack]

James Hawkins wrote:
> I just want to make sure I've got this.  Do we want to release even if
> the call to QueryInterface suceeds?

The old code sets ref=0, then just free's the memory.  My method would 
set ref=1, then call Release() which decrements ref and free's memory.

If QueryInterface succeeds, then it will increment the reference count 
again, so we'll only end up free'ing memory in Release if the 
QueryInterface fails.  It's a more COM-ish way of doing things... the 
original code still works though.

Mike