[libvirt] [PATCH] Fix uninitialized variable & error reporting in LXC veth setup

[Daniel Veillard]

On Mon, Mar 21, 2011 at 03:36:30PM +0000, Daniel P. Berrange wrote:
> On Thu, Mar 17, 2011 at 12:46:55PM -0400, Laine Stump wrote:
> > On 03/17/2011 11:57 AM, Daniel P. Berrange wrote:
> > >THe veth setup in LXC had a couple of flaws, first brInit did
> > >not report any error when it failed. Second vethCreate() did
> > >not correctly initialize the variable containing the return
> > >code, so could report failure even when it succeeded.
> > >
> > >* src/lxc/lxc_driver.c: Report error when brInit fails
> > >* src/lxc/veth.c: Fix uninitialized variable
> > >---
> > >  src/lxc/lxc_driver.c |    8 ++++++--
> > >  src/lxc/veth.c       |   17 +++++++++--------
> > >  2 files changed, 15 insertions(+), 10 deletions(-)
> > >
> > >diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> > >index 79b6879..9b131cc 100644
> > >--- a/src/lxc/lxc_driver.c
> > >+++ b/src/lxc/lxc_driver.c
> > >@@ -1024,9 +1024,13 @@ static int lxcSetupInterfaces(virConnectPtr conn,
> > >      int rc = -1, i;
> > >      char *bridge = NULL;
> > >      brControl *brctl = NULL;
> > >+    int ret;
> > >
> > >-    if (brInit(&brctl) != 0)
> > >+    if ((ret = brInit(&brctl)) != 0) {
> > >+        virReportSystemError(ret, "%s",
> > >+                             _("Unable to initialize bridging"));
> > >          return -1;
> > >+    }
> > >
> > >      for (i = 0 ; i<  def->nnets ; i++) {
> > >          char *parentVeth;
> > >@@ -1095,7 +1099,7 @@ static int lxcSetupInterfaces(virConnectPtr conn,
> > >                  goto error_exit;
> > >          }
> > >
> > >-        if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) {
> > >+        if ((ret = brAddInterface(brctl, bridge, parentVeth)) != 0) {
> > >              virReportSystemError(rc,
> > >                                   _("Failed to add %s device to %s"),
> > >                                   parentVeth, bridge);
> > >diff --git a/src/lxc/veth.c b/src/lxc/veth.c
> > >index 0fa76cf..deca26d 100644
> > >--- a/src/lxc/veth.c
> > >+++ b/src/lxc/veth.c
> > >@@ -90,7 +90,7 @@ static int getFreeVethName(char **veth, int startDev)
> > >   */
> > >  int vethCreate(char** veth1, char** veth2)
> > >  {
> > >-    int rc;
> > >+    int rc = -1;
> > >      const char *argv[] = {
> > >          "ip", "link", "add", NULL, "type", "veth", "peer", "name", NULL, NULL
> > >      };
> > >@@ -100,9 +100,8 @@ int vethCreate(char** veth1, char** veth2)
> > >      VIR_DEBUG("veth1: %s veth2: %s", NULLSTR(*veth1), NULLSTR(*veth2));
> > >
> > >      if (*veth1 == NULL) {
> > >-        vethDev = getFreeVethName(veth1, vethDev);
> > >-        if (vethDev<  0)
> > >-            return vethDev;
> > >+        if ((vethDev = getFreeVethName(veth1, vethDev))<  0)
> > >+            goto cleanup;
> > >          VIR_DEBUG("Assigned veth1: %s", *veth1);
> > >          veth1_alloc = true;
> > >      }
> > >@@ -110,11 +109,10 @@ int vethCreate(char** veth1, char** veth2)
> > >
> > >      while (*veth2 == NULL || STREQ(*veth1, *veth2)) {
> > >          VIR_FREE(*veth2);
> > >-        vethDev = getFreeVethName(veth2, vethDev + 1);
> > >-        if (vethDev<  0) {
> > >+        if ((vethDev = getFreeVethName(veth2, vethDev + 1))<  0) {
> > >              if (veth1_alloc)
> > >                  VIR_FREE(*veth1);
> > 
> > If you really want to put things back as they were prior to this
> > function being called, shouldn't you also be doing "*veth1 = NULL;"?
> > (This would also eliminate the potential of a caller doing a
> > double-free.)
> 
> VIR_FREE sets the parameter back to NULL, so *veth1 = NULL
> is in effect already done here.
> 
> > 
> > >-            return vethDev;
> > >+            goto cleanup;
> > >          }
> > >          VIR_DEBUG("Assigned veth2: %s", *veth2);
> > >      }
> > >@@ -125,9 +123,12 @@ int vethCreate(char** veth1, char** veth2)
> > >          if (veth1_alloc)
> > >              VIR_FREE(*veth1);
> > >          VIR_FREE(*veth2);
> > 
> > Likewise for both of these. Additionally, if someone were to call
> > vethCreate with a non-null veth2, and virRun() then failed,
> > vethCreate would erroneously free *veth2 - it should also be
> > protected with a "veth2_alloc".
> 
> Again VIR_FREE sets them to NULL, but we do need the
> extra check here for veth2_alloc.

  ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/