Hello Klaus,
I've problems when implementing the latest 1.4.2.-1 Patch. Here's the problem: I used VDR-Admin to change a timer setting. Every time I try to save the changed timer, VDR crashes, please see attached strace.
I've checked against all 1.4.1-* versions, with bigpatch, without. It must be something with the latest 1.4.2-1 Patch.
My system is a Pentium-M:
[root@htpc /usr/local/src]# cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 13
model name : Intel(R) Pentium(R) M processor 1.70GHz
Please let me know, if you need any additional information.
Regards,
Martin
martin wrote:
I’ve problems when implementing the latest 1.4.2.-1 Patch. Here’s the problem: I used VDR-Admin to change a timer setting. Every time I try to save the changed timer, VDR crashes, please see attached strace.
I’ve checked against all 1.4.1-* versions, with bigpatch, without. It must be something with the latest 1.4.2-1 Patch.
The only thing I could imagine causing this is the change to the cTimer::operator=() function.
Please try adding some debug output before and after the free(aux) calls, as in:
cTimer::~cTimer() { fprint(stderr, "A %p\n", aux); free(aux); fprint(stderr, "B\n"); }
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)); fprint(stderr, "C %p\n", aux); free(aux); fprint(stderr, "D\n"); aux = Timer.aux ? strdup(Timer.aux) : NULL; event = NULL; return *this; }
Let me know what the last output is.
Klaus
Hi,
On Mon, 4 Sep 2006 17:09:12 +0200 "martin" martin@air-maxx.net wrote:
I've problems when implementing the latest 1.4.2.-1 Patch. Here's the problem: I used VDR-Admin to change a timer setting. Every time I try to save the changed timer, VDR crashes, please see attached strace.
There had been a "free(aux);" added in that patch in timers.c in the assignment operator ("=") function. Probably it didn't took an earlier (conditional?) free() into account and such rans into a glibc-assertion.
Changing the "free(aux);" to "if(aux) free(aux);" would probably care for that (resembling the earlier behaviour).
-hwh
Hi,
implemented the following, but it did not solve the issue :-(
timers.c
cTimer::~cTimer() { if(aux) free(aux); }
..
lifetime = Timer.lifetime; strncpy(file, Timer.file, sizeof(file)); if (aux) free(aux); aux = Timer.aux ? strdup(Timer.aux) : NULL;
Klaus's debugging statements did not work, as gcc refuses work ..
g++ -g -O2 -Wall -Woverloaded-virtual -c -DREMOTE_KBD g++ -DLIRC_DEVICE="/dev/lircd" -DRCU_DEVICE="/dev/ttyS1" g++ -D_GNU_SOURCE -DVIDEODIR="/video" -DPLUGINDIR="./PLUGINS/lib" g++ timers.c timers.c: In destructor 'virtual cTimer::~cTimer()': timers.c:88: error: 'fprint' was not declared in this scope timers.c: In member function 'cTimer& cTimer::operator=(const cTimer&)': timers.c:111: error: 'fprint' was not declared in this scope make: *** [timers.o] Error 1
Regards, and thanks for your effort
-----Ursprüngliche Nachricht----- Von: vdr-bounces@linuxtv.org [mailto:vdr-bounces@linuxtv.org] Im Auftrag von Hans-Werner Hilse Gesendet: Montag, 4. September 2006 17:42 An: vdr@linuxtv.org Betreff: Re: [vdr] *** glibc detected *** double free or corruption 1.4.2-1 Patch
Hi,
On Mon, 4 Sep 2006 17:09:12 +0200 "martin" martin@air-maxx.net wrote:
I've problems when implementing the latest 1.4.2.-1 Patch. Here's the problem: I used VDR-Admin to change a timer setting. Every time I try to save the changed timer, VDR crashes, please see attached strace.
There had been a "free(aux);" added in that patch in timers.c in the assignment operator ("=") function. Probably it didn't took an earlier (conditional?) free() into account and such rans into a glibc-assertion.
Changing the "free(aux);" to "if(aux) free(aux);" would probably care for that (resembling the earlier behaviour).
-hwh
_______________________________________________ vdr mailing list vdr@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
martin wrote:
Hi,
implemented the following, but it did not solve the issue :-(
timers.c
cTimer::~cTimer() { if(aux) free(aux); }
..
lifetime = Timer.lifetime; strncpy(file, Timer.file, sizeof(file)); if (aux) free(aux); aux = Timer.aux ? strdup(Timer.aux) : NULL;
Klaus's debugging statements did not work, as gcc refuses work ..
Sorry, that was a typo - I was in a hurry ;-)
It needs to be fprintf().
Anyway, Alexander Rieger just sent me a PM in which he noted that the operator=() function needs to check whether this is an assignment to "self".
Please try this:
Timer& cTimer::operator= (const cTimer &Timer) { if (this != &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; } return *this; }
Klaus
Hi,
implemented the following, but it did not solve the issue :-(
timers.c
cTimer::~cTimer() { if(aux) free(aux); }
..
lifetime = Timer.lifetime; strncpy(file, Timer.file, sizeof(file)); if (aux) free(aux); aux = Timer.aux ? strdup(Timer.aux) : NULL;
Klaus's debugging statements did not work, as gcc refuses work ..
g++ -g -O2 -Wall -Woverloaded-virtual -c -DREMOTE_KBD g++ -DLIRC_DEVICE="/dev/lircd" -DRCU_DEVICE="/dev/ttyS1" g++ -D_GNU_SOURCE -DVIDEODIR="/video" -DPLUGINDIR="./PLUGINS/lib" g++ timers.c timers.c: In destructor 'virtual cTimer::~cTimer()': timers.c:88: error: 'fprint' was not declared in this scope timers.c: In member function 'cTimer& cTimer::operator=(const cTimer&)': timers.c:111: error: 'fprint' was not declared in this scope make: *** [timers.o] Error 1
Regards, and thanks for your effort
-----Ursprüngliche Nachricht----- Von: vdr-bounces@linuxtv.org [mailto:vdr-bounces@linuxtv.org] Im Auftrag von Hans-Werner Hilse Gesendet: Montag, 4. September 2006 17:42 An: vdr@linuxtv.org Betreff: Re: [vdr] *** glibc detected *** double free or corruption 1.4.2-1 Patch
Hi,
On Mon, 4 Sep 2006 17:09:12 +0200 "martin" martin@air-maxx.net wrote:
I've problems when implementing the latest 1.4.2.-1 Patch. Here's the problem: I used VDR-Admin to change a timer setting. Every time I try to save the changed timer, VDR crashes, please see attached strace.
There had been a "free(aux);" added in that patch in timers.c in the assignment operator ("=") function. Probably it didn't took an earlier (conditional?) free() into account and such rans into a glibc-assertion.
Changing the "free(aux);" to "if(aux) free(aux);" would probably care for that (resembling the earlier behaviour).
-hwh
_______________________________________________ vdr mailing list vdr@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Hans-Werner Hilse wrote:
There had been a "free(aux);" added in that patch in timers.c in the assignment operator ("=") function. Probably it didn't took an earlier (conditional?) free() into account and such rans into a glibc-assertion.
This seems to be at least one source of bugs. I've detected this with Valgrind:
==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)
However, there are also some of these:
==4652== Invalid write of size 4 ==4652== at 0x80FCA0E: cSocket::Open() (svdrp.c:69) ==4652== by 0x80FCB91: cSocket::Accept() (svdrp.c:110) ==4652== by 0x1BD13C34: ??? ==4652== by 0x1BD10F83: ??? ==4652== by 0x8103114: cThread::StartThread(cThread*) (thread.c:244) ==4652== by 0x1B935B62: start_thread (in /lib/tls/libpthread-0.60.so) ==4652== by 0x1BB28189: clone (in /lib/tls/libc-2.3.2.so) ==4652== Address 0x1F70F4A4 is 4 bytes inside a block of size 12 free'd ==4652== at 0x1B904CA8: operator delete(void*) (vg_replace_malloc.c:155) ==4652== by 0x1BD13EBA: ??? ==4652== by 0x810882D: cListBase::Clear() (tools.c:1445) ==4652== by 0x810D0B0: main (vdr.c:1226)
Cheers,
Udo
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;
Cheers,
Udo
SUCCESS! This was the problem -> @Klaus: can you please incorporate the changes in the next patch?
Thanks, Martin
-----Ursprüngliche Nachricht----- Von: vdr-bounces@linuxtv.org [mailto:vdr-bounces@linuxtv.org] Im Auftrag von Udo Richter Gesendet: Montag, 4. September 2006 18:47 An: VDR Mailing List Betreff: Re: [vdr] *** glibc detected *** double free or corruption 1.4.2-1 Patch
Udo Richter wrote:
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;
Cheers,
Udo
_______________________________________________ vdr mailing list vdr@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
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.
Klaus
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
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
martin wrote:
just to let you know: the patch you attached here, did not solve the problem! My VDR crashed again.
Just a few minutes too late...
The patch had another bug that re-introduced the original problem once again. The new copy constructor did not initialize the aux pointer, and the assign operator consequently free'd its uninitialized value - and with some luck, thats a valid pointer.
The attached patch does the missing initialization. Hopefully, thats the last bug. ;)
Cheers,
Udo
diff -Naur vdr-1.4.2-1-orig/timers.c vdr-1.4.2-1/timers.c --- vdr-1.4.2-1-orig/timers.c 2006-09-04 22:03:05.553657432 +0200 +++ vdr-1.4.2-1/timers.c 2006-09-04 22:07:03.904017384 +0200 @@ -83,6 +83,16 @@ event = NULL; // let SetEvent() be called to get a log message }
+cTimer::cTimer(const cTimer &Timer) +{ + // initialize at least the pointers + channel = NULL; + aux = NULL; + event = NULL; + // assign operator does the rest + *this = Timer; +} + cTimer::~cTimer() { free(aux); @@ -90,24 +100,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; }
diff -Naur vdr-1.4.2-1-orig/timers.h vdr-1.4.2-1/timers.h --- vdr-1.4.2-1-orig/timers.h 2006-04-08 14:41:44.000000000 +0200 +++ vdr-1.4.2-1/timers.h 2006-09-04 22:05:16.041820216 +0200 @@ -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;
Udo Richter wrote:
martin wrote:
just to let you know: the patch you attached here, did not solve the problem! My VDR crashed again.
Just a few minutes too late...
The patch had another bug that re-introduced the original problem once again. The new copy constructor did not initialize the aux pointer, and the assign operator consequently free'd its uninitialized value - and with some luck, thats a valid pointer.
The attached patch does the missing initialization. Hopefully, thats the last bug. ;)
That'll teach me not to write code when I'm on my way out to the "Biergarten"... ;-)
Thanks.
Klaus
Okay, vdr-1.4.2.1-timerassign-2.diff did work :-) Thanks Udo and Klaus for coding!
So, I'm happy now and can relax on my balcony with a Weißbier :-)
Greetz from Munich, Martin
-----Ursprüngliche Nachricht----- Von: vdr-bounces@linuxtv.org [mailto:vdr-bounces@linuxtv.org] Im Auftrag von Klaus Schmidinger Gesendet: Montag, 4. September 2006 22:25 An: vdr@linuxtv.org Betreff: Re: AW: [vdr] *** glibc detected *** double free or corruption 1.4.2-1 Patch
Udo Richter wrote:
martin wrote:
just to let you know: the patch you attached here, did not solve the problem! My VDR crashed again.
Just a few minutes too late...
The patch had another bug that re-introduced the original problem once again. The new copy constructor did not initialize the aux pointer, and the assign operator consequently free'd its uninitialized value - and with some luck, thats a valid pointer.
The attached patch does the missing initialization. Hopefully, thats the last bug. ;)
That'll teach me not to write code when I'm on my way out to the "Biergarten"... ;-)
Thanks.
Klaus
_______________________________________________ vdr mailing list vdr@linuxtv.org http://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Hans-Werner Hilse hilse@web.de wrote:
Changing the "free(aux);" to "if(aux) free(aux);" would probably care for that (resembling the earlier behaviour).
i know, the problem has been already fixed, but just for the record.
code like: if (bla) free(bla); will actually _never_ fix any bug. either bla is a valid pointer and can be free'ed or bla is NULL and the free does not hurt anyway, because one is explicitely allowed to free NULL pointers by the standard.
clemens
Hi,
On Fri, 8 Sep 2006 16:02:57 +0200 Clemens Kirchgatterer clemens@1541.org wrote:
Changing the "free(aux);" to "if(aux) free(aux);" would probably care for that (resembling the earlier behaviour).
code like: if (bla) free(bla); will actually _never_ fix any bug. either bla is a valid pointer and can be free'ed or bla is NULL and the free does not hurt anyway, because one is explicitely allowed to free NULL pointers by the standard.
Yep, you're right. My mistake was not taking glibc private data into account and just using that "if" to check if it has been freed before. Of course, this only works partially. It's only duct tape for that bug... The free() *does* hurt, however. The standard tells us not to free a pointer twice (in fact, the man page suggests that "undefined behaviour occurs"). That's why there is this "double free assertion", I think. But what my solution suggested was just a circumvention of that assertion, not the bug itself. An ugly hack, that is agreed, not fixing the bug, but making the software, errrr, work :-)
-hwh
Hans-Werner Hilse wrote:
bug... The free() *does* hurt, however. The standard tells us not to free a pointer twice (in fact, the man page suggests that "undefined behaviour occurs"). That's why there is this "double free assertion", I think. But what my solution suggested was just a circumvention of that assertion, not the bug itself.
I don't agree. The double free assertion wont be issued if you free NULL twice. Its good style to NULL unused pointers so they can always be safely free'd, but if you do have double free assertions, then the bug is not the missing if (aux), its the fact that aux was not properly NULLed after the first free.
In this case the cTimer object was improperly doubled, and you would have to NULL the pointers in both objects manually, though both copies don't know anything about each other.
Cheers,
Udo
Hi,
On Fri, 08 Sep 2006 16:58:10 +0200 Udo Richter udo_richter@gmx.de wrote:
I don't agree. The double free assertion wont be issued if you free NULL twice.
Of course. Thanks for pointing out my failure & clarification. I should have thought twice... (and I just read the code for malloc/free, should have done that earlier, it has more documentation than I ever expected).
-hwh
-
Clemens Kirchgatterer -
Hans-Werner Hilse -
Jens Auer -
Klaus Schmidinger -
martin -
Udo Richter