Stefan Huelswitt wrote:
> On 27 Mar 2005 Clemens Kirchgatterer <clemens@xxxxxxxx> wrote:
>
>>s.huelswitt@xxxxxx (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@xxxxxx>
1000 Aztec West, Almondsbury, Bristol, BS32 4SQ. 01454 462659
If a car is a horseless carriage then is a motorcycle a horseless horse?
