Message ID | d25t55$dgq$1@video.local.muempf.de |
---|---|
State | New |
Headers |
Received: from moutng.kundenserver.de ([212.227.126.186]) by www.linuxtv.org with esmtp (Exim 4.34) id 1DFTkN-0004td-QG for vdr@linuxtv.org; Sun, 27 Mar 2005 11:07:51 +0200 Received: from pD903127F.dip0.t-ipconnect.de[217.3.18.127] (helo=video.local.muempf.de) by mrelayeu.kundenserver.de with ESMTP (Nemesis), id 0MKwh2-1DFTka2Ow6-0000mb; Sun, 27 Mar 2005 11:08:04 +0200 Received: from video.local.muempf.de (localhost [127.0.0.1]) by video.local.muempf.de (8.12.6/8.12.6/SuSE Linux 0.6) with ESMTP id j2R97nHS013861 for <vdr@linuxtv.org>; Sun, 27 Mar 2005 11:07:50 +0200 Received: (from news@localhost) by video.local.muempf.de (8.12.6/8.12.6/Submit) id j2R97nUW013860 for vdr@linuxtv.org; Sun, 27 Mar 2005 11:07:49 +0200 To: vdr@linuxtv.org Path: not-for-mail From: s.huelswitt@gmx.de (Stefan Huelswitt) Newsgroups: local.linux.vdr Date: Sun, 27 Mar 2005 09:07:49 +0000 (UTC) Organization: Home, sweet home Lines: 56 Sender: nathan@gmx.de Message-ID: <d25t55$dgq$1@video.local.muempf.de> NNTP-Posting-Host: master.local.muempf.de Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: video.local.muempf.de 1111914469 13850 192.168.1.1 (27 Mar 2005 09:07:49 GMT) X-Complaints-To: s.huelswitt@gmx.de NNTP-Posting-Date: Sun, 27 Mar 2005 09:07:49 +0000 (UTC) X-Newsreader: knews 1.0b.1 X-Provags-ID: kundenserver.de abuse@kundenserver.de login:ad11259134df5e6b2749af0524d71867 Subject: [vdr] 1.3.22: memory leaks X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Klaus Schmidinger's VDR <vdr@linuxtv.org> List-Id: Klaus Schmidinger's VDR <vdr.linuxtv.org> List-Unsubscribe: <http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr>, <mailto:vdr-request@linuxtv.org?subject=unsubscribe> List-Archive: <http://www.linuxtv.org/pipermail/vdr> List-Post: <mailto:vdr@linuxtv.org> List-Help: <mailto:vdr-request@linuxtv.org?subject=help> List-Subscribe: <http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr>, <mailto:vdr-request@linuxtv.org?subject=subscribe> X-List-Received-Date: Sun, 27 Mar 2005 09:07:52 -0000 Status: O X-Status: X-Keywords: X-UID: 1127 |
Commit Message
Stefan Huelswitt
March 27, 2005, 9:07 a.m. UTC
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: Regards.
Comments
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: > 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,
Stefan Huelswitt wrote: > ... > What do you think about Daniels fix to the asprintf() problem? I've adopted that one, too. Klaus
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
--- 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;