Re: mlang: fix memory leaks in error path (found by Smatch).

[Tijl Coosemans]

On Monday 24 September 2007 14:20:51 Lionel_Debroux wrote:
> ConvertINetString leaks some heap memory in an error path. Found in
> Michael Stefaniuc's list of Wine potential bugs detected by Smatch.
>
> 2007-09-24  Lionel Debroux <lionel_debroux(a)yahoo.fr>
>        * dlls/mlang/mlang.c:
>        mlang: fix memory leaks in error path (found by Smatch).
>
> diff --git a/dlls/mlang/mlang.c b/dlls/mlang/mlang.c
> index eb14ad0..c6274c5 100644
> --- a/dlls/mlang/mlang.c
> +++ b/dlls/mlang/mlang.c
> @@ -635,9 +635,10 @@ HRESULT WINAPI ConvertINetString(
>          pDstStrW = HeapAlloc(GetProcessHeap(), 0, cDstSizeW * sizeof(WCHAR));
>          hr = ConvertINetMultiByteToUnicode(pdwMode, dwSrcEncoding, pSrcStr, pcSrcSize, pDstStrW, &cDstSizeW);
>          if (hr != S_OK)
> -            return hr;
> +            goto cleanup;
>  
>          hr = ConvertINetUnicodeToMultiByte(pdwMode, dwDstEncoding, pDstStrW, &cDstSizeW, pDstStr, pcDstSize);
> +cleanup:
>          HeapFree(GetProcessHeap(), 0, pDstStrW);
>          return hr;
>      }

This is a bikeshed issue really, but I wonder what Wine's policy on
gotos like this is. Imo, it's better to add the HeapFree call to the if
block and let the compiler work it out.

mlang: fix memory leaks in error path (found by Smatch).

[Dmitry Timoshkov]

"Tijl Coosemans" <tijl(a)ulyssis.org> wrote:
> > hr = ConvertINetMultiByteToUnicode(pdwMode, dwSrcEncoding, pSrcStr, pcSrcSize, pDstStrW, &cDstSizeW);
> >          if (hr != S_OK)
> > -            return hr;
> > +            goto cleanup;
> >
> > hr = ConvertINetUnicodeToMultiByte(pdwMode, dwDstEncoding, pDstStrW, &cDstSizeW, pDstStr, pcDstSize);
> > +cleanup:
> >          HeapFree(GetProcessHeap(), 0, pDstStrW);
> >          return hr;
> >      }
>
> This is a bikeshed issue really, but I wonder what Wine's policy on
> gotos like this is. Imo, it's better to add the HeapFree call to the if
> block and let the compiler work it out.

In this particular case goto should be avoided IMO by rewriting code
snippet like below:

if (hr == S_OK)
    hr = ConvertINetUnicodeToMultiByte(...);

-- 
Dmitry.

mlang: fix memory leaks in error path (found by Smatch).

[Michael Stefaniuc]

Tijl Coosemans wrote:
> On Monday 24 September 2007 14:20:51 Lionel_Debroux wrote:
> > ConvertINetString leaks some heap memory in an error path. Found in
> > Michael Stefaniuc's list of Wine potential bugs detected by Smatch.
> >
> > 2007-09-24  Lionel Debroux <lionel_debroux(a)yahoo.fr>
> >        * dlls/mlang/mlang.c:
> >        mlang: fix memory leaks in error path (found by Smatch).
> >
> > diff --git a/dlls/mlang/mlang.c b/dlls/mlang/mlang.c
> > index eb14ad0..c6274c5 100644
> > --- a/dlls/mlang/mlang.c
> > +++ b/dlls/mlang/mlang.c
> > @@ -635,9 +635,10 @@ HRESULT WINAPI ConvertINetString(
> >          pDstStrW = HeapAlloc(GetProcessHeap(), 0, cDstSizeW * sizeof(WCHAR));
> >          hr = ConvertINetMultiByteToUnicode(pdwMode, dwSrcEncoding, pSrcStr, pcSrcSize, pDstStrW, &cDstSizeW);
> >          if (hr != S_OK)
> > -            return hr;
> > +            goto cleanup;
> >
> > hr = ConvertINetUnicodeToMultiByte(pdwMode, dwDstEncoding, pDstStrW, &cDstSizeW, pDstStr, pcDstSize);
> > +cleanup:
> >          HeapFree(GetProcessHeap(), 0, pDstStrW);
> >          return hr;
> >      }
>
> This is a bikeshed issue really, but I wonder what Wine's policy on
> gotos like this is. Imo, it's better to add the HeapFree call to the if
> block and let the compiler work it out.
> It really depends on the code. If you have a lot of error paths with a
> lot of cleanup to do in them then gotos are way cleaner and easier to read.

bye
	michael
-- 
Michael Stefaniuc               Tel.: [phone]
Sr. Network Engineer            Fax.: [phone]
--------------------------------------------------------------------
Reg. Adresse: Red Hat GmbH, Hauptstätter Strasse 58, 70178 Stuttgart
Handelsregister: Amtsgericht Stuttgart HRB 153243
Geschäftsführer: Brendan Lane, Charlie Peters, Michael Cunningham,
                 Werner Knoblich