[vdr] 1.3.22: memory leaks

Daniel THOMPSON daniel.thompson at st.com
Tue Mar 29 10:09:33 CEST 2005


Stefan Huelswitt wrote:
> On 27 Mar 2005 Clemens Kirchgatterer <clemens at 1541.org> wrote:
> 
>>s.huelswitt at 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.
> 


-- 
Daniel Thompson (STMicroelectronics) <daniel.thompson at st.com>
1000 Aztec West, Almondsbury, Bristol, BS32 4SQ. 01454 462659

If a car is a horseless carriage then is a motorcycle a horseless horse?



More information about the vdr mailing list