*** glibc detected *** double free or corruption 1.4.2-1 Patch
Commit Message
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
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
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
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.
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
@@ -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;
@@ -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;
}