Re: [PATCH] libvirt-remote:fix use-after-free when sending event message

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/02/2017 03:02 AM, Caoxinhua wrote:
> From 6b42792cfee124a742999e698e348e99b382ba16 Mon Sep 17 00:00:00 2001
> 
> From: c00207022 <caoxinhua@xxxxxxxxxx>
> 

I cannot 'git am -3' this patch - please follow the guidelines :

http://libvirt.org/hacking.html

and use git send-email rather than attaching some patch

Also we prefer to have a "real name" associated with patches not just an
email address.

Finally when you're posting followup patches - it's considered good
etiquette to include a link to previous patches either in a cover letter
or under the --- after your signoff so the reviewer can check the
history.  The prior patch I see on this is:

http://www.redhat.com/archives/libvir-list/2017-February/msg00452.html

Thanks -

John

> Date: Thu, 2 Mar 2017 12:07:27 +0800
> 
> Subject: [PATCH] libvirt-remote:fix use-after-free when sending event
> message
> 
>  
> 
> [Changelog]:If there is a process with a client which registers event
> callbacks,
> 
> and it calls libvirt's API which uses the same virConnectPtr in that
> 
> callback function. When this process exit abnormally lead to client
> 
> disconnect, there is a possibility that the libvirtd's main thread is use
> 
> virServerClient to send event just after the virServerClient been freed
> by job
> 
> thread.
> 
> Following is backtrace:
> 
>  
> 
> #0  0x00007fda223d66d8 in virClassIsDerivedFrom (klass=0xdeadbeef,
> parent=0x7fda24c81b40) at util/virobject.c:169
> 
> #1  0x00007fda223d6a1e in virObjectIsClass
> (anyobj=anyobj@entry=0x7fd9e575b400, klass=<optimized out>) at
> util/virobject.c:365
> 
> #2  0x00007fda223d6a44 in virObjectLock (anyobj=0x7fd9e575b400) at
> util/virobject.c:317
> 
> #3  0x00007fda22507f71 in virNetServerClientSendMessage
> (client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
> 
> at rpc/virnetserverclient.c:1422
> 
> #4  0x00007fda230d714d in remoteDispatchObjectEventSend
> (client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=348,
> 
> proc=0x7fda2310e5e0 <xdr_remote_domain_event_callback_tunable_msg>,
> data=0x7ffc3857fdb0) at remote.c:3803
> 
> #5  0x00007fda230dd71b in remoteRelayDomainEventTunable (conn=<optimized
> out>, dom=0x7fda27cd7660, params=0x7fda27f3aae0, nparams=1,
> 
> opaque=0x7fd9e6c99e00) at remote.c:1033
> 
> #6  0x00007fda224484cb in virDomainEventDispatchDefaultFunc
> (conn=0x7fda27cd0120, event=0x7fda2736ea00,
> 
> cb=0x7fda230dd610 <remoteRelayDomainEventTunable>,
> cbopaque=0x7fd9e6c99e00) at conf/domain_event.c:1910
> 
> #7  0x00007fda22446871 in virObjectEventStateDispatchCallbacks
> (callbacks=<optimized out>, callbacks=<optimized out>, event=0x7fda2736ea00,
> 
> state=0x7fda24ca3960) at conf/object_event.c:722
> 
> #8  virObjectEventStateQueueDispatch (callbacks=0x7fda24c65800,
> queue=0x7ffc3857fe90, state=0x7fda24ca3960) at conf/object_event.c:736
> 
> #9  virObjectEventStateFlush (state=0x7fda24ca3960) at
> conf/object_event.c:814
> 
> #10 virObjectEventTimer (timer=<optimized out>, opaque=0x7fda24ca3960)
> at conf/object_event.c:560
> 
> #11 0x00007fda223ae8b9 in virEventPollDispatchTimeouts () at
> util/vireventpoll.c:458
> 
> #12 virEventPollRunOnce () at util/vireventpoll.c:654
> 
> #13 0x00007fda223ad1d2 in virEventRunDefaultImpl () at util/virevent.c:314
> 
> #14 0x00007fda225046cd in virNetDaemonRun (dmn=0x7fda24c775c0) at
> rpc/virnetdaemon.c:818
> 
> #15 0x00007fda230d6351 in main (argc=<optimized out>, argv=<optimized
> out>) at libvirtd.c:1623
> 
>  
> 
> Let's clean all event callback when client close.
> 
>  
> 
> Signed-off-by: Caoxinhua <caoxinhua@xxxxxxxxxx>
> 
> ---
> 
> daemon/remote.c | 76
> +++++++++++++++++++++------------------------------------
> 
> 1 file changed, 28 insertions(+), 48 deletions(-)
> 
>  
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> 
> index 23c9de4..fb7cd50 100644
> 
> --- a/daemon/remote.c
> 
> +++ b/daemon/remote.c
> 
> @@ -101,6 +101,7 @@ static void
> make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNod
> 
> static void make_nonnull_secret(remote_nonnull_secret *secret_dst,
> virSecretPtr secret_src);
> 
> static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst,
> virNWFilterPtr nwfilter_src);
> 
> static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot
> *snapshot_dst, virDomainSnapshotPtr snapshot_src);
> 
> +static void remoteClientCleanEventCallbacks(struct daemonClientPrivate
> *priv);
> 
>  static int
> 
> remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
> 
> @@ -1483,24 +1484,15 @@ void
> remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r
> 
>                                    &msg);
> 
> }
> 
> -/*
> 
> - * You must hold lock for at least the client
> 
> - * We don't free stuff here, merely disconnect the client's
> 
> - * network socket & resources.
> 
> - * We keep the libvirt connection open until any async
> 
> - * jobs have finished, then clean it up elsewhere
> 
> - */
> 
> -void remoteClientFreeFunc(void *data)
> 
> +static void remoteClientCleanEventCallbacks(struct daemonClientPrivate
> *priv)
> 
> {
> 
> -    struct daemonClientPrivate *priv = data;
> 
> -
> 
>      /* Deregister event delivery callback */
> 
> -    if (priv->conn) {
> 
> -        virIdentityPtr sysident = virIdentityGetSystem();
> 
> +    if (priv && priv->conn) {
> 
>          size_t i;
> 
> -
> 
> +        virIdentityPtr sysident = virIdentityGetSystem();
> 
>          virIdentitySetCurrent(sysident);
> 
> +        virMutexLock(&priv->lock);
> 
>          for (i = 0; i < priv->ndomainEventCallbacks; i++) {
> 
>              int callbackID = priv->domainEventCallbacks[i]->callbackID;
> 
>              if (callbackID < 0) {
> 
> @@ -1514,6 +1506,7 @@ void remoteClientFreeFunc(void *data)
> 
>                  VIR_WARN("unexpected domain event deregister failure");
> 
>          }
> 
>          VIR_FREE(priv->domainEventCallbacks);
> 
> +        priv->ndomainEventCallbacks = 0;
> 
>          for (i = 0; i < priv->nnetworkEventCallbacks; i++) {
> 
>              int callbackID = priv->networkEventCallbacks[i]->callbackID;
> 
> @@ -1529,36 +1522,7 @@ void remoteClientFreeFunc(void *data)
> 
>                  VIR_WARN("unexpected network event deregister failure");
> 
>          }
> 
>          VIR_FREE(priv->networkEventCallbacks);
> 
> -
> 
> -        for (i = 0; i < priv->nstorageEventCallbacks; i++) {
> 
> -            int callbackID = priv->storageEventCallbacks[i]->callbackID;
> 
> -            if (callbackID < 0) {
> 
> -                VIR_WARN("unexpected incomplete storage pool callback
> %zu", i);
> 
> -                continue;
> 
> -            }
> 
> -            VIR_DEBUG("Deregistering remote storage pool event relay %d",
> 
> -                      callbackID);
> 
> -            priv->storageEventCallbacks[i]->callbackID = -1;
> 
> -            if (virConnectStoragePoolEventDeregisterAny(priv->conn,
> 
> -                                                        callbackID) < 0)
> 
> -                VIR_WARN("unexpected storage pool event deregister
> failure");
> 
> -        }
> 
> -        VIR_FREE(priv->storageEventCallbacks);
> 
> -
> 
> -        for (i = 0; i < priv->nnodeDeviceEventCallbacks; i++) {
> 
> -            int callbackID = priv->nodeDeviceEventCallbacks[i]->callbackID;
> 
> -            if (callbackID < 0) {
> 
> -                VIR_WARN("unexpected incomplete node device callback
> %zu", i);
> 
> -                continue;
> 
> -            }
> 
> -            VIR_DEBUG("Deregistering remote node device event relay %d",
> 
> -                      callbackID);
> 
> -            priv->nodeDeviceEventCallbacks[i]->callbackID = -1;
> 
> -            if (virConnectNodeDeviceEventDeregisterAny(priv->conn,
> 
> -                                                       callbackID) < 0)
> 
> -                VIR_WARN("unexpected node device event deregister
> failure");
> 
> -        }
> 
> -        VIR_FREE(priv->nodeDeviceEventCallbacks);
> 
> +        priv->nnetworkEventCallbacks = 0;
> 
>          for (i = 0; i < priv->nqemuEventCallbacks; i++) {
> 
>              int callbackID = priv->qemuEventCallbacks[i]->callbackID;
> 
> @@ -1574,31 +1538,47 @@ void remoteClientFreeFunc(void *data)
> 
>                  VIR_WARN("unexpected qemu monitor event deregister
> failure");
> 
>          }
> 
>          VIR_FREE(priv->qemuEventCallbacks);
> 
> +        priv->nqemuEventCallbacks = 0;
> 
>          if (priv->closeRegistered) {
> 
>              if (virConnectUnregisterCloseCallback(priv->conn,
> 
>                                                   
> remoteRelayConnectionClosedEvent) < 0)
> 
>                  VIR_WARN("unexpected close callback event deregister
> failure");
> 
>          }
> 
> -
> 
> -        virConnectClose(priv->conn);
> 
> -
> 
> +        virMutexUnlock(&priv->lock);
> 
>          virIdentitySetCurrent(NULL);
> 
>          virObjectUnref(sysident);
> 
>      }
> 
> +}
> 
> +
> 
> +
> 
> +/*
> 
> + * You must hold lock for at least the client
> 
> + * We don't free stuff here, merely disconnect the client's
> 
> + * network socket & resources.
> 
> + * We keep the libvirt connection open until any async
> 
> + * jobs have finished, then clean it up elsewhere
> 
> + */
> 
> +void remoteClientFreeFunc(void *data)
> 
> +{
> 
> +    struct daemonClientPrivate *priv = data;
> 
> +    if (!priv || !priv->conn)
> 
> +        return;
> 
> +
> 
> +    remoteClientCleanEventCallbacks(priv);
> 
> +    virConnectClose(priv->conn);
> 
>      VIR_FREE(priv);
> 
> }
> 
> -
> 
> static void remoteClientCloseFunc(virNetServerClientPtr client)
> 
> {
> 
>      struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
> 
>      daemonRemoveAllClientStreams(priv->streams);
> 
> +    remoteClientCleanEventCallbacks(priv);
> 
> }
> 
> -
> 
> void *remoteClientInitHook(virNetServerClientPtr client,
> 
>                             void *opaque ATTRIBUTE_UNUSED)
> 
> {
> 
> -- 
> 
> 1.8.3.1
> 
>  
> 
>  
> 
>  
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux