"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
This is not safe in general, because after the 'AssignDef' call, the 'def' is now owned by the 'obj'. If you don't set it to NULL immediately you have the risk of later error cleanup paths, seeing the non-NULL def and free'ing it when they shouldn't.
The virGetNetwork() calls should be changed to call 'obj->def->name' instead of just 'def->name'.
Ok. that's safer, so I've adjusted the first two fixes. The latter two functions weren't broken, but I've adjusted the formatting to be consistent with single-stmt-no-curly-braces convention. In these multiple-1000's-of-line files, every little bit helps.
From baf2ec1ee6246f9e472b67d522676b8d41534525 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Fri, 13 Feb 2009 16:49:50 +0100 Subject: [PATCH] test:///default driver: don't dereference NULL "def"
* src/test.c (testNetworkCreate, testNetworkDefine): Since "def" is set to NULL immediately after any vir*AssignDef call (to indicate we no longer own it and to ensure no clean-up path mistakenly frees it), dereference via net->def->, not def->. --- src/test.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/test.c b/src/test.c index 0e0ec7c..226fe2e 100644 --- a/src/test.c +++ b/src/test.c @@ -2049,14 +2049,12 @@ static virNetworkPtr testNetworkCreate(virConnectPtr conn, const char *xml) { if ((def = virNetworkDefParseString(conn, xml)) == NULL) goto cleanup; - if ((net = virNetworkAssignDef(conn, &privconn->networks, - def)) == NULL) { + if ((net = virNetworkAssignDef(conn, &privconn->networks, def)) == NULL) goto cleanup; - } - net->active = 1; def = NULL; + net->active = 1; - ret = virGetNetwork(conn, def->name, def->uuid); + ret = virGetNetwork(conn, net->def->name, net->def->uuid); cleanup: virNetworkDefFree(def); @@ -2076,14 +2074,12 @@ static virNetworkPtr testNetworkDefine(virConnectPtr conn, const char *xml) { if ((def = virNetworkDefParseString(conn, xml)) == NULL) goto cleanup; - if ((net = virNetworkAssignDef(conn, &privconn->networks, - def)) == NULL) { + if ((net = virNetworkAssignDef(conn, &privconn->networks, def)) == NULL) goto cleanup; - } - net->persistent = 1; def = NULL; + net->persistent = 1; - ret = virGetNetwork(conn, def->name, def->uuid); + ret = virGetNetwork(conn, net->def->name, net->def->uuid); cleanup: virNetworkDefFree(def); @@ -2529,9 +2525,8 @@ testStoragePoolCreate(virConnectPtr conn, goto cleanup; } - if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) { + if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) goto cleanup; - } def = NULL; if (testStoragePoolObjSetDefaults(conn, pool) == -1) { @@ -2568,9 +2563,8 @@ testStoragePoolDefine(virConnectPtr conn, def->allocation = defaultPoolAlloc; def->available = defaultPoolCap - defaultPoolAlloc; - if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) { + if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) goto cleanup; - } def = NULL; if (testStoragePoolObjSetDefaults(conn, pool) == -1) { -- 1.6.2.rc0.234.g2cc0b3