1.3.22: memory leaks

  Hi,
while investigating VDR with valgrind and other memory tracing
tools, I found two places where memory leaks.

First is in dvbplayer.c, where the Action() code builds a frame
from the replayed file (readFrame). If the ringbuffer is already
full, this frame cannot be put immediately. If Empty() is called
in such a situation, the premade frame is lost.
Solution:


Regards.
  

s.huelswitt@gmx.de (Stefan Huelswitt) wrote:

> Second is in epg.c tComponent::FromString(). I cannot find
> anything bad with the code there, but valgrind reports a lot of
> memory leaks with the sscanf() call. So I guessed that sscanf() is
> leaking internaly when used with "%a[\n]" (at least with my glibc
> version 2.2.5). After changing to code to the suggestion below,
> the leaks disappeared:

from man sscanf:

a	Indicates  that  the  conversion  will be s, the needed
	memory space for the string will be malloc'ed  and the
	pointer to  it will  be assigned to the char pointer
	variable, which does not have to be initialized before. 

so, yes, the user is responsible for freeing memory allocated by sscanf.

clemens
  

On 27 Mar 2005 Clemens Kirchgatterer <clemens@1541.org> wrote:
> s.huelswitt@gmx.de (Stefan Huelswitt) wrote:
> 
>> Second is in epg.c tComponent::FromString(). I cannot find
>> anything bad with the code there, but valgrind reports a lot of
>> memory leaks with the sscanf() call. So I guessed that sscanf() is
>> leaking internaly when used with "%a[\n]" (at least with my glibc
>> version 2.2.5). After changing to code to the suggestion below,
>> the leaks disappeared:
> 
> from man sscanf:
> 
> a	Indicates  that  the  conversion  will be s, the needed
> 	memory space for the string will be malloc'ed  and the
> 	pointer to  it will  be assigned to the char pointer
> 	variable, which does not have to be initialized before. 
> 
> so, yes, the user is responsible for freeing memory allocated by sscanf.

Yes, of course. I can read man pages ;)

This is not what I'm talking about. The malloc memory is free'd
by VDR, but there is still some memory leaked. I think it's
internaly lost.
This can be proven with my patch. If the %a[ is obmitted, there
is no leak.

Regards.
  

I demand that Stefan Huelswitt may or may not have written...

[snip]
> The malloc memory is free'd by VDR, but there is still some memory leaked.
> I think it's internally lost. This can be proven with my patch. If the %a[
> is omitted, there is no leak.

And if you free(description) before the sscanf()?
  

On 27 Mar 2005 Darren Salt <linux@youmustbejoking.demon.co.uk> wrote:

> I demand that Stefan Huelswitt may or may not have written...
> 
> [snip]
>> The malloc memory is free'd by VDR, but there is still some memory leaked.
>> I think it's internally lost. This can be proven with my patch. If the %a[
>> is omitted, there is no leak.
> 
> And if you free(description) before the sscanf()?

I did that. description is never != NULL when the code is called.

Regards.
  

Stefan Huelswitt wrote:
> Hi,
> while investigating VDR with valgrind and other memory tracing
> tools, I found two places where memory leaks.
> 
> First is in dvbplayer.c, where the Action() code builds a frame
> from the replayed file (readFrame). If the ringbuffer is already
> full, this frame cannot be put immediately. If Empty() is called
> in such a situation, the premade frame is lost.
> Solution:
> 
> --- dvbplayer.c	2005-01-14 15:00:56.000000000 +0100
> +++ dvbplayer.c	2005-03-26 21:41:23.000000000 +0100
> @@ -296,6 +296,7 @@
>       nonBlockingFileReader->Clear();
>    if ((readIndex = backTrace->Get(playDir == pdForward)) < 0)
>       readIndex = writeIndex;
> +  delete readFrame;
>    readFrame = NULL;
>    playFrame = NULL;
>    ringBuffer->Clear();

Very well spotted!

I believe the same should be done in cDvbPlayer::~cDvbPlayer()
(just tested it, it also happens there).

Klaus
  

On 05 May 2005 Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:
> Stefan Huelswitt wrote:
>> Hi,
>> while investigating VDR with valgrind and other memory tracing
>> tools, I found two places where memory leaks.
>> 
>> First is in dvbplayer.c, where the Action() code builds a frame
>> from the replayed file (readFrame). If the ringbuffer is already
>> full, this frame cannot be put immediately. If Empty() is called
>> in such a situation, the premade frame is lost.
>> Solution:
>> 
>> --- dvbplayer.c	2005-01-14 15:00:56.000000000 +0100
>> +++ dvbplayer.c	2005-03-26 21:41:23.000000000 +0100
>> @@ -296,6 +296,7 @@
>>       nonBlockingFileReader->Clear();
>>    if ((readIndex = backTrace->Get(playDir == pdForward)) < 0)
>>       readIndex = writeIndex;
>> +  delete readFrame;
>>    readFrame = NULL;
>>    playFrame = NULL;
>>    ringBuffer->Clear();
> 
> Very well spotted!
> 
> I believe the same should be done in cDvbPlayer::~cDvbPlayer()
> (just tested it, it also happens there).

True, but not as common as the other place.

What do you think about Daniels fix to the asprintf() problem?

Regards.
  

Klaus Schmidinger wrote:
> Stefan Huelswitt wrote:
> 
>> Hi,
>> while investigating VDR with valgrind and other memory tracing
>> tools, I found two places where memory leaks.
>>
>> First is in dvbplayer.c, where the Action() code builds a frame
>> from the replayed file (readFrame). If the ringbuffer is already
>> full, this frame cannot be put immediately. If Empty() is called
>> in such a situation, the premade frame is lost.
>> Solution:
>>
>> --- dvbplayer.c    2005-01-14 15:00:56.000000000 +0100
>> +++ dvbplayer.c    2005-03-26 21:41:23.000000000 +0100
>> @@ -296,6 +296,7 @@
>>       nonBlockingFileReader->Clear();
>>    if ((readIndex = backTrace->Get(playDir == pdForward)) < 0)
>>       readIndex = writeIndex;
>> +  delete readFrame;
>>    readFrame = NULL;
>>    playFrame = NULL;
>>    ringBuffer->Clear();
> 
> 
> Very well spotted!
> 
> I believe the same should be done in cDvbPlayer::~cDvbPlayer()
> (just tested it, it also happens there).
> 

Like this?

cDvbPlayer::~cDvbPlayer()
{
  Detach();
  Save();
  delete index;
  delete fileName;
  delete backTrace;
  delete readFrame;
  delete ringBuffer;
}

Best Regards,
  

Stefan Huelswitt wrote:
> ...
> What do you think about Daniels fix to the asprintf() problem?

I've adopted that one, too.

Klaus
  

C.Y.M wrote:
> Klaus Schmidinger wrote:
> ...
>>I believe the same should be done in cDvbPlayer::~cDvbPlayer()
>>(just tested it, it also happens there).
>>
> 
> 
> Like this?
> 
> cDvbPlayer::~cDvbPlayer()
> {
>   Detach();
>   Save();
>   delete index;
>   delete fileName;
>   delete backTrace;
>   delete readFrame;
>   delete ringBuffer;
> }

Yes.

Klaus