4:08 a.m.
Osier Yang (2):
storage: Fix coverity warning
storage: Fix the use-after-free memory bug
src/storage/storage_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
1.8.1.4
4:08 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Fix coverity warning
Introduced by commit e0139e30444:
1777 /* Updating pool metadata */
(40) Event var_deref_op: Dereferencing null pointer "newvol".
Also see events: [assign_zero]
1778 pool->def->allocation += newvol->allocation;
1779 pool->def->available -= newvol->allocation;
---
src/storage/storage_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 7908ba6..63a954b 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1758,7 +1758,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
origvol->building = 0;
newvol->building = 0;
- newvol = NULL;
pool->asyncjobs--;
if (origpool) {
@@ -1781,6 +1780,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
VIR_INFO("Creating volume '%s' in storage pool '%s'",
volobj->name, pool->def->name);
ret = volobj;
+ newvol = NULL;
volobj = NULL;
cleanup:
--
1.8.1.4
9:43 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Fix coverity warning
On 08/20/2013 05:08 AM, Osier Yang wrote:
Introduced by commit e0139e30444:
1777 /* Updating pool metadata */
(40) Event var_deref_op: Dereferencing null pointer "newvol".
Also see events: [assign_zero]
1778 pool->def->allocation += newvol->allocation;
1779 pool->def->available -= newvol->allocation;
---
src/storage/storage_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 7908ba6..63a954b 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1758,7 +1758,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
origvol->building = 0;
newvol->building = 0;
- newvol = NULL;
pool->asyncjobs--;
if (origpool) {
...
The next condition is:
if (buildret < 0) {
virStoragePoolObjUnlock(pool);
storageVolDelete(volobj, 0);
pool = NULL;
goto cleanup;
}
Since previously we'd have 'newvol = NULL;' already, there would need to
be one added here too.. Since, prior to this there's code:
pool->volumes.objs[pool->volumes.count++] = newvol;
which saves the pointer...
Perhaps it'd work better to do the following:
unsigned long long allocation = 0x0ULL;
...
allocation = newvol->allocation;
newvol = NULL;
...
pool->def->allocation += allocation;
pool->def->available -= allocation;
@@ -1781,6 +1780,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
VIR_INFO("Creating volume '%s' in storage pool '%s'",
volobj->name, pool->def->name);
ret = volobj;
+ newvol = NULL;
and this would become unnecessary
volobj = NULL;
cleanup:
10:28 a.m.
New subject: [libvirt] [PATCH v2] storage: Fix coverity warning
Introduced by commit e0139e30444:
1777 /* Updating pool metadata */
(40) Event var_deref_op: Dereferencing null pointer "newvol".
Also see events: [assign_zero]
1778 pool->def->allocation += newvol->allocation;
1779 pool->def->available -= newvol->allocation;
---
src/storage/storage_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 7908ba6..58d0fc0 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1635,6 +1635,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
virStorageBackendPtr backend;
virStorageVolDefPtr origvol = NULL, newvol = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
+ unsigned long long allocation;
int buildret;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1758,6 +1759,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
origvol->building = 0;
newvol->building = 0;
+ allocation = newvol->allocation;
newvol = NULL;
pool->asyncjobs--;
@@ -1775,8 +1777,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
}
/* Updating pool metadata */
- pool->def->allocation += newvol->allocation;
- pool->def->available -= newvol->allocation;
+ pool->def->allocation += allocation;
+ pool->def->available -= allocation;
VIR_INFO("Creating volume '%s' in storage pool '%s'",
volobj->name, pool->def->name);
--
1.8.1.4
10:38 a.m.
New subject: [libvirt] [PATCH v2] storage: Fix coverity warning
On 08/20/2013 09:28 AM, Osier Yang wrote:
Introduced by commit e0139e30444:
1777 /* Updating pool metadata */
(40) Event var_deref_op: Dereferencing null pointer "newvol".
Also see events: [assign_zero]
1778 pool->def->allocation += newvol->allocation;
1779 pool->def->available -= newvol->allocation;
---
src/storage/storage_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
ACK - this turned out cleaner.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:08 a.m.
New subject: [libvirt] [PATCH 2/2] storage: Fix the use-after-free memory bug
Introduced by commit e0139e30444. virStorageVolDefFree free'ed the
pointers that are still used by the added volume object, this changes
it back to VIR_FREE.
---
src/storage/storage_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 63a954b..883e4e9 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1618,7 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
cleanup:
virObjectUnref(volobj);
virStorageVolDefFree(voldef);
- virStorageVolDefFree(buildvoldef);
+ VIR_FREE(buildvoldef);
if (pool)
virStoragePoolObjUnlock(pool);
return ret;
--
1.8.1.4
9:57 a.m.
New subject: [libvirt] [PATCH 2/2] storage: Fix the use-after-free memory bug
On 08/20/2013 05:08 AM, Osier Yang wrote:
Introduced by commit e0139e30444. virStorageVolDefFree free'ed
the
pointers that are still used by the added volume object, this changes
it back to VIR_FREE.
---
src/storage/storage_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 63a954b..883e4e9 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1618,7 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
cleanup:
virObjectUnref(volobj);
virStorageVolDefFree(voldef);
- virStorageVolDefFree(buildvoldef);
+ VIR_FREE(buildvoldef);
if (pool)
virStoragePoolObjUnlock(pool);
return ret;
Perhaps a comment "/* Free just the shallow copy of buildvoldef */".
Just for the clarity of why you wouldn't want to use virStorageVolDefFree().
Of course the same possible other solution applies here as well as it
did in your 1/2 patch where you define a local allocation variable to
handle the pool->def->{allocation|available} math. That way buildvoldef
moves back inside the "if (backend->buildVol)"...
ACK either way though.
John
4111
days inactive
4112
days old
7 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Osier Yang