If we pass VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG to qemuGetSchedulerParametersFlags() or *nparams is less than 1, we will unlock qemu_driver without locking it. It's very dangerous. We should lock qemu_driver after calling virCheckFlags(). --- src/qemu/qemu_driver.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 768e0f2..c6994cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5832,6 +5832,8 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + qemuDriverLock(driver); + if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -5845,7 +5847,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; } - qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (vm == NULL) {

On 06/28/2011 04:09 AM, Michal Privoznik wrote: > On 28.06.2011 09:58, Wen Congyang wrote: >> If we pass VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG to >> qemuGetSchedulerParametersFlags() or *nparams is less than 1, >> we will unlock qemu_driver without locking it. It's very dangerous. >> >> We should lock qemu_driver after calling virCheckFlags(). >> >> --- >> src/qemu/qemu_driver.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 768e0f2..c6994cd 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -5832,6 +5832,8 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, >> virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | >> VIR_DOMAIN_AFFECT_CONFIG, -1); >> >> + qemuDriverLock(driver); >> + >> if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == >> (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) { >> qemuReportError(VIR_ERR_INVALID_ARG, "%s", >> @@ -5845,7 +5847,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, >> goto cleanup; >> } An alternative would have been to replace the 'goto cleanup' with 'return -1'. In fact, with that alternative, we can error out without even spending time grabbing the lock. Then again, the error is unexpected (you can assume that most callers are compliant), and optimizing for the rare bad code won't have any impact to the execution speed of good code, so I'm happy with the patch as-is.