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();
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:
--- epg.c 2005-02-19 12:35:00.000000000 +0100 +++ epg.c 2005-03-27 10:53:06.000000000 +0200 @@ -28,13 +28,12 @@ bool tComponent::FromString(const char *s) { unsigned int Stream, Type; - int n = sscanf(s, "%X %02X %3c %a[^\n]", &Stream, &Type, language, &description); - if (n != 4) + char buf[512]; + int n = sscanf(s, "%X %02X %3c %511[^\n]", &Stream, &Type, language, buf); + if (n==4 && !isempty(buf)) + description = strdup(buf); + else description = NULL; - else if (isempty(description)) { - free(description); - description = NULL; - } stream = Stream; type = Type; return n >= 3;
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:
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.
There are routes through tComponent::FromString() that explicitly set description to NULL without checking its value first (when n != 4). This appears to me to be the leak.
Running the following code it is obvious that glibc malloc'ed desc even when desc is not converted.
n = sscanf("", "%a[^\n]", &desc); printf("n = %d; desc = %p\n", n, desc);
Thus the simpler fix for this leak, and one that is not prone to buffer overflow, is:
--- vdr-1.3.22/epg.c.orig 2005-03-29 09:03:23.484024000 +0100 +++ vdr-1.3.22/epg.c 2005-03-29 09:04:16.359024000 +0100 @@ -29,9 +29,7 @@ { unsigned int Stream, Type; int n = sscanf(s, "%X %02X %3c %a[^\n]", &Stream, &Type, language, &description); - if (n != 4) - description = NULL; - else if (isempty(description)) { + if (n != 4 || isempty(description)) { free(description); description = NULL; }
While that are more robust ways to handle the case then desc is NULL (which for my glibc it never is) the above code is safe since both isempty() and glibc's free() can safely handle NULL pointers.
Regards.
On 29 Mar 2005 Daniel THOMPSON daniel.thompson@st.com wrote:
Hi,
There are routes through tComponent::FromString() that explicitly set description to NULL without checking its value first (when n != 4). This appears to me to be the leak.
Running the following code it is obvious that glibc malloc'ed desc even when desc is not converted.
Wow, this is cool. How did you get the idea to search in that direction?
Anyways, I think the man page isn't very clear on this fact...
Thus the simpler fix for this leak, and one that is not prone to buffer overflow, is:
--- vdr-1.3.22/epg.c.orig 2005-03-29 09:03:23.484024000 +0100 +++ vdr-1.3.22/epg.c 2005-03-29 09:04:16.359024000 +0100 @@ -29,9 +29,7 @@ { unsigned int Stream, Type; int n = sscanf(s, "%X %02X %3c %a[^\n]", &Stream, &Type, language, &description);
- if (n != 4)
description = NULL;- else if (isempty(description)) {
- if (n != 4 || isempty(description)) { free(description); description = NULL; }
While that are more robust ways to handle the case then desc is NULL (which for my glibc it never is) the above code is safe since both isempty() and glibc's free() can safely handle NULL pointers.
What about this?
if (description!=NULL && (n != 4 || isempty(description)))
Regards.
Stefan Huelswitt wrote:
On 29 Mar 2005 Daniel THOMPSON daniel.thompson@st.com wrote:
Hi,
There are routes through tComponent::FromString() that explicitly set description to NULL without checking its value first (when n != 4). This appears to me to be the leak.
Running the following code it is obvious that glibc malloc'ed desc even when desc is not converted.
Wow, this is cool. How did you get the idea to search in that direction?
I got the idea from the reading your patch and the premise that it was unlikely that there was a bug in glibc.
Put simply I *never* blame core software like glibc or the compiler for bugs unless it is proved to me. These bits of software are so widely used that while blaming them is not *always* wrong it usually saves a lot of time to audit your own code first. Also I've met Ulrich Drepper and wouldn't want to let him catch me blaming glibc for something it didn't do.
Anyways, I think the man page isn't very clear on this fact...
Agreed and I guess this is why the 'know your interfaces' maxim is so important.
While that are more robust ways to handle the case then desc is NULL (which for my glibc it never is) the above code is safe since both isempty() and glibc's free() can safely handle NULL pointers.
What about this?
if (description!=NULL && (n != 4 || isempty(description)))
Looks fine to me. For belt and braces we should probably also assert that description is NULL when we enter the call (or test it and free it).
On 30 Mar 2005 Daniel THOMPSON daniel.thompson@st.com wrote:
Stefan Huelswitt wrote:
Wow, this is cool. How did you get the idea to search in that direction?
I got the idea from the reading your patch and the premise that it was unlikely that there was a bug in glibc.
Put simply I *never* blame core software like glibc or the compiler for bugs unless it is proved to me. These bits of software are so widely used that while blaming them is not *always* wrong it usually saves a lot of time to audit your own code first. Also I've met Ulrich Drepper and wouldn't want to let him catch me blaming glibc for something it didn't do.
Right, I wasn't pretty sure but as I'm using an older glibc version it might have been fixed in current version...
What about this?
if (description!=NULL && (n != 4 || isempty(description)))
Looks fine to me. For belt and braces we should probably also assert that description is NULL when we enter the call (or test it and free it).
From my investigations, I can say that it seems to be NULL
always (I checked this, because this was my first idea for the leak).
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,
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
-
C.Y.M -
Clemens Kirchgatterer -
Daniel THOMPSON -
Darren Salt -
Klaus Schmidinger -
s.huelswitt＠gmx.de