[PATCH] strmbase: Fix some memory leaks (Valgrind).

[Sven Baars]

Signed-off-by: Sven Baars <sven.wine(a)gmail.com>
---
 dlls/strmbase/mediatype.c | 14 +++++++++++---
 dlls/strmbase/renderer.c  |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/dlls/strmbase/mediatype.c b/dlls/strmbase/mediatype.c
index 91c0ddc60a..dd5773f5de 100644
--- a/dlls/strmbase/mediatype.c
+++ b/dlls/strmbase/mediatype.c
@@ -254,6 +254,7 @@ static HRESULT WINAPI IEnumMediaTypesImpl_Skip(IEnumMediaTypes * iface, ULONG cM
 static HRESULT WINAPI IEnumMediaTypesImpl_Reset(IEnumMediaTypes * iface)
 {
     ULONG i;
+    HRESULT hr;
     AM_MEDIA_TYPE amt;
     IEnumMediaTypesImpl *This = impl_from_IEnumMediaTypes(iface);
 
@@ -264,14 +265,21 @@ static HRESULT WINAPI IEnumMediaTypesImpl_Reset(IEnumMediaTypes * iface)
     CoTaskMemFree(This->enumMediaDetails.pMediaTypes);
 
     i = 0;
-    while (This->enumMediaFunction(This->basePin, i, &amt) == S_OK) i++;
+    while (This->enumMediaFunction(This->basePin, i, &amt) == S_OK)
+    {
+        FreeMediaType(&amt);
+        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)))
+        This->enumMediaFunction(This->basePin, i, &amt);
+        hr = CopyMediaType(&This->enumMediaDetails.pMediaTypes[i], &amt);
+        FreeMediaType(&amt);
+
+        if (FAILED(hr))
         {
             while (i--)
                 FreeMediaType(&This->enumMediaDetails.pMediaTypes[i]);
diff --git a/dlls/strmbase/renderer.c b/dlls/strmbase/renderer.c
index c06661ce73..3d5e4b5e7d 100644
--- a/dlls/strmbase/renderer.c
+++ b/dlls/strmbase/renderer.c
@@ -346,6 +346,7 @@ HRESULT WINAPI BaseRendererImpl_Receive(BaseRenderer *This, IMediaSample * pSamp
         {
             return VFW_E_TYPE_NOT_ACCEPTED;
         }
+        DeleteMediaType(pmt);
     }
 
     This->pMediaSample = pSample;
-- 
2.17.1

[PATCH] wsdapi: Fix some memory leaks (Valgrind).

[Sven Baars]

Signed-off-by: Sven Baars <sven.wine(a)gmail.com>
---
 dlls/wsdapi/soap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/dlls/wsdapi/soap.c b/dlls/wsdapi/soap.c
index 2ecc01313f..aa8d15f4c5 100644
--- a/dlls/wsdapi/soap.c
+++ b/dlls/wsdapi/soap.c
@@ -1426,6 +1426,9 @@ static HRESULT ws_element_to_wsdxml_element(WS_XML_READER *reader, IWSDXMLContex
                 if (FAILED(ret)) goto cleanup;
                 WSDXMLAddChild(cur_element, element);
 
+                WSDFreeLinkedMemory(name);
+                name = NULL;
+
                 cur_wsd_attrib = NULL;
 
                 /* Add attributes */
@@ -1462,6 +1465,9 @@ static HRESULT ws_element_to_wsdxml_element(WS_XML_READER *reader, IWSDXMLContex
                     new_wsd_attrib->Element = cur_element;
                     new_wsd_attrib->Next = NULL;
 
+                    WSDAttachLinkedMemory(new_wsd_attrib, name);
+                    name = NULL;
+
                     if (cur_wsd_attrib == NULL)
                         element->FirstAttribute = new_wsd_attrib;
                     else
@@ -1524,6 +1530,7 @@ outofmemory:
 cleanup:
     /* Free uri and element_name if applicable */
     WSDFreeLinkedMemory(uri);
+    WSDFreeLinkedMemory(name);
     return ret;
 }
 
@@ -1712,10 +1719,10 @@ HRESULT read_message(IWSDiscoveryPublisherImpl *impl, const char *xml, int xml_l
     IWSDXMLContext *context = NULL;
     WS_XML_STRING *soap_uri = NULL;
     const WS_XML_NODE *node;
-    WS_XML_READER *reader;
+    WS_XML_READER *reader = NULL;
     LPCWSTR value = NULL;
     LPWSTR uri, prefix;
-    WS_HEAP *heap;
+    WS_HEAP *heap = NULL;
     HRESULT ret;
     int i;
 
@@ -1945,6 +1952,8 @@ cleanup:
     free_xml_string(soap_uri);
     WSDFreeLinkedMemory(soap_msg);
     if (context != NULL) IWSDXMLContext_Release(context);
+    if (reader != NULL) WsFreeReader(reader);
+    if (heap != NULL) WsFreeHeap(heap);
 
     return ret;
 }
-- 
2.17.1

[PATCH] gdiplus: Don't return copies of font families from GdipGetFontCollectionFamilyList.

[Sven Baars]

Signed-off-by: Sven Baars <sven.wine(a)gmail.com>
---
This was added in 5694825ae3c3a581976716a62e1b2d1fb54e5674, but the
corrected test shows this may not be correct. Maybe Vincent still knows
why this was done?

 dlls/gdiplus/font.c       | 20 ++++----------------
 dlls/gdiplus/tests/font.c | 14 +++++++++-----
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c
index 5e6aa5430f..cbe7859fe7 100644
--- a/dlls/gdiplus/font.c
+++ b/dlls/gdiplus/font.c
@@ -1576,7 +1576,6 @@ GpStatus WINGDIPAPI GdipGetFontCollectionFamilyList(
         GpFontFamily* gpfamilies[], INT* numFound)
 {
     INT i;
-    GpStatus stat=Ok;
 
     TRACE("%p, %d, %p, %p\n", fontCollection, numSought, gpfamilies, numFound);
 
@@ -1585,24 +1584,13 @@ GpStatus WINGDIPAPI GdipGetFontCollectionFamilyList(
 
     memset(gpfamilies, 0, sizeof(*gpfamilies) * numSought);
 
-    for (i = 0; i < numSought && i < fontCollection->count && stat == Ok; i++)
+    for (i = 0; i < numSought && i < fontCollection->count; i++)
     {
-        stat = GdipCloneFontFamily(fontCollection->FontFamilies[i], &gpfamilies[i]);
+        gpfamilies[i] = fontCollection->FontFamilies[i];
     }
+    *numFound = i;
 
-    if (stat == Ok)
-        *numFound = i;
-    else
-    {
-        int numToFree=i;
-        for (i=0; i<numToFree; i++)
-        {
-            GdipDeleteFontFamily(gpfamilies[i]);
-            gpfamilies[i] = NULL;
-        }
-    }
-
-    return stat;
+    return Ok;
 }
 
 void free_installed_fonts(void)
diff --git a/dlls/gdiplus/tests/font.c b/dlls/gdiplus/tests/font.c
index 03e7a04e08..f91418d806 100644
--- a/dlls/gdiplus/tests/font.c
+++ b/dlls/gdiplus/tests/font.c
@@ -1187,7 +1187,7 @@ todo_wine
 
 static void test_GdipGetFontCollectionFamilyList(void)
 {
-    GpFontFamily *family, *family2;
+    GpFontFamily *family, *family2, **families;
     GpFontCollection *collection;
     INT found, count;
     GpStatus status;
@@ -1228,15 +1228,19 @@ static void test_GdipGetFontCollectionFamilyList(void)
     ok(found == 1, "Unexpected list count %d.\n", found);
     ok(family != NULL, "Expected family instance.\n");
 
-    family = NULL;
+    family2 = NULL;
     found = 0;
     status = GdipGetFontCollectionFamilyList(collection, 1, &family2, &found);
     ok(status == Ok, "Failed to get family list, status %d.\n", status);
     ok(found == 1, "Unexpected list count %d.\n", found);
-    ok(family2 != family, "Unexpected family instance.\n");
+    ok(family2 == family, "Unexpected family instance.\n");
 
-    GdipDeleteFontFamily(family);
-    GdipDeleteFontFamily(family2);
+    families = GdipAlloc((count + 1) * sizeof(*families));
+    found = 0;
+    status = GdipGetFontCollectionFamilyList(collection, count + 1, families, &found);
+    ok(status == Ok, "Failed to get family list, status %d.\n", status);
+    ok(found == count, "Unexpected list count %d, extected %d.\n", found, count);
+    GdipFree(families);
 }
 
 static void test_GdipGetFontCollectionFamilyCount(void)
-- 
2.17.1

[PATCH] gdiplus: Don't return copies of font families from GdipGetFontCollectionFamilyList.

[Nikolay Sivov]

On 1/24/19 1:59 PM, Sven Baars wrote:
> Signed-off-by: Sven Baars <sven.wine(a)gmail.com>
> ---
> This was added in 5694825ae3c3a581976716a62e1b2d1fb54e5674, but the
> corrected test shows this may not be correct. Maybe Vincent still knows
> why this was done?
>
> dlls/gdiplus/font.c       | 20 ++++----------------
>   dlls/gdiplus/tests/font.c | 14 +++++++++-----
>   2 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c
> index 5e6aa5430f..cbe7859fe7 100644
> --- a/dlls/gdiplus/font.c
> +++ b/dlls/gdiplus/font.c
> @@ -1576,7 +1576,6 @@ GpStatus WINGDIPAPI GdipGetFontCollectionFamilyList(
>           GpFontFamily* gpfamilies[], INT* numFound)
>   {
>       INT i;
> -    GpStatus stat=Ok;
>
> TRACE("%p, %d, %p, %p\n", fontCollection, numSought, gpfamilies, numFound);
>
> @@ -1585,24 +1584,13 @@ GpStatus WINGDIPAPI GdipGetFontCollectionFamilyList(
>
> memset(gpfamilies, 0, sizeof(*gpfamilies) * numSought);
>
> -    for (i = 0; i < numSought && i < fontCollection->count && stat == Ok; i++)
> +    for (i = 0; i < numSought && i < fontCollection->count; i++)
>       {
> -        stat = GdipCloneFontFamily(fontCollection->FontFamilies[i], &gpfamilies[i]);
> +        gpfamilies[i] = fontCollection->FontFamilies[i];
>       }
> +    *numFound = i;
>
> -    if (stat == Ok)
> -        *numFound = i;
> -    else
> -    {
> -        int numToFree=i;
> -        for (i=0; i<numToFree; i++)
> -        {
> -            GdipDeleteFontFamily(gpfamilies[i]);
> -            gpfamilies[i] = NULL;
> -        }
> -    }
> -
> -    return stat;
> +    return Ok;
>   }

Last time I looked at this one, I remember that it needed to be 
reference-counted.

Test path for that is to compare instance returned from GdipGetFamily() 
to what you get from collection.
> void free_installed_fonts(void)
> diff --git a/dlls/gdiplus/tests/font.c b/dlls/gdiplus/tests/font.c
> index 03e7a04e08..f91418d806 100644
> --- a/dlls/gdiplus/tests/font.c
> +++ b/dlls/gdiplus/tests/font.c
> @@ -1187,7 +1187,7 @@ todo_wine
>
> static void test_GdipGetFontCollectionFamilyList(void)
>   {
> -    GpFontFamily *family, *family2;
> +    GpFontFamily *family, *family2, **families;
>       GpFontCollection *collection;
>       INT found, count;
>       GpStatus status;
> @@ -1228,15 +1228,19 @@ static void test_GdipGetFontCollectionFamilyList(void)
>       ok(found == 1, "Unexpected list count %d.\n", found);
>       ok(family != NULL, "Expected family instance.\n");
>
> -    family = NULL;
> +    family2 = NULL;
>       found = 0;
>       status = GdipGetFontCollectionFamilyList(collection, 1, &family2, &found);
>       ok(status == Ok, "Failed to get family list, status %d.\n", status);
>       ok(found == 1, "Unexpected list count %d.\n", found);
> -    ok(family2 != family, "Unexpected family instance.\n");
> +    ok(family2 == family, "Unexpected family instance.\n");
>
> -    GdipDeleteFontFamily(family);
> -    GdipDeleteFontFamily(family2);
> +    families = GdipAlloc((count + 1) * sizeof(*families));
> +    found = 0;
> +    status = GdipGetFontCollectionFamilyList(collection, count + 1, families, &found);
> +    ok(status == Ok, "Failed to get family list, status %d.\n", status);
> +    ok(found == count, "Unexpected list count %d, extected %d.\n", found, count);
> +    GdipFree(families);
>   }
>
> static void test_GdipGetFontCollectionFamilyCount(void)

[PATCH] gdiplus: Don't return copies of font families from GdipGetFontCollectionFamilyList.

[Sven Baars]

On 24-01-19 12:12, Nikolay Sivov wrote:
> On 1/24/19 1:59 PM, Sven Baars wrote:
> > Signed-off-by: Sven Baars <sven.wine(a)gmail.com>
> > ---
> > This was added in 5694825ae3c3a581976716a62e1b2d1fb54e5674, but the
> > corrected test shows this may not be correct. Maybe Vincent still knows
> > why this was done?
> >
> >   dlls/gdiplus/font.c       | 20 ++++----------------
> >   dlls/gdiplus/tests/font.c | 14 +++++++++-----
> >   2 files changed, 13 insertions(+), 21 deletions(-)
> >
> > diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c
> > index 5e6aa5430f..cbe7859fe7 100644
> > --- a/dlls/gdiplus/font.c
> > +++ b/dlls/gdiplus/font.c
> > @@ -1576,7 +1576,6 @@ GpStatus WINGDIPAPI
> > GdipGetFontCollectionFamilyList(
> >           GpFontFamily* gpfamilies[], INT* numFound)
> >   {
> >       INT i;
> > -    GpStatus stat=Ok;
> >         TRACE("%p, %d, %p, %p\n", fontCollection, numSought,
> > gpfamilies, numFound);
> >   @@ -1585,24 +1584,13 @@ GpStatus WINGDIPAPI
> > GdipGetFontCollectionFamilyList(
> >         memset(gpfamilies, 0, sizeof(*gpfamilies) * numSought);
> >   -    for (i = 0; i < numSought && i < fontCollection->count && stat
> > == Ok; i++)
> > +    for (i = 0; i < numSought && i < fontCollection->count; i++)
> >       {
> > -        stat = GdipCloneFontFamily(fontCollection->FontFamilies[i],
> > &gpfamilies[i]);
> > +        gpfamilies[i] = fontCollection->FontFamilies[i];
> >       }
> > +    *numFound = i;
> >   -    if (stat == Ok)
> > -        *numFound = i;
> > -    else
> > -    {
> > -        int numToFree=i;
> > -        for (i=0; i<numToFree; i++)
> > -        {
> > -            GdipDeleteFontFamily(gpfamilies[i]);
> > -            gpfamilies[i] = NULL;
> > -        }
> > -    }
> > -
> > -    return stat;
> > +    return Ok;
> >   }
>
> Last time I looked at this one, I remember that it needed to be
> reference-counted.
>
> Test path for that is to compare instance returned from
> GdipGetFamily() to what you get from collection.

Apparently the font families are indeed not cloned for the fonts themselves:

https://testbot.winehq.org/JobDetails.pl?Key=46590

but using GdipDeleteFontFamily on the font family that is returned from
GdipGetFontCollectionFamilyList crashes on Windows as can be seen here

https://testbot.winehq.org/JobDetails.pl?Key=46598

whereas this succeeds (removing only the topmost test)

https://testbot.winehq.org/JobDetails.pl?Key=46594

so I don't think it is refcounted there (or the refcount is not
increased). At least I would assume that GdipDeleteFontFamily reduced
the refcount if it worked like that. That or
GdipNewPrivateFontCollection is just bugged since deleting the font
family does work in the other tests...

[PATCH] gdiplus: Don't return copies of font families from GdipGetFontCollectionFamilyList.

[Vincent Povirk]

I think I've seen applications delete font families enumerated from
the installed font collection.

Maybe there's no reference count, and installed fonts have a single
family object forever (or until GdiplusShutdown).

[PATCH] gdiplus: Don't return copies of font families from GdipGetFontCollectionFamilyList.

[Sven Baars]

On 24-01-19 16:25, Vincent Povirk wrote:
> I think I've seen applications delete font families enumerated from
> the installed font collection.
>
> Maybe there's no reference count, and installed fonts have a single
> family object forever (or until GdiplusShutdown).
> Should I just submit the improved test with the todo_wines and leave the
> memory leaks then?

[PATCH] gdiplus: Don't return copies of font families from GdipGetFontCollectionFamilyList.

[Vincent Povirk]

> Should I just submit the improved test with the todo_wines and leave the
> memory leaks then?

Yes, that sounds useful.

[PATCH] gdiplus: Fix a memory leak on error path (Valgrind).

[Sven Baars]

Signed-off-by: Sven Baars <sven.wine(a)gmail.com>
---
 dlls/gdiplus/image.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c
index 2e844cfcc5..8a136eff09 100644
--- a/dlls/gdiplus/image.c
+++ b/dlls/gdiplus/image.c
@@ -3570,7 +3570,11 @@ static GpStatus initialize_decoder_wic(IStream *stream, REFGUID container, IWICB
     if (FAILED(hr)) return hresult_to_status(hr);
 
     hr = IWICBitmapDecoder_Initialize(*decoder, stream, WICDecodeMetadataCacheOnLoad);
-    if (FAILED(hr)) return hresult_to_status(hr);
+    if (FAILED(hr))
+    {
+        IWICBitmapDecoder_Release(*decoder);
+        return hresult_to_status(hr);
+    }
     return Ok;
 }
 
-- 
2.17.1

[PATCH] gdiplus: Fix a memory leak on error path (Valgrind).

[Vincent Povirk]

Signed-off-by: Vincent Povirk <vincent(a)codeweavers.com>

[PATCH] windowscodecs: Fix a memory leak (Valgrind).

[Sven Baars]

Signed-off-by: Sven Baars <sven.wine(a)gmail.com>
---
 dlls/windowscodecs/pngformat.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlls/windowscodecs/pngformat.c b/dlls/windowscodecs/pngformat.c
index 7050c95121..faffeae6ae 100644
--- a/dlls/windowscodecs/pngformat.c
+++ b/dlls/windowscodecs/pngformat.c
@@ -620,7 +620,6 @@ static HRESULT WINAPI PngDecoder_Initialize(IWICBitmapDecoder *iface, IStream *p
     if (setjmp(jmpbuf))
     {
         ppng_destroy_read_struct(&This->png_ptr, &This->info_ptr, &This->end_info);
-        HeapFree(GetProcessHeap(), 0, row_pointers);
         This->png_ptr = NULL;
         hr = WINCODEC_ERR_UNKNOWNIMAGEFORMAT;
         goto end;
@@ -816,6 +815,9 @@ static HRESULT WINAPI PngDecoder_Initialize(IWICBitmapDecoder *iface, IStream *p
 end:
     LeaveCriticalSection(&This->lock);
 
+    if (row_pointers)
+        HeapFree(GetProcessHeap(), 0, row_pointers);
+
     return hr;
 }
 
-- 
2.17.1

[PATCH] windowscodecs: Fix a memory leak (Valgrind).

[Vincent Povirk]

Signed-off-by: Vincent Povirk <vincent(a)codeweavers.com>

[Alexandre Julliard]

Sven Baars <sven.wine(a)gmail.com> writes:
> for (i = 0; i < This->enumMediaDetails.cMediaTypes; i++)
>      {
> -        This->enumMediaFunction(This->basePin, i,&amt);
> -        if (FAILED(CopyMediaType(&This->enumMediaDetails.pMediaTypes[i], &amt)))
> +        This->enumMediaFunction(This->basePin, i, &amt);
> +        hr = CopyMediaType(&This->enumMediaDetails.pMediaTypes[i], &amt);
> +        FreeMediaType(&amt);

The extra copy doesn't seem necessary, you could fetch directly into the
destination array.

-- 
Alexandre Julliard
julliard(a)winehq.org

[Sven Baars]

On 29-01-19 19:21, Alexandre Julliard wrote:
> Sven Baars <sven.wine(a)gmail.com> writes:
> > for (i = 0; i < This->enumMediaDetails.cMediaTypes; i++)
> >      {
> > -        This->enumMediaFunction(This->basePin, i,&amt);
> > -        if (FAILED(CopyMediaType(&This->enumMediaDetails.pMediaTypes[i], &amt)))
> > +        This->enumMediaFunction(This->basePin, i, &amt);
> > +        hr = CopyMediaType(&This->enumMediaDetails.pMediaTypes[i], &amt);
> > +        FreeMediaType(&amt);
>
> The extra copy doesn't seem necessary, you could fetch directly into the
> destination array.

That seems to work indeed. I will send a new patch. Thanks!