[libvirt] [PATCH] Fix pool define crash
There's a null dereference in the storage driver when defining a pool. Attached patch fixes it for me. Thanks, Cole
Cole Robinson wrote:
There's a null dereference in the storage driver when defining a pool. Attached patch fixes it for me.
diff --git a/src/storage_driver.c b/src/storage_driver.c index 2432a9a..ac5e443 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -546,7 +546,7 @@ storagePoolDefine(virConnectPtr conn, goto cleanup; def = NULL;
- if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) { + if (virStoragePoolObjSaveDef(conn, driver, pool, pool->def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); goto cleanup; }
Hm, I definitely see what you are getting at, and why the crash is there, but I'm not sure this is totally correct. virStoragePoolObjAssignDef() only assigns this def to pool->def iff the storage pool is not running. That means that if the pool is running, and we make this change, for running pools you would always get the old def, not the updated one. I think we need to move the "def = NULL" further down, but that of course will require other changes so we still have the unified cleanup target. -- Chris Lalancette
Chris Lalancette wrote:
Cole Robinson wrote:
There's a null dereference in the storage driver when defining a pool. Attached patch fixes it for me.
diff --git a/src/storage_driver.c b/src/storage_driver.c index 2432a9a..ac5e443 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -546,7 +546,7 @@ storagePoolDefine(virConnectPtr conn, goto cleanup; def = NULL;
- if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) { + if (virStoragePoolObjSaveDef(conn, driver, pool, pool->def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); goto cleanup; }
Hm, I definitely see what you are getting at, and why the crash is there, but I'm not sure this is totally correct. virStoragePoolObjAssignDef() only assigns this def to pool->def iff the storage pool is not running. That means that if the pool is running, and we make this change, for running pools you would always get the old def, not the updated one. I think we need to move the "def = NULL" further down, but that of course will require other changes so we still have the unified cleanup target.
Ahh, I didn't realize the AssignDef caveat. Thanks for pointing that out. Something like this should work then? - Cole
Cole Robinson wrote:
Ahh, I didn't realize the AssignDef caveat. Thanks for pointing that out. Something like this should work then?
diff --git a/src/storage_driver.c b/src/storage_driver.c index 2432a9a..330317c 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -544,12 +544,13 @@ storagePoolDefine(virConnectPtr conn,
if (!(pool = virStoragePoolObjAssignDef(conn, &driver->pools, def))) goto cleanup; - def = NULL;
if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); + def = NULL; goto cleanup; } + def = NULL;
ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid);
Yeah, that looks better. Seems to be good to me, so ACK. -- Chris Lalancette
On Wed, Dec 17, 2008 at 09:26:51PM +0100, Chris Lalancette wrote:
Cole Robinson wrote:
Ahh, I didn't realize the AssignDef caveat. Thanks for pointing that out. Something like this should work then?
diff --git a/src/storage_driver.c b/src/storage_driver.c index 2432a9a..330317c 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -544,12 +544,13 @@ storagePoolDefine(virConnectPtr conn,
if (!(pool = virStoragePoolObjAssignDef(conn, &driver->pools, def))) goto cleanup; - def = NULL;
if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); + def = NULL; goto cleanup; } + def = NULL;
ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid);
Yeah, that looks better. Seems to be good to me, so ACK.
Functionally correct, so fine to commit, but I still think we should change the virStoragePoolObjSaveDef method params, but not urgent unless you want to change it now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Wed, Dec 17, 2008 at 09:26:51PM +0100, Chris Lalancette wrote:
Cole Robinson wrote:
Ahh, I didn't realize the AssignDef caveat. Thanks for pointing that out. Something like this should work then?
diff --git a/src/storage_driver.c b/src/storage_driver.c index 2432a9a..330317c 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -544,12 +544,13 @@ storagePoolDefine(virConnectPtr conn,
if (!(pool = virStoragePoolObjAssignDef(conn, &driver->pools, def))) goto cleanup; - def = NULL;
if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); + def = NULL; goto cleanup; } + def = NULL;
ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid);
Yeah, that looks better. Seems to be good to me, so ACK.
Functionally correct, so fine to commit, but I still think we should change the virStoragePoolObjSaveDef method params, but not urgent unless you want to change it now.
Daniel
I opted just to commit the above fix. We can address the API issue later. Thanks, Cole
On Wed, Dec 17, 2008 at 09:00:11PM +0100, Chris Lalancette wrote:
Cole Robinson wrote:
There's a null dereference in the storage driver when defining a pool. Attached patch fixes it for me.
diff --git a/src/storage_driver.c b/src/storage_driver.c index 2432a9a..ac5e443 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -546,7 +546,7 @@ storagePoolDefine(virConnectPtr conn, goto cleanup; def = NULL;
- if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) { + if (virStoragePoolObjSaveDef(conn, driver, pool, pool->def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); goto cleanup; }
Hm, I definitely see what you are getting at, and why the crash is there, but I'm not sure this is totally correct. virStoragePoolObjAssignDef() only assigns this def to pool->def iff the storage pool is not running. That means that if the pool is running, and we make this change, for running pools you would always get the old def, not the updated one. I think we need to move the "def = NULL" further down, but that of course will require other changes so we still have the unified cleanup target.
Opps, yes you are correct. IMHO, the API contract here is the problem Instead of being given a 'virStoragePoolDefPtr' instance, it should be given the parent 'virStoragePoolObjPtr', along with a flag indicating whether to save the live or inactive config. That way the lookup of the correct 'def' is in one place, not in all callers, and it makes the API contract clear Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Cole Robinson <crobinso@redhat.com> wrote:
There's a null dereference in the storage driver when defining a pool. Attached patch fixes it for me.
Thanks, Cole diff --git a/src/storage_driver.c b/src/storage_driver.c index 2432a9a..ac5e443 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -546,7 +546,7 @@ storagePoolDefine(virConnectPtr conn, goto cleanup; def = NULL;
- if (virStoragePoolObjSaveDef(conn, driver, pool, def) < 0) { + if (virStoragePoolObjSaveDef(conn, driver, pool, pool->def) < 0) { virStoragePoolObjRemove(&driver->pools, pool); goto cleanup; }
Looks right, and passes this test: qemud/libvirtd & sleep 1 src/virsh --connect qemu:///session pool-define-as b dir c d e /f j src/virsh --connect qemu:///session pool-dumpxml b Whereas before the patch, running pool-define-as would cause libvirtd to segfault. So ACK.
On Wed, Dec 17, 2008 at 01:24:14PM -0500, Cole Robinson wrote:
There's a null dereference in the storage driver when defining a pool. Attached patch fixes it for me.
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Chris Lalancette -
Cole Robinson -
Daniel P. Berrange -
Jim Meyering