Thanks for your guides, I'm sorry. this is the first time I send patch to libvirt. I'll modify it later and use git send-email to send another mail. -----邮件原件----- 发件人: John Ferlan [mailto:jferlan@xxxxxxxxxx] 发送时间: 2017年3月8日 3:15 收件人: Caoxinhua; libvir-list@xxxxxxxxxx 抄送: Huangweidong (C); Yanqiangjun; weifuqiang; Wangjing (King, Euler) 主题: Re: [PATCH] libvirt-remote:fix use-after-free when sending event message 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
