11:53 a.m.
As reported by Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>:
https://www.redhat.com/archives/libvir-list/2018-March/msg01156.html
there is a race to using the priv->monConfig between the Create
and destroy domain processing. The patches describe the steps taken
to resolve the issue.
John Ferlan (4):
conf: Use virDomainChrSourceDefNew for vhostuser
qemu: Use virDomainChrSourceDefNew for monConfig
conf: Convert virDomainChrSourceDefNew to return object
qemu: Obtain reference on monConfig
src/conf/domain_conf.c | 37 ++++++++++++++++++++++++++++++++-----
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 8 ++++++--
5 files changed, 43 insertions(+), 8 deletions(-)
--
2.13.6
11:53 a.m.
New subject: [libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefNew for vhostuser
Rather than using VIR_ALLOC, use the New API since we already
use the virDomainChrSourceDefFree function when done.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aacd06a87a..caf3f47c63 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11138,7 +11138,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
- if (VIR_ALLOC(def->data.vhostuser) < 0)
+ if (!(def->data.vhostuser = virDomainChrSourceDefNew(xmlopt)))
goto error;
def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
--
2.13.6
1:31 p.m.
New subject: [libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefNew for vhostuser
On 04/06/2018 12:53 PM, John Ferlan wrote:
Rather than using VIR_ALLOC, use the New API since we already
use the virDomainChrSourceDefFree function when done.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aacd06a87a..caf3f47c63 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11138,7 +11138,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
- if (VIR_ALLOC(def->data.vhostuser) < 0)
+ if (!(def->data.vhostuser = virDomainChrSourceDefNew(xmlopt)))
goto error;
def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
Reviewed-by: Laine Stump <laine(a)laine.org>
There have been cases in the past where a *New() or *Free() function was
added but not consistently used, leading to leaks or (worse, but at
least easier to detect) crashes due to null pointer dereferences. It
would really be nice if there was some way to automate the auditing of
all the VIR_ALLOCs, but I don't think it's possible with the simple make
syntax-check target (that's supposed to be a challenge to someone :-),
and there's 1223 uses of VIR_ALLOC() (6421 of VIR_FREE()), so even a one
time manual audit is really out of the question (and the results would
be immediately obsolete as soon as a new VIR_ALLOC or VIR_FREE was added).
1:47 p.m.
New subject: [libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefNew for vhostuser
On 04/06/2018 01:31 PM, Laine Stump wrote:
On 04/06/2018 12:53 PM, John Ferlan wrote:
> Rather than using VIR_ALLOC, use the New API since we already
> use the virDomainChrSourceDefFree function when done.
>
> - if (VIR_ALLOC(def->data.vhostuser) < 0)
> + if (!(def->data.vhostuser = virDomainChrSourceDefNew(xmlopt)))
> goto error;
>
> def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
Reviewed-by: Laine Stump <laine(a)laine.org>
There have been cases in the past where a *New() or *Free() function was
added but not consistently used, leading to leaks or (worse, but at
least easier to detect) crashes due to null pointer dereferences. It
would really be nice if there was some way to automate the auditing of
all the VIR_ALLOCs, but I don't think it's possible with the simple make
syntax-check target (that's supposed to be a challenge to someone :-),
and there's 1223 uses of VIR_ALLOC() (6421 of VIR_FREE()), so even a one
time manual audit is really out of the question (and the results would
be immediately obsolete as soon as a new VIR_ALLOC or VIR_FREE was added).
I wonder - is there some way we could automate the auditing, perhaps in
a debug-only mode, by expanding the VIR_ALLOC() macro to do a giant
compile-time blacklist using a chain of gcc's __builtin_choose_expr()
and __builtin_types_compatible_p(ptr, BadType) to forbid direct use of
VIR_ALLOC() on any type we add to the blacklist chain. Meanwhile, we'd
have to add a new variant of VIR_ALLOC() for use within call sites like
virDomainChrSourceDevNew(), that are performing the actual allocation
for any type that is otherwise blacklisted (and where it is easier to
audit that calls to the new macro are really limited to the constructor
functions of those blacklisted types).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
9:29 a.m.
New subject: [libvirt] [PATCH 1/4] conf: Use virDomainChrSourceDefNew for vhostuser
On Fri, Apr 06, 2018 at 06:53 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
Rather than using VIR_ALLOC, use the New API since we already
use the virDomainChrSourceDefFree function when done.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aacd06a87a..caf3f47c63 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11138,7 +11138,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
- if (VIR_ALLOC(def->data.vhostuser) < 0)
+ if (!(def->data.vhostuser = virDomainChrSourceDefNew(xmlopt)))
goto error;
def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
--
2.13.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:53 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: Use virDomainChrSourceDefNew for monConfig
Rather than VIR_ALLOC, use the New function for allocation. We
already use the Free function anyway.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
5 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index caf3f47c63..fd57364cd4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12211,7 +12211,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
}
-static virDomainChrSourceDefPtr
+virDomainChrSourceDefPtr
virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt)
{
virDomainChrSourceDefPtr def = NULL;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 68473c309e..89a7131fdb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2827,6 +2827,9 @@ bool virDomainDefHasDeviceAddress(virDomainDefPtr def,
void virDomainDefFree(virDomainDefPtr vm);
+virDomainChrSourceDefPtr
+virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt);
+
virDomainChrDefPtr virDomainChrDefNew(virDomainXMLOptionPtr xmlopt);
virDomainDefPtr virDomainDefNew(void);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c550310cc0..cab324c4d7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -218,6 +218,7 @@ virDomainChrSourceDefClear;
virDomainChrSourceDefCopy;
virDomainChrSourceDefFree;
virDomainChrSourceDefGetPath;
+virDomainChrSourceDefNew;
virDomainChrSpicevmcTypeFromString;
virDomainChrSpicevmcTypeToString;
virDomainChrTcpProtocolTypeFromString;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f03aa03e8a..bdc6e58d6e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2422,7 +2422,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
xmlNodePtr node = NULL;
virQEMUCapsPtr qemuCaps = NULL;
- if (VIR_ALLOC(priv->monConfig) < 0)
+ if (!(priv->monConfig = virDomainChrSourceDefNew(NULL)))
goto error;
if (!(monitorpath =
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c0105c8b84..a8dab92dd6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5717,7 +5717,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
goto cleanup;
}
- if (VIR_ALLOC(priv->monConfig) < 0)
+ if (!(priv->monConfig = virDomainChrSourceDefNew(NULL)))
goto cleanup;
VIR_DEBUG("Preparing monitor state");
--
2.13.6
1:47 p.m.
New subject: [libvirt] [PATCH 2/4] qemu: Use virDomainChrSourceDefNew for monConfig
On 04/06/2018 12:53 PM, John Ferlan wrote:
Rather than VIR_ALLOC, use the New function for allocation. We
already use the Free function anyway.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
5 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index caf3f47c63..fd57364cd4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12211,7 +12211,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
}
-static virDomainChrSourceDefPtr
+virDomainChrSourceDefPtr
virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt)
{
virDomainChrSourceDefPtr def = NULL;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 68473c309e..89a7131fdb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2827,6 +2827,9 @@ bool virDomainDefHasDeviceAddress(virDomainDefPtr def,
void virDomainDefFree(virDomainDefPtr vm);
+virDomainChrSourceDefPtr
+virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt);
+
virDomainChrDefPtr virDomainChrDefNew(virDomainXMLOptionPtr xmlopt);
virDomainDefPtr virDomainDefNew(void);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c550310cc0..cab324c4d7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -218,6 +218,7 @@ virDomainChrSourceDefClear;
virDomainChrSourceDefCopy;
virDomainChrSourceDefFree;
virDomainChrSourceDefGetPath;
+virDomainChrSourceDefNew;
virDomainChrSpicevmcTypeFromString;
virDomainChrSpicevmcTypeToString;
virDomainChrTcpProtocolTypeFromString;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f03aa03e8a..bdc6e58d6e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2422,7 +2422,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
xmlNodePtr node = NULL;
virQEMUCapsPtr qemuCaps = NULL;
- if (VIR_ALLOC(priv->monConfig) < 0)
+ if (!(priv->monConfig = virDomainChrSourceDefNew(NULL)))
goto error;
if (!(monitorpath =
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c0105c8b84..a8dab92dd6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5717,7 +5717,7 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
goto cleanup;
}
- if (VIR_ALLOC(priv->monConfig) < 0)
+ if (!(priv->monConfig = virDomainChrSourceDefNew(NULL)))
goto cleanup;
VIR_DEBUG("Preparing monitor state");
It's a bit obscure to get to, but qemu_parse_command.c:2487 has a
VIR_ALLOC(chr) that ends up being put into a monConfig - you'll need to
change that one to use virDomainChrSourceDefNew() to be complete. If you
do that, then:
Reviewed-by: Laine Stump <laine(a)laine.org>
9:38 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: Use virDomainChrSourceDefNew for monConfig
On Fri, Apr 06, 2018 at 06:53 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
Rather than VIR_ALLOC, use the New function for allocation. We
already use the Free function anyway.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
src/conf/domain_conf.h | 3 +++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_process.c | 2 +-
5 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index caf3f47c63..fd57364cd4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12211,7 +12211,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
}
-static virDomainChrSourceDefPtr
[…snip]
With the change suggested by Laine: Reviewed-by: Marc Hartmayer
<mhartmay(a)linux.vnet.ibm.com>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:53 a.m.
New subject: [libvirt] [PATCH 3/4] conf: Convert virDomainChrSourceDefNew to return object
Let's use object referencing to handle the ChrSourceDef. A subsequent
patch then can allow the monConfig to take an extra reference before
dropping the domain lock to then ensure nothing free's the memory that
needs to be used.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fd57364cd4..b4c5de8b33 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2260,8 +2260,10 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
return 0;
}
-void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
+static void
+virDomainChrSourceDefDispose(void *obj)
{
+ virDomainChrSourceDefPtr def = obj;
size_t i;
if (!def)
@@ -2275,11 +2277,16 @@ void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
virSecurityDeviceLabelDefFree(def->seclabels[i]);
VIR_FREE(def->seclabels);
}
+}
- VIR_FREE(def);
+void
+virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
+{
+ virObjectUnref(def);
}
+
/* virDomainChrSourceDefIsEqual:
* @src: Source
* @tgt: Target
@@ -12211,12 +12218,32 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
}
+static virClassPtr virDomainChrSourceDefClass;
+
+static int
+virDomainChrSourceDefOnceInit(void)
+{
+ virDomainChrSourceDefClass = virClassNew(virClassForObject(),
+ "virDomainChrSourceDef",
+ sizeof(virDomainChrSourceDef),
+ virDomainChrSourceDefDispose);
+ if (!virDomainChrSourceDefClass)
+ return -1;
+ else
+ return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virDomainChrSourceDef);
+
virDomainChrSourceDefPtr
virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt)
{
virDomainChrSourceDefPtr def = NULL;
- if (VIR_ALLOC(def) < 0)
+ if (virDomainChrSourceDefInitialize() < 0)
+ return NULL;
+
+ if (!(def = virObjectNew(virDomainChrSourceDefClass)))
return NULL;
if (xmlopt && xmlopt->privateData.chrSourceNew &&
--
2.13.6
9:51 a.m.
New subject: [libvirt] [PATCH 3/4] conf: Convert virDomainChrSourceDefNew to return object
On Fri, Apr 06, 2018 at 06:53 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
Let's use object referencing to handle the ChrSourceDef. A
subsequent
patch then can allow the monConfig to take an extra reference before
dropping the domain lock to then ensure nothing free's the memory that
needs to be used.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fd57364cd4..b4c5de8b33 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2260,8 +2260,10 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
return 0;
}
[…snip…]
+
+VIR_ONCE_GLOBAL_INIT(virDomainChrSourceDef);
+
virDomainChrSourceDefPtr
virDomainChrSourceDefNew(virDomainXMLOptionPtr xmlopt)
{
virDomainChrSourceDefPtr def = NULL;
- if (VIR_ALLOC(def) < 0)
+ if (virDomainChrSourceDefInitialize() < 0)
+ return NULL;
+
+ if (!(def = virObjectNew(virDomainChrSourceDefClass)))
return NULL;
if (xmlopt && xmlopt->privateData.chrSourceNew &&
!(def->privateData = xmlopt->privateData.chrSourceNew()))
VIR_FREE(def);
^^^^^^^^^^^^^^
Replace it with virDomainChrSourceDefFree(def) or virObjectUnref(def).
--
2.13.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Otherwise, Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:53 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Obtain reference on monConfig
Because we allow a QEMU_JOB_DESTROY to occur while we're starting
up and we drop the @vm lock prior to qemuMonitorOpen, it's possible
that a domain destroy operation "wins" the race, calls qemuProcessStop
which will free and reinitialize priv->monConfig. Depending on the
exact timing either qemuMonitorOpen will be passed a NULL @config
variable or it will be using free'd (and possibly reclaimed) memory
as the @config parameter - neither of which is good.
Resolve this by localizing the @monConfig, taking an extra reference,
and then once we get the @vm lock again removing our reference since
we are done with it.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_process.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a8dab92dd6..988c6b1537 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1776,6 +1776,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int
asyncJob,
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuMonitorPtr mon = NULL;
unsigned long long timeout = 0;
+ virDomainChrSourceDefPtr monConfig;
if (qemuSecuritySetDaemonSocketLabel(driver->securityManager, vm->def) < 0)
{
VIR_ERROR(_("Failed to set security context for monitor for %s"),
@@ -1794,10 +1795,12 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm,
int asyncJob,
virObjectRef(vm);
ignore_value(virTimeMillisNow(&priv->monStart));
+ monConfig = priv->monConfig;
+ virObjectRef(monConfig);
virObjectUnlock(vm);
mon = qemuMonitorOpen(vm,
- priv->monConfig,
+ monConfig,
priv->monJSON,
timeout,
&monitorCallbacks,
@@ -1813,6 +1816,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int
asyncJob,
virObjectLock(vm);
virObjectUnref(vm);
+ virObjectUnref(monConfig);
priv->monStart = 0;
if (!virDomainObjIsActive(vm)) {
--
2.13.6
9:25 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Obtain reference on monConfig
On Fri, Apr 06, 2018 at 06:53 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
Because we allow a QEMU_JOB_DESTROY to occur while we're
starting
up and we drop the @vm lock prior to qemuMonitorOpen, it's possible
that a domain destroy operation "wins" the race, calls qemuProcessStop
which will free and reinitialize priv->monConfig. Depending on the
exact timing either qemuMonitorOpen will be passed a NULL @config
variable or it will be using free'd (and possibly reclaimed) memory
as the @config parameter - neither of which is good.
Resolve this by localizing the @monConfig, taking an extra reference,
and then once we get the @vm lock again removing our reference since
we are done with it.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_process.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a8dab92dd6..988c6b1537 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1776,6 +1776,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int
asyncJob,
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuMonitorPtr mon = NULL;
unsigned long long timeout = 0;
+ virDomainChrSourceDefPtr monConfig;
if (qemuSecuritySetDaemonSocketLabel(driver->securityManager, vm->def) < 0)
{
VIR_ERROR(_("Failed to set security context for monitor for %s"),
@@ -1794,10 +1795,12 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm,
int asyncJob,
virObjectRef(vm);
ignore_value(virTimeMillisNow(&priv->monStart));
+ monConfig = priv->monConfig;
+ virObjectRef(monConfig);
virObjectUnlock(vm);
mon = qemuMonitorOpen(vm,
- priv->monConfig,
+ monConfig,
priv->monJSON,
timeout,
&monitorCallbacks,
@@ -1813,6 +1816,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int
asyncJob,
virObjectLock(vm);
virObjectUnref(vm);
+ virObjectUnref(monConfig);
Only for consistency: I would first unref @monConfig and then do the
unref for @vm.
priv->monStart = 0;
if (!virDomainObjIsActive(vm)) {
--
2.13.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
9:53 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Obtain reference on monConfig
On Mon, Apr 09, 2018 at 04:25 PM +0200, Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
wrote:
On Fri, Apr 06, 2018 at 06:53 PM +0200, John Ferlan
<jferlan(a)redhat.com> wrote:
> Because we allow a QEMU_JOB_DESTROY to occur while we're starting
> up and we drop the @vm lock prior to qemuMonitorOpen, it's possible
> that a domain destroy operation "wins" the race, calls qemuProcessStop
> which will free and reinitialize priv->monConfig. Depending on the
> exact timing either qemuMonitorOpen will be passed a NULL @config
> variable or it will be using free'd (and possibly reclaimed) memory
> as the @config parameter - neither of which is good.
>
> Resolve this by localizing the @monConfig, taking an extra reference,
> and then once we get the @vm lock again removing our reference since
> we are done with it.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_process.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index a8dab92dd6..988c6b1537 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1776,6 +1776,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm,
int asyncJob,
> qemuDomainObjPrivatePtr priv = vm->privateData;
> qemuMonitorPtr mon = NULL;
> unsigned long long timeout = 0;
> + virDomainChrSourceDefPtr monConfig;
>
> if (qemuSecuritySetDaemonSocketLabel(driver->securityManager, vm->def)
< 0) {
> VIR_ERROR(_("Failed to set security context for monitor for %s"),
> @@ -1794,10 +1795,12 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr
vm, int asyncJob,
> virObjectRef(vm);
>
> ignore_value(virTimeMillisNow(&priv->monStart));
> + monConfig = priv->monConfig;
> + virObjectRef(monConfig);
> virObjectUnlock(vm);
>
> mon = qemuMonitorOpen(vm,
> - priv->monConfig,
> + monConfig,
> priv->monJSON,
> timeout,
> &monitorCallbacks,
> @@ -1813,6 +1816,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm,
int asyncJob,
>
> virObjectLock(vm);
> virObjectUnref(vm);
> + virObjectUnref(monConfig);
Only for consistency: I would first unref @monConfig and then do the
unref for @vm.
With this change: Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> priv->monStart = 0;
>
> if (!virDomainObjIsActive(vm)) {
> --
> 2.13.6
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2419
days inactive
2422
days old
12 comments
4 participants
participants (4)
-
Eric Blake -
John Ferlan -
Laine Stump -
Marc Hartmayer