[libvirt] [PATCH 0/3] Fix few issues pointed out by coverity
Peter Krempa (3): uuid: Fix coverity warning of unchecked return value vbox: snapshot: Avoid memleak in virVBoxSnapshotConfAllChildren vbox: snapshot: Avoid memleaks in functions dealing with disk arrays src/vbox/vbox_snapshot_conf.c | 64 +++++++++++++++++++++++++++++++++++-------- tests/qemuxml2argvtest.c | 4 ++- 2 files changed, 55 insertions(+), 13 deletions(-) -- 1.9.3
Coverity checks for patterns of handling return values of functions. Some recent addition must have tripped a threshold where coverity now complains that we usually check the return value of virUUIDGenerate but don't do it in one place. Add a check to make coverity happy. --- tests/qemuxml2argvtest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 24d104e..d8782d8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -51,7 +51,9 @@ fakeSecretLookupByUsage(virConnectPtr conn, if (STRNEQ(usageID, "mycluster_myname")) return NULL; - virUUIDGenerate(uuid); + if (virUUIDGenerate(uuid) < 0) + return NULL; + return virGetSecret(conn, uuid, usageType, usageID); } -- 1.9.3
On re-allocation failure the function would leak already allocated memory. --- src/vbox/vbox_snapshot_conf.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 30ac6fe..69c7e42 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -483,18 +483,24 @@ virVBoxSnapshotConfAllChildren(virVBoxSnapshotConfHardDiskPtr disk, for (i = 0; i < disk->nchildren; i++) { tempSize = virVBoxSnapshotConfAllChildren(disk->children[i], &tempList); if (VIR_EXPAND_N(ret, returnSize, tempSize) < 0) - return 0; + goto error; for (j = 0; j < tempSize; j++) { ret[returnSize - tempSize + j] = tempList[j]; } } if (VIR_EXPAND_N(ret, returnSize, 1) < 0) - return 0; + goto error; ret[returnSize - 1] = disk; *list = ret; return returnSize; + + error: + for (i = 0; i < returnSize; i++) + virVboxSnapshotConfHardDiskFree(ret[i]); + VIR_FREE(ret); + return 0; } void -- 1.9.3
Le Tuesday 17 June 2014 11:24:08, Peter Krempa a écrit :
On re-allocation failure the function would leak already allocated memory. --- src/vbox/vbox_snapshot_conf.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 30ac6fe..69c7e42 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -483,18 +483,24 @@ virVBoxSnapshotConfAllChildren(virVBoxSnapshotConfHardDiskPtr disk, for (i = 0; i < disk->nchildren; i++) { tempSize = virVBoxSnapshotConfAllChildren(disk->children[i], &tempList); if (VIR_EXPAND_N(ret, returnSize, tempSize) < 0) - return 0; + goto error;
for (j = 0; j < tempSize; j++) { ret[returnSize - tempSize + j] = tempList[j]; } } if (VIR_EXPAND_N(ret, returnSize, 1) < 0) - return 0; + goto error;
ret[returnSize - 1] = disk; *list = ret; return returnSize; + + error: + for (i = 0; i < returnSize; i++) + virVboxSnapshotConfHardDiskFree(ret[i]); + VIR_FREE(ret); + return 0; }
I think we don't have to use virVboxSnapshotConfHardDiskFree because there is not memory allocation in this function. It's just a pointer recopy, so if the disks are freed, this might lead to a segfault or a bad behaviour.
void
-- Yohan BELLEGUIC Software Engineer - diateam : Architectes de l'information Phone : +33 (0)2 98 050 050 Fax : +33 (0)2 98 050 05
In virVBoxSnapshotConfRemoveFakeDisks and virVBoxSnapshotConfDiskIsInMediaRegistry the disk array constructed from all the disks would be leaked at the end of the function and on allocation errors. Also the temporary disk list would be leaked. Add a cleanup section and free the memory properly. Found by coverity. --- src/vbox/vbox_snapshot_conf.c | 54 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 69c7e42..49fd2b6 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -1471,28 +1471,45 @@ virVBoxSnapshotConfRemoveFakeDisks(virVBoxSnapshotConfMachinePtr machine) size_t diskSize = 0; virVBoxSnapshotConfHardDiskPtr *tempList = NULL; virVBoxSnapshotConfHardDiskPtr *diskList = NULL; + if (VIR_ALLOC_N(diskList, 0) < 0) - return ret; + return -1; for (i = 0; i < machine->mediaRegistry->ndisks; i++) { tempSize = virVBoxSnapshotConfAllChildren(machine->mediaRegistry->disks[i], &tempList); if (VIR_EXPAND_N(diskList, diskSize, tempSize) < 0) - return ret; - for (j = 0; j < tempSize; j++) { + goto cleanup; + + for (j = 0; j < tempSize; j++) diskList[diskSize - tempSize + j] = tempList[j]; - } + + VIR_FREE(tempList); } + for (i = 0; i < diskSize; i++) { if (strstr(diskList[i]->location, "fake") != NULL) { if (virVBoxSnapshotConfRemoveHardDisk(machine->mediaRegistry, diskList[i]->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to remove hard disk %s from media registry"), diskList[i]->location); - return ret; + goto cleanup; } } } + ret = 0; + + cleanup: + for (i = 0; i < diskSize; i++) + virVboxSnapshotConfHardDiskFree(diskList[i]); + VIR_FREE(diskList); + + if (tempList) { + for (i = 0; i < tempSize; i++) + virVboxSnapshotConfHardDiskFree(tempList[i]); + VIR_FREE(tempList); + } + return ret; } @@ -1513,24 +1530,41 @@ virVBoxSnapshotConfDiskIsInMediaRegistry(virVBoxSnapshotConfMachinePtr machine, size_t diskSize = 0; virVBoxSnapshotConfHardDiskPtr *tempList = NULL; virVBoxSnapshotConfHardDiskPtr *diskList = NULL; + if (VIR_ALLOC_N(diskList, 0) < 0) - return ret; + return -1; for (i = 0; i < machine->mediaRegistry->ndisks; i++) { tempSize = virVBoxSnapshotConfAllChildren(machine->mediaRegistry->disks[i], &tempList); if (VIR_EXPAND_N(diskList, diskSize, tempSize) < 0) - return ret; - for (j = 0; j < tempSize; j++) { + goto cleanup; + + for (j = 0; j < tempSize; j++) diskList[diskSize - tempSize + j] = tempList[j]; - } + + VIR_FREE(tempList); } + for (i = 0; i < diskSize; i++) { if (STREQ(diskList[i]->location, location)) { ret = 1; - return ret; + goto cleanup; } } + ret = 0; + + cleanup: + for (i = 0; i < diskSize; i++) + virVboxSnapshotConfHardDiskFree(diskList[i]); + VIR_FREE(diskList); + + if (tempList) { + for (i = 0; i < tempSize; i++) + virVboxSnapshotConfHardDiskFree(tempList[i]); + VIR_FREE(tempList); + } + return ret; } -- 1.9.3
Le Tuesday 17 June 2014 11:24:09, Peter Krempa a écrit :
In virVBoxSnapshotConfRemoveFakeDisks and virVBoxSnapshotConfDiskIsInMediaRegistry the disk array constructed from all the disks would be leaked at the end of the function and on allocation errors. Also the temporary disk list would be leaked.
Add a cleanup section and free the memory properly.
Found by coverity. --- src/vbox/vbox_snapshot_conf.c | 54 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 44 insertions(+), 10 deletions(-)
diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 69c7e42..49fd2b6 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -1471,28 +1471,45 @@ virVBoxSnapshotConfRemoveFakeDisks(virVBoxSnapshotConfMachinePtr machine) size_t diskSize = 0; virVBoxSnapshotConfHardDiskPtr *tempList = NULL; virVBoxSnapshotConfHardDiskPtr *diskList = NULL; + if (VIR_ALLOC_N(diskList, 0) < 0) - return ret; + return -1;
for (i = 0; i < machine->mediaRegistry->ndisks; i++) { tempSize = virVBoxSnapshotConfAllChildren(machine->mediaRegistry->disks[i], &tempList); if (VIR_EXPAND_N(diskList, diskSize, tempSize) < 0) - return ret; - for (j = 0; j < tempSize; j++) { + goto cleanup; + + for (j = 0; j < tempSize; j++) diskList[diskSize - tempSize + j] = tempList[j]; - } + + VIR_FREE(tempList); } + for (i = 0; i < diskSize; i++) { if (strstr(diskList[i]->location, "fake") != NULL) { if (virVBoxSnapshotConfRemoveHardDisk(machine->mediaRegistry, diskList[i]->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to remove hard disk %s from media registry"), diskList[i]->location); - return ret; + goto cleanup; } } } + ret = 0; + + cleanup: + for (i = 0; i < diskSize; i++) + virVboxSnapshotConfHardDiskFree(diskList[i]); + VIR_FREE(diskList); + + if (tempList) { + for (i = 0; i < tempSize; i++) + virVboxSnapshotConfHardDiskFree(tempList[i]); + VIR_FREE(tempList); + } Same comments than the previous patch + return ret; }
@@ -1513,24 +1530,41 @@ virVBoxSnapshotConfDiskIsInMediaRegistry(virVBoxSnapshotConfMachinePtr machine, size_t diskSize = 0; virVBoxSnapshotConfHardDiskPtr *tempList = NULL; virVBoxSnapshotConfHardDiskPtr *diskList = NULL; + if (VIR_ALLOC_N(diskList, 0) < 0) - return ret; + return -1;
for (i = 0; i < machine->mediaRegistry->ndisks; i++) { tempSize = virVBoxSnapshotConfAllChildren(machine->mediaRegistry->disks[i], &tempList); if (VIR_EXPAND_N(diskList, diskSize, tempSize) < 0) - return ret; - for (j = 0; j < tempSize; j++) { + goto cleanup; + + for (j = 0; j < tempSize; j++) diskList[diskSize - tempSize + j] = tempList[j]; - } + + VIR_FREE(tempList); } + for (i = 0; i < diskSize; i++) { if (STREQ(diskList[i]->location, location)) { ret = 1; - return ret; + goto cleanup; } } + ret = 0; + + cleanup: + for (i = 0; i < diskSize; i++) + virVboxSnapshotConfHardDiskFree(diskList[i]); + VIR_FREE(diskList); + + if (tempList) { + for (i = 0; i < tempSize; i++) + virVboxSnapshotConfHardDiskFree(tempList[i]); + VIR_FREE(tempList); + } Same comments than the previous patch + return ret; }
-- Yohan BELLEGUIC Software Engineer - diateam : Architectes de l'information Phone : +33 (0)2 98 050 050 Fax : +33 (0)2 98 050 05
On 06/17/2014 11:24 AM, Peter Krempa wrote:
Peter Krempa (3): uuid: Fix coverity warning of unchecked return value vbox: snapshot: Avoid memleak in virVBoxSnapshotConfAllChildren vbox: snapshot: Avoid memleaks in functions dealing with disk arrays
src/vbox/vbox_snapshot_conf.c | 64 +++++++++++++++++++++++++++++++++++-------- tests/qemuxml2argvtest.c | 4 ++- 2 files changed, 55 insertions(+), 13 deletions(-)
ACK
On 06/17/14 11:42, Ján Tomko wrote:
On 06/17/2014 11:24 AM, Peter Krempa wrote:
Peter Krempa (3): uuid: Fix coverity warning of unchecked return value vbox: snapshot: Avoid memleak in virVBoxSnapshotConfAllChildren vbox: snapshot: Avoid memleaks in functions dealing with disk arrays
src/vbox/vbox_snapshot_conf.c | 64 +++++++++++++++++++++++++++++++++++-------- tests/qemuxml2argvtest.c | 4 ++- 2 files changed, 55 insertions(+), 13 deletions(-)
ACK
I've pushed 1/3 and will repost the rest due to the issues pointed out. Peter
participants (3)
-
Ján Tomko -
Peter Krempa -
Yohan Belleguic