[PATCH v2 0/1] MR423: ieframe fix

[David Kahurani (＠kahurani)]

Signed-off-by: David Kahurani <k.kahurani(a)gmail.com>

--
  v2: ieframe: Fix memory leak and possible reference leak
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/423

[PATCH v2 1/1] ieframe: Fix memory leak and possible reference leak

[David Kahurani]

From: David Kahurani <k.kahurani(a)gmail.com>

Signed-off-by: David Kahurani <k.kahurani(a)gmail.com>
---
 dlls/ieframe/iexplore.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/dlls/ieframe/iexplore.c b/dlls/ieframe/iexplore.c
index 1616d4d7f84..f1e1c5f7f60 100644
--- a/dlls/ieframe/iexplore.c
+++ b/dlls/ieframe/iexplore.c
@@ -1028,8 +1028,13 @@ static HDDEDATA open_dde_url(WCHAR *dde_url)
 
     hres = IWebBrowser2_Navigate2(&ie->IWebBrowser2_iface, &urlv, NULL, NULL, NULL, NULL);
     if(FAILED(hres))
+    {
+        SysFreeString(V_BSTR(&urlv));
+        IWebBrowser2_Release(&ie->IWebBrowser2_iface);
         return 0;
+    }
 
+    SysFreeString(V_BSTR(&urlv));
     IWebBrowser2_Release(&ie->IWebBrowser2_iface);
     return ULongToHandle(DDE_FACK);
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/423

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/ieframe/iexplore.c:
> hres = IWebBrowser2_Navigate2(&ie->IWebBrowser2_iface, &urlv, NULL, NULL, NULL, NULL);
>      if(FAILED(hres))
> +    {
> +        SysFreeString(V_BSTR(&urlv));
> +        IWebBrowser2_Release(&ie->IWebBrowser2_iface);
>          return 0;
> +    }
>
> +    SysFreeString(V_BSTR(&urlv));
>      IWebBrowser2_Release(&ie->IWebBrowser2_iface);
>      return ULongToHandle(DDE_FACK);
>  }

VariantClear() seems more fitting. Also, you don't need this duplication. Just use FAILED(hres) to return 0 or DDE_FACK.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/423#note_3881

[David Kahurani (＠kahurani)]

On Mon Jul 11 17:59:39 2022 +0000, Nikolay Sivov wrote:
> VariantClear() seems more fitting. Also, you don't need this
> duplication. Just use FAILED(hres) to return 0 or DDE_FACK.
> Hello.

Thanks for the review.

Yes, VariatClear() would indeed be more fitting but I used SysFreeString instead for consistency with the rest of the file. Should I ignore that? Or...maybe look into migrating the whole module?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/423#note_3882

[Jacek Caban (＠jacek)]

On Mon Jul 11 18:30:06 2022 +0000, David Kahurani wrote:
> Hello.
> Thanks for the review.
> Yes, VariatClear() would indeed be more fitting but I used SysFreeString
> instead for consistency with the rest of the file. Should I ignore that?
> Or...maybe look into migrating the whole module?
> Both VariantClear and SysFreeString seem fine to me here as long as it's not duplicated.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/423#note_3885