[libvirt] [PATCH 0/2] Fix some issues in storage driver error paths
While working through the pool driver, I found a couple of issues in error paths for the storage driver. John Ferlan (2): storage: Fix error path in virStoragePoolObjLoad storage: Fix error path in storagePoolDefineXML src/conf/storage_conf.c | 4 ++-- src/storage/storage_driver.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) -- 2.5.0
While reviewing how storage driver used ObjListPtr's for reference in some recent secret driver patches to use the same mechanism, I came across an instance where the wrong API was called for error paths after successfully allocating the storage pool pointer and inserting into the driver pool list. The path is after virStoragePoolObjAssignDef succeeds - the 'def' passed in is assigned to pool->def (or newDef) so it shouldn't be the only thing deleted. The pool is now part of driver->pools.objs, so it would need to be removed (as happens in the storagePoolCreateXML error paths). Rather than calling virStoragePoolDefFree to free the def which is now assigned to the pool, call virStoragePoolObjRemove to ensure the pool element is removed from the driver list and that anything stored in pool is properly handled by virStoragePoolObjFree including the call to virStoragePoolDefFree for the pool->{def|newDef} element. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 3657dfd..bfba521 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1852,12 +1852,12 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, VIR_FREE(pool->configFile); /* for driver reload */ if (VIR_STRDUP(pool->configFile, path) < 0) { - virStoragePoolDefFree(def); + virStoragePoolObjRemove(pools, pool); return NULL; } VIR_FREE(pool->autostartLink); /* for driver reload */ if (VIR_STRDUP(pool->autostartLink, autostartLink) < 0) { - virStoragePoolDefFree(def); + virStoragePoolObjRemove(pools, pool); return NULL; } -- 2.5.0
Found by inspection - after calling virStoragePoolObjAssignDef the pool is part of the driver->pools.objs list and the failure path for the virStoragePoolObjSaveDef will use virStoragePoolObjRemove to remove the pool from the objs list which will unlock and free the pool pointer (as pools->objs[i] during the loop). Since the call doesn't clear the pool address from the callee, we need to set it to NULL; otherwise, the virStoragePoolObjUnlock in the cleanup: code will fail miserably. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e0ded01..1d96618 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -784,6 +784,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStoragePoolObjSaveDef(driver, pool, def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); def = NULL; + pool = NULL; goto cleanup; } def = NULL; -- 2.5.0
On Thu, Feb 25, 2016 at 04:02:39PM -0500, John Ferlan wrote:
While working through the pool driver, I found a couple of issues in error paths for the storage driver.
John Ferlan (2): storage: Fix error path in virStoragePoolObjLoad storage: Fix error path in storagePoolDefineXML
src/conf/storage_conf.c | 4 ++-- src/storage/storage_driver.c | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)
ACK to both. Jan
participants (2)
-
John Ferlan -
Ján Tomko