4:24 a.m.
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
4:24 a.m.
New subject: [libvirt] [PATCH 1/3] uuid: Fix coverity warning of unchecked return value
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
4:24 a.m.
New subject: [libvirt] [PATCH 2/3] vbox: snapshot: Avoid memleak in virVBoxSnapshotConfAllChildren
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
4:49 a.m.
New subject: [libvirt] [PATCH 2/3] vbox: snapshot: Avoid memleak in virVBoxSnapshotConfAllChildren
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
4:24 a.m.
New subject: [libvirt] [PATCH 3/3] vbox: snapshot: Avoid memleaks in functions dealing with disk arrays
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
4:53 a.m.
New subject: [libvirt] [PATCH 3/3] vbox: snapshot: Avoid memleaks in functions dealing with disk arrays
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
4:42 a.m.
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
6:18 a.m.
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
3811
days inactive
3811
days old
7 comments
3 participants
participants (3)
-
Ján Tomko -
Peter Krempa -
Yohan Belleguic