[libvirt] [PATCH 00/10] Patches for various coverity issues
locking: Fixes Coverity OVERRUN warning due to the size of the source string being 19 bytes and the memcpy size of 48 bytes security: Fixes Coverity warning STRING_OVERFLOW about due to the copy being from a variable sized string to a fixed length string. Even though on read other checks ensure the source buffer is no longer than the number of bytes in the destination, Coverity doesn't know that. rpc: Fixes Coverity warning DEADCODE due to removal of code as specified in the commit message parallels: Fixes Coverity warning NULL_RETURNS from not checking result of virJSONValueObjectGetString() call as part of commit id: 8ce9e2ab util: Fixes Coverity warning NULL_RETURNS due to not checking the return value 'child' from a virJSONValueNewObject() call. qemu: Fixes Coverity warning NULL_RETURNS from not checking the return value 'activeDev' from a pciDeviceListFind() call. selinux: Fixes Coverity warning RESOURCE_LEAK as a result of code specified in the commit message esx: Fixes Coverity warning REVERSE_INULL as a result of needlessly checking if objectSpec != NULL. At the point in the code, objectSpec must be valid. virobject: Fixes Coverity warning CONSTANT_EXPRESSION_RESULT and DEADCODE as a result of commit id b545f65 using "if (!virObjectInitialize() < 0)" network: Fixes Coverity warning UNUSED_VALUE as specific in the commit message John Ferlan (10): locking: use virStrcpy instead of memcpy security: Use virStrcpy to move the label rpc: Remove call to virKeepAliveStop at init parallels: Need to handle virJSONValueObjectGetString error util: Need to check child JSON allocation before use qemu: Check valid activeDev before calling pciDeviceSetUsedBy selinux: Resolve resource leak using the default disk label esx: No need to check for objectSpec virobject: Remove the bogus ! from call to virObjectInitialize() network: Remove dead code getting, but not using ipdef src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/esx/esx_vi.c | 7 +++---- src/locking/lock_driver_sanlock.c | 9 ++++++++- src/network/bridge_driver.c | 3 --- src/parallels/parallels_driver.c | 5 ++++- src/qemu/qemu_hostdev.c | 3 ++- src/rpc/virnetserverclient.c | 2 -- src/security/security_dac.c | 8 +++++++- src/security/security_selinux.c | 6 +++--- src/util/virlockspace.c | 3 +++ src/util/virobject.c | 4 ++-- 12 files changed, 64 insertions(+), 18 deletions(-) -- 1.7.11.7
--- src/locking/lock_driver_sanlock.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 0b7c6d5..a054806 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -206,7 +206,14 @@ static int virLockManagerSanlockSetupLockspace(void) goto error; } - memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); + if (!virStrcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, + SANLK_NAME_LEN)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Lockspace path '%s' exceeded %d characters"), + VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, + SANLK_PATH_LEN); + goto error; + } ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
--- src/locking/lock_driver_sanlock.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 0b7c6d5..a054806 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -206,7 +206,14 @@ static int virLockManagerSanlockSetupLockspace(void) goto error; }
- memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); + if (!virStrcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE,
ls.name is static in this function, you can use virStrcpyStatic here to save the length parameter.
+ SANLK_NAME_LEN)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Lockspace path '%s' exceeded %d characters"), + VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, + SANLK_PATH_LEN); + goto error; + } ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) {
--- src/locking/lock_driver_sanlock.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 0b7c6d5..95f7d61 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -206,7 +206,14 @@ static int virLockManagerSanlockSetupLockspace(void) goto error; } - memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); + if (!virStrcpyStatic(ls.name, + VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Lockspace path '%s' exceeded %d characters"), + VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, + SANLK_PATH_LEN); + goto error; + } ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { -- 1.7.11.7
On 01/18/13 14:50, John Ferlan wrote:
--- src/locking/lock_driver_sanlock.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
<off-topic> Next time please start a new thread with V2 patches. It's easy to overlook them in an existing thread. </off-topic>
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 0b7c6d5..95f7d61 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -206,7 +206,14 @@ static int virLockManagerSanlockSetupLockspace(void) goto error; }
- memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); + if (!virStrcpyStatic(ls.name, + VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Lockspace path '%s' exceeded %d characters"), + VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, + SANLK_PATH_LEN); + goto error; + }
Hm the error message will trigger only if we ever change the length of VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE. I think that it won't ever happen but this might be a good sanity check.
ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) {
If nobody else responds otherwise till tomorrow, I will push the patch as-is. Peter
On 01/22/13 13:42, Peter Krempa wrote:
On 01/18/13 14:50, John Ferlan wrote:
--- src/locking/lock_driver_sanlock.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
If nobody else responds otherwise till tomorrow, I will push the patch as-is.
Pushed.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
--- src/security/security_dac.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deff024..7ef7eb9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -995,7 +995,13 @@ virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return -1; if (secdef->label) - strcpy(seclabel->label, secdef->label); + if (!virStrcpy(seclabel->label, secdef->label, + VIR_SECURITY_LABEL_BUFLEN)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("security label exceeds max %d bytes"), + VIR_SECURITY_LABEL_BUFLEN-1); + return -1; + } return 0; } -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
--- src/security/security_dac.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deff024..7ef7eb9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -995,7 +995,13 @@ virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return -1;
if (secdef->label) - strcpy(seclabel->label, secdef->label); + if (!virStrcpy(seclabel->label, secdef->label, + VIR_SECURITY_LABEL_BUFLEN)) {
I'd rather go for a ignore_value here. AFAIK it's impossible to overflow the allocated buffer here.
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("security label exceeds max %d bytes"), + VIR_SECURITY_LABEL_BUFLEN-1); + return -1; + }
return 0; }
Peter
--- src/security/security_dac.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index deff024..93a0301 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -995,7 +995,8 @@ virSecurityDACGetProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return -1; if (secdef->label) - strcpy(seclabel->label, secdef->label); + ignore_value(virStrcpy(seclabel->label, secdef->label, + VIR_SECURITY_LABEL_BUFLEN)); return 0; } -- 1.7.11.7
This code is not reachable as of commit id: bb85f229. --- src/rpc/virnetserverclient.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 85239dd..e254309 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1360,8 +1360,6 @@ virNetServerClientInitKeepAlive(virNetServerClientPtr client, cleanup: virObjectUnlock(client); - if (ka) - virKeepAliveStop(ka); virObjectUnref(ka); return ret; -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
This code is not reachable as of commit id: bb85f229. --- src/rpc/virnetserverclient.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 85239dd..e254309 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1360,8 +1360,6 @@ virNetServerClientInitKeepAlive(virNetServerClientPtr client,
cleanup: virObjectUnlock(client); - if (ka) - virKeepAliveStop(ka); virObjectUnref(ka);
This unref also doesn't make sense. The cleanup path is reached only when ka is NULL. This allows to optimize the function even more.
return ret;
The code is not reachable as of commit id: bb85f229. Removed virKeepAliveStop() and virObjectUnref() because 'ka' cannot be anything but NULL at the cleanup label. --- src/rpc/virnetserverclient.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 85239dd..40e8481 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1360,9 +1360,6 @@ virNetServerClientInitKeepAlive(virNetServerClientPtr client, cleanup: virObjectUnlock(client); - if (ka) - virKeepAliveStop(ka); - virObjectUnref(ka); return ret; } -- 1.7.11.7
On 01/18/13 15:29, John Ferlan wrote:
The code is not reachable as of commit id: bb85f229. Removed virKeepAliveStop() and virObjectUnref() because 'ka' cannot be anything but NULL at the cleanup label. --- src/rpc/virnetserverclient.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 85239dd..40e8481 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1360,9 +1360,6 @@ virNetServerClientInitKeepAlive(virNetServerClientPtr client,
cleanup: virObjectUnlock(client); - if (ka) - virKeepAliveStop(ka); - virObjectUnref(ka);
return ret; }
ACK with one nit. the asignment ka = NULL just before the cleanup label is dead code now too. I will be pushing this with the following change squashed in: diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 40e8481..af0560e 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1356,7 +1356,6 @@ virNetServerClientInitKeepAlive(virNetServerClientPtr client, virObjectRef(client); client->keepalive = ka; - ka = NULL; cleanup: virObjectUnlock(client); Peter
--- src/parallels/parallels_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index ea193af..1cf66ea 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -514,7 +514,10 @@ parallelsGetNetInfo(virDomainNetDefPtr net, net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; if (virJSONValueObjectHasKey(value, "state")) { - tmp = virJSONValueObjectGetString(value, "state"); + if (!(tmp = virJSONValueObjectGetString(value, "state"))) { + parallelsParseError(); + goto error; + } if STREQ(tmp, "disconnected") net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; } -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
--- src/parallels/parallels_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index ea193af..1cf66ea 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -514,7 +514,10 @@ parallelsGetNetInfo(virDomainNetDefPtr net,
net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; if (virJSONValueObjectHasKey(value, "state")) { - tmp = virJSONValueObjectGetString(value, "state");
Hm calling this without checking should be safe as the virJSONValueObjectHasKey basically guarantees successful return of a value in virJSONValueObjectGetString.
+ if (!(tmp = virJSONValueObjectGetString(value, "state"))) { + parallelsParseError(); + goto error; + } if STREQ(tmp, "disconnected") net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; }
I don't mind doing the change, but I think this is a false positive. I give ACK to this patch, but it shouldn't be a problem to go without it. Peter
On 01/17/2013 01:03 PM, Peter Krempa wrote:
On 01/17/13 20:17, John Ferlan wrote:
--- src/parallels/parallels_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index ea193af..1cf66ea 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -514,7 +514,10 @@ parallelsGetNetInfo(virDomainNetDefPtr net,
net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; if (virJSONValueObjectHasKey(value, "state")) { - tmp = virJSONValueObjectGetString(value, "state");
Hm calling this without checking should be safe as the virJSONValueObjectHasKey basically guarantees successful return of a value in virJSONValueObjectGetString.
+ if (!(tmp = virJSONValueObjectGetString(value, "state"))) { + parallelsParseError(); + goto error; + } if STREQ(tmp, "disconnected")
Another three possible solutions, both shorter, for silencing Coverity would be: 1. Add in a null check (slight speed penalty, though): if (virJSONValueObjectHasKey(value, "state")) { tmp = virJSONValueObjectGetString(value, "state"); if STREQ_NULLABLE(tmp, "disconnected") 2. use ga_assert() to tell Coverity something that we already know: if (virJSONValueObjectHasKey(value, "state")) { tmp = virJSONValueObjectGetString(value, "state"); ga_assert(tmp); if STREQ(tmp, "disconnected") 3. My favorite - use fewer function calls to begin with: if ((tmp = virJSONValueObjectGetString(value, "state")) && STREQ(tmp, "disconnected"))
I give ACK to this patch, but it shouldn't be a problem to go without it.
I think a v2 would be appropriate with a shorter solution. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/parallels/parallels_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index ea193af..1b47246 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -513,10 +513,9 @@ parallelsGetNetInfo(virDomainNetDefPtr net, } net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; - if (virJSONValueObjectHasKey(value, "state")) { - tmp = virJSONValueObjectGetString(value, "state"); - if STREQ(tmp, "disconnected") - net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; + if ((tmp = virJSONValueObjectGetString(value, "state")) && + STREQ(tmp, "disconnected")) { + net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; } return 0; -- 1.7.11.7
--- src/util/virlockspace.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 163404f..fd89598 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -483,6 +483,9 @@ virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace) virJSONValuePtr owners = NULL; size_t i; + if (!child) + goto error; + if (virJSONValueArrayAppend(resources, child) < 0) { virJSONValueFree(child); goto error; -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
--- src/util/virlockspace.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 163404f..fd89598 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -483,6 +483,9 @@ virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace) virJSONValuePtr owners = NULL; size_t i;
+ if (!child) + goto error;
No error is raised if this code path is taken resulting into a strange error message. a virReportOOMError() should be reported here.
+ if (virJSONValueArrayAppend(resources, child) < 0) { virJSONValueFree(child); goto error;
Peter
--- src/util/virlockspace.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 163404f..4ff0f3a 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -459,8 +459,10 @@ virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace) virJSONValuePtr resources; virHashKeyValuePairPtr pairs = NULL, tmp; - if (!object) + if (!object) { + virReportOOMError(); return NULL; + } virMutexLock(&lockspace->lock); @@ -483,6 +485,11 @@ virJSONValuePtr virLockSpacePreExecRestart(virLockSpacePtr lockspace) virJSONValuePtr owners = NULL; size_t i; + if (!child) { + virReportOOMError(); + goto error; + } + if (virJSONValueArrayAppend(resources, child) < 0) { virJSONValueFree(child); goto error; -- 1.7.11.7
--- src/qemu/qemu_hostdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 896ad9b..ba6dadd 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -511,7 +511,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, dev = pciDeviceListGet(pcidevs, i); activeDev = pciDeviceListFind(driver->activePciHostdevs, dev); - pciDeviceSetUsedBy(activeDev, name); + if (activeDev) + pciDeviceSetUsedBy(activeDev, name); } /* Loop 8: Now set the original states for hostdev def */ -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
--- src/qemu/qemu_hostdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 896ad9b..ba6dadd 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -511,7 +511,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, dev = pciDeviceListGet(pcidevs, i); activeDev = pciDeviceListFind(driver->activePciHostdevs, dev);
- pciDeviceSetUsedBy(activeDev, name); + if (activeDev) + pciDeviceSetUsedBy(activeDev, name);
Strange that no-one complained about this. This is either a false positive or nobody used that yet :) ACK Peter
Commit id a994ef2d1 changed the mechanism to store/update the default security label from using disk->seclabels[0] to allocating one on the fly. That change allocated the label, but never saved it. This patch will save the label. --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/security/security_selinux.c | 6 +++--- 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..0b3427f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16064,3 +16064,32 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model) return seclabel; } + +virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model) +{ + virSecurityDeviceLabelDefPtr seclabel = NULL; + + if (VIR_ALLOC(seclabel) < 0) { + virReportOOMError(); + return NULL; + } + + if (model) { + seclabel->model = strdup(model); + if (seclabel->model == NULL) { + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel); + return NULL; + } + } + + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) { + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel); + return NULL; + } + def->seclabels[def->nseclabels - 1] = seclabel; + + return seclabel; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce36003..9770ffb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model); +virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model); + typedef const char* (*virEventActionToStringFunc)(int type); typedef int (*virEventActionFromStringFunc)(const char *type); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b5e1a9a..511e923 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ - if (VIR_ALLOC(disk_seclabel) < 0) { - virReportOOMError(); + disk_seclabel = + virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME); + if (!disk_seclabel) return -1; - } disk_seclabel->norelabel = true; ret = 0; } -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
Commit id a994ef2d1 changed the mechanism to store/update the default security label from using disk->seclabels[0] to allocating one on the fly. That change allocated the label, but never saved it. This patch will save the label. --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/security/security_selinux.c | 6 +++--- 3 files changed, 35 insertions(+), 3 deletions(-)
This patch might be the fix for: https://bugzilla.redhat.com/show_bug.cgi?id=891324 and https://bugzilla.redhat.com/show_bug.cgi?id=891324
On 01/17/13 20:17, John Ferlan wrote:
Commit id a994ef2d1 changed the mechanism to store/update the default security label from using disk->seclabels[0] to allocating one on the fly. That change allocated the label, but never saved it. This patch will save the label. --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 3 +++ src/security/security_selinux.c | 6 +++--- 3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..0b3427f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16064,3 +16064,32 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model)
return seclabel; } + +virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model) +{ + virSecurityDeviceLabelDefPtr seclabel = NULL; + + if (VIR_ALLOC(seclabel) < 0) { + virReportOOMError();
all ...
+ return NULL; + } + + if (model) { + seclabel->model = strdup(model); + if (seclabel->model == NULL) { + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel);
.. these ..
+ return NULL; + } + } + + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) { + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel);
... error paths can be merged under a no_memory label saving a few lines of code. (virSecurityDeviceLabelDefFree handles NULL arguments gracefully)
+ return NULL; + } + def->seclabels[def->nseclabels - 1] = seclabel; + + return seclabel; +} diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce36003..9770ffb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);
+virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model); + typedef const char* (*virEventActionToStringFunc)(int type); typedef int (*virEventActionFromStringFunc)(const char *type);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b5e1a9a..511e923 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ - if (VIR_ALLOC(disk_seclabel) < 0) { - virReportOOMError(); + disk_seclabel = + virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME); + if (!disk_seclabel) return -1; - } disk_seclabel->norelabel = true; ret = 0; }
Otherwise this seems to me be a good approach how to fix it, but I'm not that familiar with the selinux code. Hopefully, if this fix is wrong somebody will chime in on the v2. Peter
Commit id a994ef2d1 changed the mechanism to store/update the default security label from using disk->seclabels[0] to allocating one on the fly. That change allocated the label, but never saved it. This patch will save the label. The new virDomainDiskDefAddSecurityLabelDef() is a copy of the virDomainDefAddSecurityLabelDef(). --- src/conf/domain_conf.c | 51 ++++++++++++++++++++++++++++++----------- src/conf/domain_conf.h | 3 +++ src/security/security_selinux.c | 6 ++--- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..7640af7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16041,26 +16041,51 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model) { virSecurityLabelDefPtr seclabel = NULL; - if (VIR_ALLOC(seclabel) < 0) { - virReportOOMError(); - return NULL; - } + if (VIR_ALLOC(seclabel) < 0) + goto no_memory; if (model) { seclabel->model = strdup(model); - if (seclabel->model == NULL) { - virReportOOMError(); - virSecurityLabelDefFree(seclabel); - return NULL; - } + if (seclabel->model == NULL) + goto no_memory; } - if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) { - virReportOOMError(); - virSecurityLabelDefFree(seclabel); - return NULL; + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) + goto no_memory; + + def->seclabels[def->nseclabels - 1] = seclabel; + + return seclabel; + +no_memory: + virReportOOMError(); + virSecurityLabelDefFree(seclabel); + return NULL; +} + +virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model) +{ + virSecurityDeviceLabelDefPtr seclabel = NULL; + + if (VIR_ALLOC(seclabel) < 0) + goto no_memory; + + if (model) { + seclabel->model = strdup(model); + if (seclabel->model == NULL) + goto no_memory; } + + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) + goto no_memory; + def->seclabels[def->nseclabels - 1] = seclabel; return seclabel; + +no_memory: + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel); + return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce36003..9770ffb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model); +virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model); + typedef const char* (*virEventActionToStringFunc)(int type); typedef int (*virEventActionFromStringFunc)(const char *type); diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b5e1a9a..511e923 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ - if (VIR_ALLOC(disk_seclabel) < 0) { - virReportOOMError(); + disk_seclabel = + virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME); + if (!disk_seclabel) return -1; - } disk_seclabel->norelabel = true; ret = 0; } -- 1.7.11.7
The fix seems correct to me. On Fri, Jan 18, 2013 at 09:34:13AM -0500, John Ferlan wrote:
Commit id a994ef2d1 changed the mechanism to store/update the default security label from using disk->seclabels[0] to allocating one on the fly. That change allocated the label, but never saved it. This patch will save the label. The new virDomainDiskDefAddSecurityLabelDef() is a copy of the virDomainDefAddSecurityLabelDef(). --- src/conf/domain_conf.c | 51 ++++++++++++++++++++++++++++++----------- src/conf/domain_conf.h | 3 +++ src/security/security_selinux.c | 6 ++--- 3 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..7640af7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16041,26 +16041,51 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model) { virSecurityLabelDefPtr seclabel = NULL;
- if (VIR_ALLOC(seclabel) < 0) { - virReportOOMError(); - return NULL; - } + if (VIR_ALLOC(seclabel) < 0) + goto no_memory;
if (model) { seclabel->model = strdup(model); - if (seclabel->model == NULL) { - virReportOOMError(); - virSecurityLabelDefFree(seclabel); - return NULL; - } + if (seclabel->model == NULL) + goto no_memory; }
- if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) { - virReportOOMError(); - virSecurityLabelDefFree(seclabel); - return NULL; + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) + goto no_memory; + + def->seclabels[def->nseclabels - 1] = seclabel; + + return seclabel; + +no_memory: + virReportOOMError(); + virSecurityLabelDefFree(seclabel); + return NULL; +} + +virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model) +{ + virSecurityDeviceLabelDefPtr seclabel = NULL; + + if (VIR_ALLOC(seclabel) < 0) + goto no_memory; + + if (model) { + seclabel->model = strdup(model); + if (seclabel->model == NULL) + goto no_memory; } + + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) + goto no_memory; + def->seclabels[def->nseclabels - 1] = seclabel;
return seclabel; + +no_memory: + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel); + return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce36003..9770ffb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);
+virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model); + typedef const char* (*virEventActionToStringFunc)(int type); typedef int (*virEventActionFromStringFunc)(const char *type);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b5e1a9a..511e923 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ - if (VIR_ALLOC(disk_seclabel) < 0) { - virReportOOMError(); + disk_seclabel = + virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME); + if (!disk_seclabel) return -1; - } disk_seclabel->norelabel = true; ret = 0; } -- 1.7.11.7
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/18/13 15:34, John Ferlan wrote:
Commit id a994ef2d1 changed the mechanism to store/update the default security label from using disk->seclabels[0] to allocating one on the fly. That change allocated the label, but never saved it. This patch will save the label. The new virDomainDiskDefAddSecurityLabelDef() is a copy of the virDomainDefAddSecurityLabelDef(). --- src/conf/domain_conf.c | 51 ++++++++++++++++++++++++++++++----------- src/conf/domain_conf.h | 3 +++ src/security/security_selinux.c | 6 ++--- 3 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b9ba13..7640af7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16041,26 +16041,51 @@ virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model) { virSecurityLabelDefPtr seclabel = NULL;
- if (VIR_ALLOC(seclabel) < 0) { - virReportOOMError(); - return NULL; - } + if (VIR_ALLOC(seclabel) < 0) + goto no_memory;
if (model) { seclabel->model = strdup(model); - if (seclabel->model == NULL) { - virReportOOMError(); - virSecurityLabelDefFree(seclabel); - return NULL; - } + if (seclabel->model == NULL) + goto no_memory; }
- if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) { - virReportOOMError(); - virSecurityLabelDefFree(seclabel); - return NULL; + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) + goto no_memory; + + def->seclabels[def->nseclabels - 1] = seclabel; + + return seclabel; + +no_memory: + virReportOOMError(); + virSecurityLabelDefFree(seclabel); + return NULL; +}
This first part would be better in a separate patch ...
+ +virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model) +{ + virSecurityDeviceLabelDefPtr seclabel = NULL; + + if (VIR_ALLOC(seclabel) < 0) + goto no_memory; + + if (model) { + seclabel->model = strdup(model); + if (seclabel->model == NULL) + goto no_memory; } + + if (VIR_EXPAND_N(def->seclabels, def->nseclabels, 1) < 0) + goto no_memory; + def->seclabels[def->nseclabels - 1] = seclabel;
return seclabel; + +no_memory: + virReportOOMError(); + virSecurityDeviceLabelDefFree(seclabel); + return NULL; }
but I see why you did it.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ce36003..9770ffb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2222,6 +2222,9 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model);
+virSecurityDeviceLabelDefPtr +virDomainDiskDefAddSecurityLabelDef(virDomainDiskDefPtr def, const char *model); + typedef const char* (*virEventActionToStringFunc)(int type); typedef int (*virEventActionFromStringFunc)(const char *type);
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b5e1a9a..511e923 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1095,10 +1095,10 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ - if (VIR_ALLOC(disk_seclabel) < 0) { - virReportOOMError(); + disk_seclabel = + virDomainDiskDefAddSecurityLabelDef(disk, SECURITY_SELINUX_NAME); + if (!disk_seclabel) return -1; - } disk_seclabel->norelabel = true; ret = 0; }
ACK. Peter
Coverity complains that the objectSpec != NULL check was unnecessary because there was no way to get to the label with objectSpec = NULL. --- src/esx/esx_vi.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index 99c1eb1..d84ce19 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -2168,11 +2168,10 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx, /* * Remove values given by the caller from the data structures to prevent * them from being freed by the call to esxVI_PropertyFilterSpec_Free(). + * objectSpec cannot be NULL here. */ - if (objectSpec != NULL) { - objectSpec->obj = NULL; - objectSpec->selectSet = NULL; - } + objectSpec->obj = NULL; + objectSpec->selectSet = NULL; if (propertySpec != NULL) { propertySpec->type = NULL; -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
Coverity complains that the objectSpec != NULL check was unnecessary because there was no way to get to the label with objectSpec = NULL. --- src/esx/esx_vi.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Yep, commit 041aac86 made that code path impossible. ACK Peter
--- src/util/virobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 838b5cd..7f08a11 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -74,7 +74,7 @@ VIR_ONCE_GLOBAL_INIT(virObject); */ virClassPtr virClassForObject(void) { - if (!virObjectInitialize() < 0) + if (virObjectInitialize() < 0) return NULL; return virObjectClass; @@ -88,7 +88,7 @@ virClassPtr virClassForObject(void) */ virClassPtr virClassForObjectLockable(void) { - if (!virObjectInitialize() < 0) + if (virObjectInitialize() < 0) return NULL; return virObjectLockableClass; -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
--- src/util/virobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 838b5cd..7f08a11 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -74,7 +74,7 @@ VIR_ONCE_GLOBAL_INIT(virObject); */ virClassPtr virClassForObject(void) { - if (!virObjectInitialize() < 0) + if (virObjectInitialize() < 0)
Funny bug :) either of those works, but not together. ACK Peter
The fetch of 'ipdef' in networkRefreshDhcpDaemon() when the loop to fill in ipv4def fails to find an ipv4 address with dhcp defined. The filled in ipdef value was not used. Code was made unnecessary with commit it 2d5cd1. --- src/network/bridge_driver.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6f3c839..268dada 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1156,9 +1156,6 @@ networkRefreshDhcpDaemon(struct network_driver *driver, if (!ipv4def && (ipdef->nranges || ipdef->nhosts)) ipv4def = ipdef; } - /* If no IPv4 addresses had dhcp info, pick the first (if there were any). */ - if (!ipdef) - ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, 0); ipv6def = NULL; for (ii = 0; -- 1.7.11.7
On 01/17/13 20:17, John Ferlan wrote:
The fetch of 'ipdef' in networkRefreshDhcpDaemon() when the loop to fill in ipv4def fails to find an ipv4 address with dhcp defined. The filled in ipdef value was not used. Code was made unnecessary with commit it 2d5cd1. --- src/network/bridge_driver.c | 3 --- 1 file changed, 3 deletions(-)
ACK. I will push the patch later. Peter
I pushed patches 6,8,9 and 10 and I will review patch 7 tomorrow. The others will require a V2. Peter
participants (4)
-
Eric Blake -
John Ferlan -
Marcelo Cerri -
Peter Krempa