[libvirt] [PATCHv2 0/2] vbox: Fix various coverity issues.
Version 2 fixes issues pointed out by Yohan Belleguic. Peter Krempa (2): vbox: snapshot: Avoid memleak in virVBoxSnapshotConfAllChildren vbox: snapshot: Avoid memleaks in functions dealing with disk arrays src/vbox/vbox_snapshot_conf.c | 56 ++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 14 deletions(-) -- 1.9.3
On re-allocation failure the function would leak already allocated memory. --- src/vbox/vbox_snapshot_conf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 30ac6fe..ced0952 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -477,24 +477,32 @@ virVBoxSnapshotConfAllChildren(virVBoxSnapshotConfHardDiskPtr disk, virVBoxSnapshotConfHardDiskPtr *tempList = NULL; size_t i = 0; size_t j = 0; + if (VIR_ALLOC_N(ret, 0) < 0) return 0; 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++) { + for (j = 0; j < tempSize; j++) ret[returnSize - tempSize + j] = tempList[j]; - } + + VIR_FREE(tempList); } + if (VIR_EXPAND_N(ret, returnSize, 1) < 0) - return 0; + goto error; ret[returnSize - 1] = disk; *list = ret; return returnSize; + + error: + VIR_FREE(tempList); + VIR_FREE(ret); + return 0; } void -- 1.9.3
On 17.6.2014 13:31, Peter Krempa wrote:
On re-allocation failure the function would leak already allocated memory. --- src/vbox/vbox_snapshot_conf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
ACK Pavel
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 | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index ced0952..3f7ad78 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -1473,28 +1473,38 @@ 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: + VIR_FREE(diskList); + VIR_FREE(tempList); + return ret; } @@ -1515,24 +1525,34 @@ 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: + VIR_FREE(diskList); + VIR_FREE(tempList); + return ret; } -- 1.9.3
On 17.6.2014 13:32, Peter Krempa wrote:
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 | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-)
ACK Pavel
On 06/17/14 16:33, Pavel Hrdina wrote:
On 17.6.2014 13:32, Peter Krempa wrote:
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 | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-)
ACK
Pushed; Thanks. Peter
participants (2)
-
Pavel Hrdina -
Peter Krempa