Re: [libvirt] [PATCH v2] qemu: Prevent crash of libvirtd without guest agent configuration

16 Jul 2013

On 07/16/13 13:42, Peter Krempa wrote:
...
On 07/16/13 11:30, Alex Jia wrote:
...
If users haven't configured guest agent then qemuAgentCommand() will
dereference a NULL 'mon' pointer, which causes crash of libvirtd.
With the patch, when the qemu-ga service isn't running in the guest,
a expected error "error: Guest agent is not responding: Guest agent
not available for now" will be raised, and the error "error: argument
unsupported: QEMU guest agent is not configured" is raised when the
guest hasn't configured guest agent.
GDB backtrace:
(gdb) bt
#0  virNetServerFatalSignal (sig=11, siginfo=<value optimized out>,
context=<value optimized out>) at rpc/virnetserver.c:326
#1  <signal handler called>
#2  qemuAgentCommand (mon=0x0, cmd=0x7f39300017b0,
reply=0x7f394b090910, seconds=-2) at qemu/qemu_agent.c:975
#3  0x00007f39429507f6 in qemuAgentGetVCPUs (mon=0x0,
info=0x7f394b0909b8) at qemu/qemu_agent.c:1475
#4  0x00007f39429d9857 in qemuDomainGetVcpusFlags (dom=<value
optimized out>, flags=9) at qemu/qemu_driver.c:4849
#5  0x00007f3957dffd8d in virDomainGetVcpusFlags
(domain=0x7f39300009c0, flags=8) at libvirt.c:9843
How to reproduce?
# To start a guest without guest agent configuration
# then run the following cmdline
# virsh vcpucount foobar --guest
error: End of file while reading data: Input/output error
error: One or more references were leaked after disconnect from the
hypervisor
error: Failed to reconnect to the hypervisor
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=984821
Signed-off-by: Alex Jia <ajia@redhat.com>
---
  src/qemu/qemu_driver.c |   26 ++++++++++++++++++++++++++
  1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 495867a..699388c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4122,6 +4122,19 @@ qemuDomainSetVcpusFlags(virDomainPtr dom,
unsigned int nvcpus,
              goto endjob;
          }
+        if (priv->agentError) {
+            virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
+                           _("QEMU guest agent is not "
+                             "available due to an error"));
+            goto cleanup;
this needs to be "goto endjob"
...
+        }
+
+        if (!priv->agent) {
+            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                           _("QEMU guest agent is not configured"));
+            goto cleanup;
here too.
...
+        }
+
          qemuDomainObjEnterAgent(vm);
          ncpuinfo = qemuAgentGetVCPUs(priv->agent, &cpuinfo);
          qemuDomainObjExitAgent(vm);
@@ -4839,6 +4852,19 @@ qemuDomainGetVcpusFlags(virDomainPtr dom,
unsigned int flags)
          if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
              goto cleanup;
+        if (priv->agentError) {
+            virReportError(VIR_ERR_AGENT_UNRESPONSIVE, "%s",
+                           _("QEMU guest agent is not "
+                             "available due to an error"));
+            goto cleanup;
+        }
+
+        if (!priv->agent) {
+            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                           _("QEMU guest agent is not configured"));
+            goto cleanup;
+        }
+
          if (!virDomainObjIsActive(vm)) {
              virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                             _("domain is not running"));
and also both here.
ACK with that changed.
I amended the commit message to be a bit more specific and changed the 
goto labels and pushed this patch. Thanks for finding this issue.
...
Peter
Peter

    