[libvirt] [PATCH] qemu: fix libvirtd crash when querying halted cpus info

[Viktor Mihajlovski]

On 15.11.2016 13:19, Peter Krempa wrote:
> On Wed, Nov 02, 2016 at 18:56:49 +0300, Maxim Nestratov wrote:
>> It was introduced by commit 7a51d9ebb, which started to use 
>> monitor commands without job acquiring, which is unsafe and
>> leads to simultaneous access to vm->mon structure by different
>> threads.
>> 
>> Crash backtrace is the following (shortened):
>> 
>> Program received signal SIGSEGV, Segmentation fault. 
>> qemuMonitorSend (mon=mon at entry=0x7f4ef4000d20,
>> msg=msg at entry=0x7f4f18e78640) at qemu/qemu_monitor.c:1011 1011
>> while (!mon->msg->finished) {
>> 
>> 0  qemuMonitorSend () at qemu/qemu_monitor.c:1011 1
>> 0x00007f691abdc720 in qemuMonitorJSONCommandWithFd () at
>> qemu/qemu_monitor_json.c:298 2  0x00007f691abde64a in
>> qemuMonitorJSONCommand at qemu/qemu_monitor_json.c:328 3
>> qemuMonitorJSONQueryCPUs at qemu/qemu_monitor_json.c:1408 4
>> 0x00007f691abcaebd in qemuMonitorGetCPUInfo g at entry=false) at
>> qemu/qemu_monitor.c:1931 5  0x00007f691ab96863 in
>> qemuDomainRefreshVcpuHalted at qemu/qemu_domain.c:6309 6
>> 0x00007f691ac0af99 in qemuDomainGetStatsVcpu at
>> qemu/qemu_driver.c:18945 7  0x00007f691abef921 in
>> qemuDomainGetStats  at qemu/qemu_driver.c:19469 8
>> qemuConnectGetAllDomainStats at qemu/qemu_driver.c:19559 9
>> 0x00007f693382e806 in virConnectGetAllDomainStats at
>> libvirt-domain.c:11546 10 0x00007f6934470c40 in
>> remoteDispatchConnectGetAllDomainStats at remote.c:6267
>> 
>> (gdb) p mon->msg $1 = (qemuMonitorMessagePtr) 0x0
>> 
>> This change fixes it by calling qemuDomainRefreshVcpuHalted only
>> when job is acquired.
>> 
>> Signed-off-by: Maxim Nestratov <mnestratov at virtuozzo.com> --- 
>> src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed,
>> 13 insertions(+), 7 deletions(-)
>> 
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c 
>> index dd1907b..43a546d 100644 --- a/src/qemu/qemu_driver.c +++
>> b/src/qemu/qemu_driver.c @@ -18915,7 +18915,7 @@
>> qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, virDomainObjPtr
>> dom, virDomainStatsRecordPtr record, int *maxparams, -
>> unsigned int privflags ATTRIBUTE_UNUSED) +
>> unsigned int privflags) { size_t i; int ret = -1; @@ -18939,14
>> +18939,20 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, 
>> return -1;
>> 
>> if (VIR_ALLOC_N(cpuinfo, virDomainDefGetVcpus(dom->def)) < 0 || -
>> VIR_ALLOC_N(cpuwait, virDomainDefGetVcpus(dom->def)) < 0) -
>> goto cleanup; - -    if (qemuDomainRefreshVcpuHalted(driver,
>> dom, -                                    QEMU_ASYNC_JOB_NONE) ==
>> 0 && +        VIR_ALLOC_N(cpuwait,
>> virDomainDefGetVcpus(dom->def)) < 0 || VIR_ALLOC_N(cpuhalted,
>> virDomainDefGetVcpus(dom->def)) < 0) goto cleanup;
>> 
>> +    if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { +
> 
> Spurious empty line.
> 
>> +        if (qemuDomainRefreshVcpuHalted(driver, dom, +
>> QEMU_ASYNC_JOB_NONE) < 0) { +            /* it's ok to be silent
>> and go ahead, because halted vcpu info +             * wasn't
>> here from the beginning */ +            virResetLastError(); +
>> } +    } +
> 
> Not visible in this context: The output of the parameter needs to
> be suppressed if the parameter can't be requested.
> 
> Peter
> 
right, the VIR_ALLOC_N(cpuhalted...) should be conditional on the
success of qemuDomainRefreshVcpuHalted, as it was originally...

-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294