From 9ec1a56923ef992cb742670b37e5cee461e3a1c0 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Fri, 3 Apr 2009 14:10:17 +0000 Subject: [PATCH] Fix crash in svirt verification, and incorrect cleanup in VM failure paths --- ChangeLog | 13 ++++++ src/domain_conf.c | 22 +++++----- src/qemu_driver.c | 94 ++++++++++++++++++++++++------------------ src/security.c | 5 ++- src/security_selinux.c | 13 +++--- 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 + + 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 * 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; }