From patchwork Tue Mar 29 08:09:33 2005 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel THOMPSON X-Patchwork-Id: 11833 Received: from fra-del-01.spheriq.net ([195.46.51.97]) by www.linuxtv.org with esmtp (Exim 4.34) id 1DGBnp-0003f1-CI for vdr@linuxtv.org; Tue, 29 Mar 2005 10:10:21 +0200 Received: from fra-out-03.spheriq.net (fra-out-03.spheriq.net [195.46.51.131]) by fra-del-01.spheriq.net with ESMTP id j2T8A48r013958 for ; Tue, 29 Mar 2005 08:10:04 GMT Received: from fra-cus-01.spheriq.net (fra-cus-01.spheriq.net [195.46.51.37]) by fra-out-03.spheriq.net with ESMTP id j2T8A4Sb032386 for ; Tue, 29 Mar 2005 08:10:04 GMT Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by fra-cus-01.spheriq.net with ESMTP id j2T8A1rb021621 (version=TLSv1/SSLv3 cipher=EDH-RSA-DES-CBC3-SHA bits=168 verify=OK) for ; Tue, 29 Mar 2005 08:10:04 GMT Received: from zeta.dmz-eu.st.com (ns2.st.com [164.129.230.9]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 87FF2DA69 for ; Tue, 29 Mar 2005 08:09:38 +0000 (GMT) Received: by zeta.dmz-eu.st.com (STMicroelectronics, from userid 60012) id A9C0D47300; Tue, 29 Mar 2005 08:10:48 +0000 (GMT) Received: from zeta.dmz-eu.st.com (localhost [127.0.0.1]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 41081759AE for ; Tue, 29 Mar 2005 08:10:48 +0000 (UTC) Received: from mail1.bri.st.com (mail1.bri.st.com [164.129.8.218]) by zeta.dmz-eu.st.com (STMicroelectronics) with ESMTP id 7F4DD47305 for ; Tue, 29 Mar 2005 08:10:47 +0000 (GMT) Received: from [164.129.15.35] (butch.bri.st.com [164.129.15.35]) by mail1.bri.st.com (MOS 3.4.4-GR) with ESMTP id BBE00551 (AUTH "daniel thompson"); Tue, 29 Mar 2005 09:09:33 +0100 (BST) Message-ID: <42490D3D.6090705@st.com> Date: Tue, 29 Mar 2005 09:09:33 +0100 From: Daniel THOMPSON Organization: STMicroelectronics (R&D) Ltd User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041020 X-Accept-Language: en-us, en MIME-Version: 1.0 To: "Klaus Schmidinger's VDR" Subject: Re: [vdr] 1.3.22: memory leaks References: <20050327113933.285e28b0.clemens@1541.org> In-Reply-To: X-O-General-Status: No X-O-Spam1-Status: Not Scanned X-O-Spam2-Status: Not Scanned X-O-URL-Status: Not Scanned X-O-Virus1-Status: No X-O-Virus2-Status: Not Scanned X-O-Virus3-Status: No X-O-Virus4-Status: No X-O-Virus5-Status: Not Scanned X-O-Image-Status: Not Scanned X-O-Attach-Status: No X-SpheriQ-Ver: 2.1.1 X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Klaus Schmidinger's VDR List-Id: Klaus Schmidinger's VDR List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Mar 2005 08:10:21 -0000 Status: O X-Status: X-Keywords: X-UID: 1190 Stefan Huelswitt wrote: > On 27 Mar 2005 Clemens Kirchgatterer 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. > --- 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;