[libvirt] [PATCH] conf: Remove console stream callback only when freeing console helper
Commit ba226d334acbc49f6751b430e0c4e00f69eef6bf tried to fix crash of the daemon when a domain with a open console was destroyed. The fix was wrong as it tried to remove the callback also when the stream was aborted, where at that point the fd stream driver was already freed and removed. This patch clears the callbacks with a helper right before the hash is freed, so that it doesn't interfere with other codepaths where the stream object is freed. --- src/conf/virconsole.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/conf/virconsole.c b/src/conf/virconsole.c index 912aff6..b3f6eb6 100644 --- a/src/conf/virconsole.c +++ b/src/conf/virconsole.c @@ -219,9 +219,6 @@ static void virConsoleHashEntryFree(void *data, const char *pty = name; virStreamPtr st = data; - /* remove callback from stream */ - virFDStreamSetInternalCloseCb(st, NULL, NULL, NULL); - /* free stream reference */ virStreamFree(st); @@ -290,6 +287,18 @@ error: } /** + * Helper to clear stream callbacks when freeing the hash + */ +static void virConsoleFreeClearCallbacks(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED) +{ + virStreamPtr st = payload; + + virFDStreamSetInternalCloseCb(st, NULL, NULL, NULL); +} + +/** * Free structures for handling open console streams. * * @cons Pointer to the private structure. @@ -300,6 +309,7 @@ void virConsoleFree(virConsolesPtr cons) return; virMutexLock(&cons->lock); + virHashForEach(cons->hash, virConsoleFreeClearCallbacks, NULL); virHashFree(cons->hash); virMutexUnlock(&cons->lock); virMutexDestroy(&cons->lock); -- 1.7.8.6
On 08/03/2012 05:27 PM, Peter Krempa wrote:
Commit ba226d334acbc49f6751b430e0c4e00f69eef6bf tried to fix crash of the daemon when a domain with a open console was destroyed. The fix was wrong as it tried to remove the callback also when the stream was aborted, where at that point the fd stream driver was already freed and removed.
This patch clears the callbacks with a helper right before the hash is freed, so that it doesn't interfere with other codepaths where the stream object is freed.
I just tried your patch, it still exists use after free issue: ==21843== 1 errors in context 1 of 11: ==21843== Invalid read of size 4 ==21843== at 0x4D2B79D: virStreamFree (libvirt.c:15345) ==21843== by 0x40B2E1: vshRunConsole (console.c:404) ==21843== by 0x4226CE: cmdRunConsole (virsh-domain.c:1658) ==21843== by 0x422AE3: cmdConsole (virsh-domain.c:1693) ==21843== by 0x42CBC4: vshCommandRun (virsh.c:1867) ==21843== by 0x42F872: main (virsh.c:3269) ==21843== Address 0x53c0250 is 0 bytes inside a block of size 40 free'd ==21843== at 0x4A0595D: free (vg_replace_malloc.c:366) ==21843== by 0x4C916C8: virFree (memory.c:309) ==21843== by 0x4D111BB: virUnrefStream (datatypes.c:1072) ==21843== by 0x4D2B7BD: virStreamFree (libvirt.c:15353) ==21843== by 0x40A984: virConsoleShutdown (console.c:103) ==21843== by 0x4C8912E: virEventPollRunOnce (event_poll.c:485) ==21843== by 0x4C87CA4: virEventRunDefaultImpl (event.c:247) ==21843== by 0x42C8A1: vshEventLoop (virsh.c:2406) ==21843== by 0x4C9C065: virThreadHelper (threads-pthread.c:161) ==21843== by 0x39CF8077F0: start_thread (pthread_create.c:301) ==21843== by 0x39CF0E570C: clone (clone.S:115) Regards, Alex
--- src/conf/virconsole.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/conf/virconsole.c b/src/conf/virconsole.c index 912aff6..b3f6eb6 100644 --- a/src/conf/virconsole.c +++ b/src/conf/virconsole.c @@ -219,9 +219,6 @@ static void virConsoleHashEntryFree(void *data, const char *pty = name; virStreamPtr st = data;
- /* remove callback from stream */ - virFDStreamSetInternalCloseCb(st, NULL, NULL, NULL); - /* free stream reference */ virStreamFree(st);
@@ -290,6 +287,18 @@ error: }
/** + * Helper to clear stream callbacks when freeing the hash + */ +static void virConsoleFreeClearCallbacks(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED) +{ + virStreamPtr st = payload; + + virFDStreamSetInternalCloseCb(st, NULL, NULL, NULL); +} + +/** * Free structures for handling open console streams. * * @cons Pointer to the private structure. @@ -300,6 +309,7 @@ void virConsoleFree(virConsolesPtr cons) return;
virMutexLock(&cons->lock); + virHashForEach(cons->hash, virConsoleFreeClearCallbacks, NULL); virHashFree(cons->hash); virMutexUnlock(&cons->lock); virMutexDestroy(&cons->lock);
On 08/03/12 11:56, Alex Jia wrote:
On 08/03/2012 05:27 PM, Peter Krempa wrote:
Commit ba226d334acbc49f6751b430e0c4e00f69eef6bf tried to fix crash of the daemon when a domain with a open console was destroyed. The fix was wrong as it tried to remove the callback also when the stream was aborted, where at that point the fd stream driver was already freed and removed.
This patch clears the callbacks with a helper right before the hash is freed, so that it doesn't interfere with other codepaths where the stream object is freed.
I just tried your patch, it still exists use after free issue:
==21843== 1 errors in context 1 of 11: ==21843== Invalid read of size 4 ==21843== at 0x4D2B79D: virStreamFree (libvirt.c:15345) ==21843== by 0x40B2E1: vshRunConsole (console.c:404) ==21843== by 0x4226CE: cmdRunConsole (virsh-domain.c:1658) ==21843== by 0x422AE3: cmdConsole (virsh-domain.c:1693) ==21843== by 0x42CBC4: vshCommandRun (virsh.c:1867) ==21843== by 0x42F872: main (virsh.c:3269) ==21843== Address 0x53c0250 is 0 bytes inside a block of size 40 free'd ==21843== at 0x4A0595D: free (vg_replace_malloc.c:366) ==21843== by 0x4C916C8: virFree (memory.c:309) ==21843== by 0x4D111BB: virUnrefStream (datatypes.c:1072) ==21843== by 0x4D2B7BD: virStreamFree (libvirt.c:15353) ==21843== by 0x40A984: virConsoleShutdown (console.c:103) ==21843== by 0x4C8912E: virEventPollRunOnce (event_poll.c:485) ==21843== by 0x4C87CA4: virEventRunDefaultImpl (event.c:247) ==21843== by 0x42C8A1: vshEventLoop (virsh.c:2406) ==21843== by 0x4C9C065: virThreadHelper (threads-pthread.c:161) ==21843== by 0x39CF8077F0: start_thread (pthread_create.c:301) ==21843== by 0x39CF0E570C: clone (clone.S:115)
We are indeed accessing already freed objects, but this problem is in virsh and not in the daemon where the patch is fixing code. PEter
Regards, Alex
On 08/03/2012 03:27 AM, Peter Krempa wrote:
Commit ba226d334acbc49f6751b430e0c4e00f69eef6bf tried to fix crash of the daemon when a domain with a open console was destroyed. The fix was
s/a open/an open/
wrong as it tried to remove the callback also when the stream was aborted, where at that point the fd stream driver was already freed and removed.
This patch clears the callbacks with a helper right before the hash is freed, so that it doesn't interfere with other codepaths where the stream object is freed. --- src/conf/virconsole.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/03/12 18:40, Eric Blake wrote:
On 08/03/2012 03:27 AM, Peter Krempa wrote:
Commit ba226d334acbc49f6751b430e0c4e00f69eef6bf tried to fix crash of the daemon when a domain with a open console was destroyed. The fix was
s/a open/an open/
wrong as it tried to remove the callback also when the stream was aborted, where at that point the fd stream driver was already freed and removed.
This patch clears the callbacks with a helper right before the hash is freed, so that it doesn't interfere with other codepaths where the stream object is freed. --- src/conf/virconsole.c | 16 +++++++++++++--- 1 files changed, 13 insertions(+), 3 deletions(-)
ACK.
Pushed. Thanks! Peter
participants (3)
-
Alex Jia -
Eric Blake -
Peter Krempa