Fix crash in svirt verification, and incorrect cleanup in VM failure paths

5 files changed, 89 insertions, 58 deletions

diff --git a/ChangeLog b/ChangeLog
index 556354a236..fa26cd5638 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+Fri Apr  3 15:07:00 BST 2009 Daniel P. Berrange <berrange@redhat.com>
+
+	Fix crash in svirt verification, and incorrect cleanup in
+	VM failure paths.
+	* src/domain_conf.c: Don't extract 'model' from seclabel unless
+	requesting 'live' config, or if its a static label. Add missing
+	error report
+	* src/qemu_driver.c: Fix cleanup in auto-reconnect to running VMs.
+	Fix cleanup of resources if starting a new VM fails
+	* src/security.c: Fix crash if no seclabel model is defined in
+	the virSecuriyDriverVerify method
+	* src/security_selinux.c: Fix error message typo & fix whitespace
+
 Fri Apr  3 15:03:00 BST 2009 Daniel P. Berrange <berrange@redhat.com>
 
 	* src/virsh.c: Add --console arg for create & start commands
diff --git a/src/domain_conf.c b/src/domain_conf.c
index 0efa50feec..2c339af276 100644
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -1859,15 +1859,6 @@ virSecurityLabelDefParseXML(virConnectPtr conn,
     if (virXPathNode(conn, "./seclabel", ctxt) == NULL)
         return 0;
 
-    p = virXPathStringLimit(conn, "string(./seclabel/@model)",
-                            VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
-    if (p == NULL) {
-        virDomainReportError(conn, VIR_ERR_XML_ERROR,
-                             "%s", _("missing security model"));
-        goto error;
-    }
-    def->seclabel.model = p;
-
     p = virXPathStringLimit(conn, "string(./seclabel/@type)",
                             VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
     if (p == NULL) {
@@ -1888,6 +1879,14 @@ virSecurityLabelDefParseXML(virConnectPtr conn,
      */
     if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC ||
         !(flags & VIR_DOMAIN_XML_INACTIVE)) {
+        p = virXPathStringLimit(conn, "string(./seclabel/@model)",
+                                VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
+        if (p == NULL) {
+            virDomainReportError(conn, VIR_ERR_XML_ERROR,
+                                 "%s", _("missing security model"));
+            goto error;
+        }
+        def->seclabel.model = p;
 
         p = virXPathStringLimit(conn, "string(./seclabel/label[1])",
                                 VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
@@ -1905,8 +1904,11 @@ virSecurityLabelDefParseXML(virConnectPtr conn,
         !(flags & VIR_DOMAIN_XML_INACTIVE)) {
         p = virXPathStringLimit(conn, "string(./seclabel/imagelabel[1])",
                                 VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
-        if (p == NULL)
+        if (p == NULL) {
+            virDomainReportError(conn, VIR_ERR_XML_ERROR,
+                                 _("security imagelabel is missing"));
             goto error;
+        }
         def->seclabel.imagelabel = p;
     }
 
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 4a0ae8268c..79ee0726b5 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -332,7 +332,7 @@ qemudReconnectVMs(struct qemud_driver *driver)
         }
 
         if ((vm->logfile = qemudLogFD(NULL, driver->logDir, vm->def->name)) < 0)
-            return -1;
+            goto next_error;
 
         if (vm->def->id >= driver->nextvmid)
             driver->nextvmid = vm->def->id + 1;
@@ -344,9 +344,12 @@ next_error:
         /* we failed to reconnect the vm so remove it's traces */
         vm->def->id = -1;
         qemudRemoveDomainStatus(NULL, driver, vm);
-        virDomainDefFree(vm->def);
-        vm->def = vm->newDef;
-        vm->newDef = NULL;
+        /* Restore orignal def, if we'd loaded a live one */
+        if (vm->newDef) {
+            virDomainDefFree(vm->def);
+            vm->def = vm->newDef;
+            vm->newDef = NULL;
+        }
 next:
         virDomainObjUnlock(vm);
         if (status)
@@ -1319,14 +1322,6 @@ static int qemudStartVMDaemon(virConnectPtr conn,
     hookData.vm = vm;
     hookData.driver = driver;
 
-   /* If you are using a SecurityDriver with dynamic labelling,
-      then generate a security label for isolation */
-    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
-        driver->securityDriver &&
-        driver->securityDriver->domainGenSecurityLabel &&
-        driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
-        return -1;
-
     FD_ZERO(&keepfd);
 
     if (virDomainIsActive(vm)) {
@@ -1335,6 +1330,14 @@ static int qemudStartVMDaemon(virConnectPtr conn,
         return -1;
     }
 
+    /* If you are using a SecurityDriver with dynamic labelling,
+       then generate a security label for isolation */
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
+        driver->securityDriver &&
+        driver->securityDriver->domainGenSecurityLabel &&
+        driver->securityDriver->domainGenSecurityLabel(conn, vm) < 0)
+        return -1;
+
     if (vm->def->graphics &&
         vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
         vm->def->graphics->data.vnc.autoport) {
@@ -1342,7 +1345,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
         if (port < 0) {
             qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                              "%s", _("Unable to find an unused VNC port"));
-            return -1;
+            goto cleanup;
         }
         vm->def->graphics->data.vnc.port = port;
     }
@@ -1351,17 +1354,17 @@ static int qemudStartVMDaemon(virConnectPtr conn,
         virReportSystemError(conn, errno,
                              _("cannot create log directory %s"),
                              driver->logDir);
-        return -1;
+        goto cleanup;
     }
 
     if((vm->logfile = qemudLogFD(conn, driver->logDir, vm->def->name)) < 0)
-        return -1;
+        goto cleanup;
 
     emulator = vm->def->emulator;
     if (!emulator)
         emulator = virDomainDefDefaultEmulator(conn, vm->def, driver->caps);
     if (!emulator)
-        return -1;
+        goto cleanup;
 
     /* Make sure the binary we are about to try exec'ing exists.
      * Technically we could catch the exec() failure, but that's
@@ -1371,7 +1374,7 @@ static int qemudStartVMDaemon(virConnectPtr conn,
         virReportSystemError(conn, errno,
                              _("Cannot find QEMU binary %s"),
                              emulator);
-        return -1;
+        goto cleanup;
     }
 
     if (qemudExtractVersionInfo(emulator,
@@ -1380,21 +1383,17 @@ static int qemudStartVMDaemon(virConnectPtr conn,
         qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                          _("Cannot determine QEMU argv syntax %s"),
                          emulator);
-        return -1;
+        goto cleanup;
     }
 
     if (qemuPrepareHostDevices(conn, vm->def) < 0)
-        return -1;
+        goto cleanup;
 
     vm->def->id = driver->nextvmid++;
     if (qemudBuildCommandLine(conn, driver, vm,
                               qemuCmdFlags, &argv, &progenv,
-                              &tapfds, &ntapfds, migrateFrom) < 0) {
-        close(vm->logfile);
-        vm->def->id = -1;
-        vm->logfile = -1;
-        return -1;
-    }
+                              &tapfds, &ntapfds, migrateFrom) < 0)
+        goto cleanup;
 
     tmp = progenv;
     while (*tmp) {
@@ -1457,12 +1456,8 @@ static int qemudStartVMDaemon(virConnectPtr conn,
                              "%s", _("Unable to daemonize QEMU process"));
             ret = -1;
         }
-    }
-
-    if (ret == 0) {
         vm->state = migrateFrom ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING;
-    } else
-        vm->def->id = -1;
+    }
 
     for (i = 0 ; argv[i] ; i++)
         VIR_FREE(argv[i]);
@@ -1479,19 +1474,38 @@ static int qemudStartVMDaemon(virConnectPtr conn,
         VIR_FREE(tapfds);
     }
 
-    if (ret == 0) {
-        if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) ||
-            (qemudDetectVcpuPIDs(conn, vm) < 0) ||
-            (qemudInitCpus(conn, vm, migrateFrom) < 0) ||
-            (qemudInitPasswords(conn, driver, vm) < 0) ||
-            (qemudDomainSetMemoryBalloon(conn, vm, vm->def->memory) < 0) ||
-            (qemudSaveDomainStatus(conn, qemu_driver, vm) < 0)) {
-            qemudShutdownVMDaemon(conn, driver, vm);
-            return -1;
-        }
+    if (ret == -1)
+        goto cleanup;
+
+    if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) ||
+        (qemudDetectVcpuPIDs(conn, vm) < 0) ||
+        (qemudInitCpus(conn, vm, migrateFrom) < 0) ||
+        (qemudInitPasswords(conn, driver, vm) < 0) ||
+        (qemudDomainSetMemoryBalloon(conn, vm, vm->def->memory) < 0) ||
+        (qemudSaveDomainStatus(conn, qemu_driver, vm) < 0)) {
+        qemudShutdownVMDaemon(conn, driver, vm);
+        ret = -1;
+        /* No need for 'goto cleanup' now since qemudShutdownVMDaemon does enough */
     }
 
     return ret;
+
+cleanup:
+    if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
+        VIR_FREE(vm->def->seclabel.model);
+        VIR_FREE(vm->def->seclabel.label);
+        VIR_FREE(vm->def->seclabel.imagelabel);
+    }
+    if (vm->def->graphics &&
+        vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+        vm->def->graphics->data.vnc.autoport)
+        vm->def->graphics->data.vnc.port = -1;
+    if (vm->logfile != -1) {
+        close(vm->logfile);
+        vm->logfile = -1;
+    }
+    vm->def->id = -1;
+    return -1;
 }
 
 
diff --git a/src/security.c b/src/security.c
index 573895e8d4..4138547105 100644
--- a/src/security.c
+++ b/src/security.c
@@ -33,7 +33,8 @@ virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def)
     unsigned int i;
     const virSecurityLabelDefPtr secdef = &def->seclabel;
 
-    if (STREQ(secdef->model, "none"))
+    if (!secdef->model ||
+        STREQ(secdef->model, "none"))
         return 0;
 
     for (i = 0; security_drivers[i] != NULL ; i++) {
@@ -42,7 +43,7 @@ virSecurityDriverVerify(virConnectPtr conn, virDomainDefPtr def)
         }
     }
     virSecurityReportError(conn, VIR_ERR_XML_ERROR,
-                           _("invalid security model"));
+                           _("invalid security model '%s'"), secdef->model);
     return -1;
 }
 
diff --git a/src/security_selinux.c b/src/security_selinux.c
index 73d6aeaba0..ac317d791d 100644
--- a/src/security_selinux.c
+++ b/src/security_selinux.c
@@ -162,11 +162,12 @@ SELinuxGenSecurityLabel(virConnectPtr conn,
     char *scontext = NULL;
     int c1 = 0;
     int c2 = 0;
-    if ( ( vm->def->seclabel.label ) ||
-         ( vm->def->seclabel.model ) ||
-         ( vm->def->seclabel.imagelabel )) {
-        virSecurityReportError(conn, VIR_ERR_ERROR,
-                               "%s", _("security labellin already defined for VM"));
+
+    if (vm->def->seclabel.label ||
+        vm->def->seclabel.model ||
+        vm->def->seclabel.imagelabel) {
+        virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                               "%s", _("security label already defined for VM"));
         return rc;
     }
 
@@ -197,7 +198,7 @@ SELinuxGenSecurityLabel(virConnectPtr conn,
         goto err;
     }
     vm->def->seclabel.model = strdup(SECURITY_SELINUX_NAME);
-    if (! vm->def->seclabel.model) {
+    if (!vm->def->seclabel.model) {
         virReportOOMError(conn);
         goto err;
     }