1.3.22: memory leaks

  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:

       }

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.