[libvirt] [PATCH] qemu: Avoid libvirtd crash in qemuDomainObjExitAgentInternal

[Alex Jia]

On 08/07/2012 07:34 PM, Daniel P. Berrange wrote:
> On Tue, Aug 07, 2012 at 03:18:38PM +0800, Alex Jia wrote:
>> * src/qemu/qemu_domain.c (qemuDomainObjExitAgentInternal): fix crashing
>>    libvirtd due to derefing a NULL pointer.
>>
>> For details, please see bug:
>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=845966
>>
>> Signed-off-by: Alex Jia<ajia at redhat.com>
>> ---
>>   src/qemu/qemu_domain.c |   10 ++++++----
>>   1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 86f0265..8667b6c 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1136,12 +1136,14 @@ qemuDomainObjExitAgentInternal(struct qemud_driver *driver,
>>                                  virDomainObjPtr obj)
>>   {
>>       qemuDomainObjPrivatePtr priv = obj->privateData;
>> -    int refs;
>> +    int refs = -1;
>>
>> -    refs = qemuAgentUnref(priv->agent);
>> +    if (priv->agent) {
>> +        refs = qemuAgentUnref(priv->agent);
>>
>> -    if (refs>  0)
>> -        qemuAgentUnlock(priv->agent);
>> +        if (refs>  0)
>> +            qemuAgentUnlock(priv->agent);
>> +    }
>>
>>       if (driver_locked)
>>           qemuDriverLock(driver);
> I'm not convinced this is the right fix. The whole point of the Enter/ExitAgent
> methods is to hold an extra reference on priv->agent, so that it is *not*
> deleted while a agent command is run.
>
> What is setting priv->agent to NULL while the command is still active ?

In fact, the command 'guest-suspend-disk' is freed by virJSONValueFree() 
in qemuAgentSuspend() after the command is successfully sent via 
'qemuAgentCommand()':

(gdb) s
qemuDomainPMSuspendForDuration (dom=<value optimized out>, target=1, 
duration=<value optimized out>, flags=<value optimized out>) at 
qemu/qemu_driver.c:13123
13123       qemuDomainObjExitAgent(driver, vm);
(gdb) p *vm
$68 = {object = {magic = 3405643788, refs = 4, klass = 0x7f4ce815a9b0}, 
lock = {lock = {__data = {__lock = 1, __count = 0, __owner = 20285, 
__nusers = 1, __kind = 0, __spins = 0, __list = {__prev = 0x0,
           __next = 0x0}}, __size = 
"\001\000\000\000\000\000\000\000=O\000\000\001", '\000' <repeats 26 
times>, __align = 1}}, pid = 20379, *state = {state = 4, reason = 0}*, 
autostart = 0, persistent = 1,
   updated = 0, def = 0x7f4ce815e500, newDef = 0x7f4ce8069b80, snapshots 
= {objs = 0x7f4ce815e240, metaroot = {def = 0x0, parent = 0x0, sibling = 
0x0, nchildren = 0, first_child = 0x0}},
   current_snapshot = 0x0, hasManagedSave = false, privateData = 
0x7f4ce8154660, privateDataFreeFunc = 0x7f4cefada190 
<qemuDomainObjPrivateFree>, taint = 4}
  (gdb) s
13122       ret = qemuAgentSuspend(priv->agent, target);

(gdb) p *priv
$70 = {job = {cond = {cond = {__data = {__lock = 0, __futex = 0, 
__total_seq = 0, __wakeup_seq = 0, __woken_seq = 0, __mutex = 0x0, 
__nwaiters = 0, __broadcast_seq = 0}, __size = '\000' <repeats 47 times>,
         __align = 0}}, active = QEMU_JOB_MODIFY, owner = 20286, 
asyncCond = {cond = {__data = {__lock = 0, __futex = 0, __total_seq = 0, 
__wakeup_seq = 0, __woken_seq = 0, __mutex = 0x0, __nwaiters = 0,
           __broadcast_seq = 0}, __size = '\000' <repeats 47 times>, 
__align = 0}}, asyncJob = QEMU_ASYNC_JOB_NONE, asyncOwner = 0, phase = 
0, mask = 0, start = 0, dump_memory_only = false, info = {
       type = 0, timeElapsed = 0, timeRemaining = 0, dataTotal = 0, 
dataProcessed = 0, dataRemaining = 0, memTotal = 0, memProcessed = 0, 
memRemaining = 0, fileTotal = 0, fileProcessed = 0,
       fileRemaining = 0}}, mon = 0x7f4ce80a3570, monConfig = 0x0, 
monJSON = 1, monError = false, monStart = 0, *agent = 0x0*, agentError = 
false, agentStart = 1344402957193, gotShutdown = true,
   beingDestroyed = false, pidfile = 0x7f4ce816ba90 
"/var/run/libvirt/qemu/myRHEL6.pid", nvcpupids = 1, vcpupids = 
0x7f4ce8146be0, pciaddrs = 0x7f4ce8171b30, persistentAddrs = 1, qemuCaps 
= 0x7f4ce80b4030,
   lockState = 0x0, fakeReboot = false, jobs_queued = 1, migMaxBandwidth 
= 32, origname = 0x0, cons = 0x7f4ce8165ce0, cleanupCallbacks = 0x0, 
ncleanupCallbacks = 0, ncleanupCallbacks_max = 0}
(gdb) p priv->agent
$71 =*(qemuAgentPtr) 0x0*
(gdb) s
1138        refs = qemuAgentUnref(priv->agent);
(gdb) s
qemuAgentUnref (mon=0x0) at qemu/qemu_agent.c:168
(gdb) s
170         VIR_DEBUG("%d", mon->refs);
(gdb) s
168     {
(gdb) s
169         mon->refs--;
(gdb) s

Program received signal SIGSEGV, Segmentation fault.
qemuAgentUnref (*mon=0x0*) at qemu/qemu_agent.c:169
169 *mon->refs--*;
(gdb) s
virNetServerFatalSignal (sig=11, siginfo=0x7f4cf748f630, 
context=0x7f4cf748f500) at rpc/virnetserver.c:296


In addition, the old qemuAgentUnref(mon) hasn't judge whether its 
parameter is NULL then will deref a NULL pointer, I should simply fix it 
in  qemuAgentUnref(), for example, if 'mon' is NULL then directly return.

Fortunately, your commit b57ee09 potentially fix this issue via using 
virObjectUnref() instead of qemuAgentUnref(), if the parameter 
'priv->agent' is NULL then the virObjectUnref(priv->agent) will directly 
return false:

bool virObjectUnref(void *anyobj)
{
     virObjectPtr obj = anyobj;

     if (!obj)
         return false;

     ......
}


Regards,
Alex
> Daniel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120808/f12df7b2/attachment-0001.htm>