Re: RFC: winealsa.drv: fix memory leak (found by Smatch)

[Michael Stefaniuc]

Hello Lionel,

the patch has different problems:
- lpNewData is used only in "case MOD_MIDIPORT" so moving the HeapAlloc
call there makes more sense.
- Please keep the indentation level of the surrounding code; here it is 4.
- Tabs are 8 spaces.

Lionel_Debroux wrote:
> modLongData leaks some heap memory in three error paths. Found in
> Michael Stefaniuc's list of Wine potential bugs detected by Smatch.
>
> While looking at the function, I noticed
> if (lpNewData)
>    	HeapFree(GetProcessHeap(), 0, lpData);
> at lines 1015-1016.
> I think "lpData" should be "lpNewData", because this function allocates
> "lpNewData", not "lpData" which is passed to the function through a
> LPMIDIHDR structure.
> So I added a FIXME, and I am asking for comments/corrections :-)

lpNewData is allocated only if lpData has a wrong start/end byte in the
header. It is passed on in that case instead of lpData. If the functions
to which it is passed on process the data asynchronous then the current
code is correct: the caller cannot free the data (lpNewData) as the
callee might still need to process it whereas lpData is unused and can
be freed. The "event" name suggests an asynchronous processing of the
data but that's only a guess as i don't know the code.

bye
    michael
> 2007-08-31  Lionel Debroux 
>        * dlls/winealsa.drv/midi.c:
>        winealsa.drv: fix memory leak (found by Smatch)
> > From e148279419c180bd3db1b8b1a61f737bd92c73be Mon Sep 17 00:00:00 2001
>
> From: Lionel Debroux 
> Date: Fri, 31 Aug 2007 14:33:40 +0200
> Subject: winealsa.drv: fix memory leak (found by Smatch).
>
> dlls/winealsa.drv/midi.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/winealsa.drv/midi.c b/dlls/winealsa.drv/midi.c
> index f5e98ed..2cf9287 100644
> --- a/dlls/winealsa.drv/midi.c
> +++ b/dlls/winealsa.drv/midi.c
> @@ -1013,20 +1013,26 @@ static DWORD modLongData(WORD wDevID, LPMIDIHDR lpMidiHdr, DWORD dwSize)
>    snd_seq_ev_set_sysex(&event, lpMidiHdr->dwBufferLength + len_add, lpNewData ? lpNewData : lpData);
>    snd_seq_event_output_direct(midiSeq, &event);
>    if (lpNewData)
> -
> HeapFree(GetProcessHeap(), 0, lpData);
> -
> HeapFree(GetProcessHeap(), 0, lpData); /* FIXME: lpNewData instead of lpData ? */
> break;
> default:
> WARN("Technology not supported (yet) %d !\n",
>    MidiOutDev[wDevID].caps.wTechnology);
> - return MMSYSERR_NOTENABLED;
> -
>
> if (lpNewData)
> -
>    HeapFree(GetProcessHeap(), 0, lpNewData);
> -
>   return MMSYSERR_NOTENABLED;
>
> }
>
> lpMidiHdr->dwFlags &= ~MHDR_INQUEUE;
>    lpMidiHdr->dwFlags |= MHDR_DONE;
>    if (MIDI_NotifyClient(wDevID, MOM_DONE, (DWORD)lpMidiHdr, 0L) != MMSYSERR_NOERROR) {
>  WARN("can't notify client !\n");
> - return MMSYSERR_INVALPARAM;
> - if (lpNewData)
> -
>    HeapFree(GetProcessHeap(), 0, lpNewData);
> -
>   return MMSYSERR_INVALPARAM;
> }
> - if (lpNewData)
> -
>    HeapFree(GetProcessHeap(), 0, lpNewData);
> return MMSYSERR_NOERROR;
>
> }