Re: [libvirt] [PATCH] Fix virsh {net,pool}-edit

[Jim Meyering <jim@xxxxxxxxxxxx>]

"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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@xxxxxxxxxx>
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

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list