[libvirt] [PATCH] conf: Remove console stream callback only when freeing console helper

[Alex Jia]

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);