*** glibc detected *** double free or corruption 1.4.2-1 Patch

Message ID 44FC5EC6.3040005@cadsoft.de
State New
Headers

Commit Message

Klaus Schmidinger Sept. 4, 2006, 5:13 p.m. UTC
  Klaus Schmidinger wrote:
> Udo Richter wrote:
>> Udo Richter wrote:
>>> ==4652== Invalid free() / delete / delete[]
>>> ==4652==    at 0x1B904B04: free (vg_replace_malloc.c:152)
>>> ==4652==    by 0x8103F5F: cTimer::operator=(cTimer const&) 
>>> (timers.c:108)
>>> ==4652==    by 0x80FE349: cSVDRP::CmdMODT(char const*) (svdrp.c:1136)
>>> ==4652==    by 0x81015C1: cSVDRP::Process() (svdrp.c:1563)
>>> ==4652==    by 0x80B3458: cInterface::GetKey(bool) (interface.c:37)
>>> ==4652==    by 0x810D919: main (vdr.c:866)
>>> ==4652==  Address 0x1BEEAC90 is 0 bytes inside a block of size 63 free'd
>>> ==4652==    at 0x1B904B04: free (vg_replace_malloc.c:152)
>>> ==4652==    by 0x8104D6F: cTimer::Parse(char const*) (timers.c:244)
>>> ==4652==    by 0x80FE493: cSVDRP::CmdMODT(char const*) (svdrp.c:1132)
>>> ==4652==    by 0x81015C1: cSVDRP::Process() (svdrp.c:1563)
>>> ==4652==    by 0x80B3458: cInterface::GetKey(bool) (interface.c:37)
>>> ==4652==    by 0x810D919: main (vdr.c:866)
>>
>>
>> I think I've found it:
>>
>> This is line 1127 of svdrp.c:
>>
>>     cTimer t = *timer;
>>
>> Although this looks like it calls cTimer::operator=, it actually calls 
>> the default copy constructor of cTimer, because in this case = is not 
>> an assignment, but an initialization. Because of that, the aux field 
>> is used by both objects, thus the double free. Try this line to see if 
>> it causes this:
>>
>>     cTimer t;
>>     t = *timer;
> 
> It's probably best to implement an actual copy-constructor.
> 
> Please try the attached patch, which contains both changes.

Opps, sorry, there was a typo.

Attached is the correct version.

(Never code when in a hurry... ;-)

Klaus
  

Comments

Udo Richter Sept. 4, 2006, 5:41 p.m. UTC | #1
Klaus Schmidinger wrote:
>> It's probably best to implement an actual copy-constructor.
>>
>> Please try the attached patch, which contains both changes.
> 
> Attached is the correct version.

Thats the better fix of course, and may fix this in case some plugin did 
the same mistake. (couldn't find one in one of my plugins though)

Of course this touches the header and requires to bump APIVERSION. And 
it probably requires compiling all plugins.

Cheers,

Udo
  
Klaus Schmidinger Sept. 4, 2006, 5:45 p.m. UTC | #2
Udo Richter wrote:
> Klaus Schmidinger wrote:
>>> It's probably best to implement an actual copy-constructor.
>>>
>>> Please try the attached patch, which contains both changes.
>>
>> Attached is the correct version.
> 
> Thats the better fix of course, and may fix this in case some plugin did 
> the same mistake. (couldn't find one in one of my plugins though)
> 
> Of course this touches the header and requires to bump APIVERSION. And 
> it probably requires compiling all plugins.

Well, I wonder if we'll ever see a bugfix release that
doesn't need to bump APIVERSION ;-)
If only I had never agreed to introduce this beast...

Klaus
  
Jens Auer Sept. 4, 2006, 5:47 p.m. UTC | #3
Klaus Schmidinger wrote:
> Well, I wonder if we'll ever see a bugfix release that
> doesn't need to bump APIVERSION ;-)
> If only I had never agreed to introduce this beast...
Maybe it would be a good thing to decouple the public API interface and 
the implementation with the Pimpl-idiom. This would guarantee stability 
of the  public API, but enable the development of the implementation.
  
martin Sept. 4, 2006, 7:53 p.m. UTC | #4
Hi Klaus,

just to let you know: the patch you attached here, did not solve the
problem! My VDR crashed again.

Regards,
Martin

-----Urspr√ľngliche Nachricht-----
Von: vdr-bounces@linuxtv.org [mailto:vdr-bounces@linuxtv.org] Im Auftrag von
Klaus Schmidinger
Gesendet: Montag, 4. September 2006 19:14
An: vdr@linuxtv.org
Betreff: Re: [vdr] *** glibc detected *** double free or corruption 1.4.2-1
Patch

Klaus Schmidinger wrote:
> Udo Richter wrote:
>> Udo Richter wrote:
>>> ==4652== Invalid free() / delete / delete[]
>>> ==4652==    at 0x1B904B04: free (vg_replace_malloc.c:152)
>>> ==4652==    by 0x8103F5F: cTimer::operator=(cTimer const&) 
>>> (timers.c:108)
>>> ==4652==    by 0x80FE349: cSVDRP::CmdMODT(char const*) (svdrp.c:1136)
>>> ==4652==    by 0x81015C1: cSVDRP::Process() (svdrp.c:1563)
>>> ==4652==    by 0x80B3458: cInterface::GetKey(bool) (interface.c:37)
>>> ==4652==    by 0x810D919: main (vdr.c:866)
>>> ==4652==  Address 0x1BEEAC90 is 0 bytes inside a block of size 63 free'd
>>> ==4652==    at 0x1B904B04: free (vg_replace_malloc.c:152)
>>> ==4652==    by 0x8104D6F: cTimer::Parse(char const*) (timers.c:244)
>>> ==4652==    by 0x80FE493: cSVDRP::CmdMODT(char const*) (svdrp.c:1132)
>>> ==4652==    by 0x81015C1: cSVDRP::Process() (svdrp.c:1563)
>>> ==4652==    by 0x80B3458: cInterface::GetKey(bool) (interface.c:37)
>>> ==4652==    by 0x810D919: main (vdr.c:866)
>>
>>
>> I think I've found it:
>>
>> This is line 1127 of svdrp.c:
>>
>>     cTimer t = *timer;
>>
>> Although this looks like it calls cTimer::operator=, it actually 
>> calls the default copy constructor of cTimer, because in this case = 
>> is not an assignment, but an initialization. Because of that, the aux 
>> field is used by both objects, thus the double free. Try this line to 
>> see if it causes this:
>>
>>     cTimer t;
>>     t = *timer;
> 
> It's probably best to implement an actual copy-constructor.
> 
> Please try the attached patch, which contains both changes.

Opps, sorry, there was a typo.

Attached is the correct version.

(Never code when in a hurry... ;-)

Klaus
  

Patch

--- timers.h	2006/04/08 12:41:44	1.28
+++ timers.h	2006/09/04 17:07:39
@@ -44,6 +44,7 @@ 
 public:
   cTimer(bool Instant = false, bool Pause = false, cChannel *Channel = NULL);
   cTimer(const cEvent *Event);
+  cTimer(const cTimer &Timer);
   virtual ~cTimer();
   cTimer& operator= (const cTimer &Timer);
   virtual int Compare(const cListObject &ListObject) const;
--- timers.c	2006/09/02 10:20:36	1.63
+++ timers.c	2006/09/04 17:08:32
@@ -83,6 +83,11 @@ 
   event = NULL; // let SetEvent() be called to get a log message
 }
 
+cTimer::cTimer(const cTimer &Timer)
+{
+  *this = Timer;
+}
+
 cTimer::~cTimer()
 {
   free(aux);
@@ -90,24 +95,26 @@ 
 
 cTimer& cTimer::operator= (const cTimer &Timer)
 {
-  startTime    = Timer.startTime;
-  stopTime     = Timer.stopTime;
-  lastSetEvent = 0;
-  recording    = Timer.recording;
-  pending      = Timer.pending;
-  inVpsMargin  = Timer.inVpsMargin;
-  flags        = Timer.flags;
-  channel      = Timer.channel;
-  day          = Timer.day;
-  weekdays     = Timer.weekdays;
-  start        = Timer.start;
-  stop         = Timer.stop;
-  priority     = Timer.priority;
-  lifetime     = Timer.lifetime;
-  strncpy(file, Timer.file, sizeof(file));
-  free(aux);
-  aux = Timer.aux ? strdup(Timer.aux) : NULL;
-  event = NULL;
+  if (&Timer != this) {
+     startTime    = Timer.startTime;
+     stopTime     = Timer.stopTime;
+     lastSetEvent = 0;
+     recording    = Timer.recording;
+     pending      = Timer.pending;
+     inVpsMargin  = Timer.inVpsMargin;
+     flags        = Timer.flags;
+     channel      = Timer.channel;
+     day          = Timer.day;
+     weekdays     = Timer.weekdays;
+     start        = Timer.start;
+     stop         = Timer.stop;
+     priority     = Timer.priority;
+     lifetime     = Timer.lifetime;
+     strncpy(file, Timer.file, sizeof(file));
+     free(aux);
+     aux = Timer.aux ? strdup(Timer.aux) : NULL;
+     event = NULL;
+     }
   return *this;
 }