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

[Laine Stump]

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.)

> -            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".


It doesn't make any difference with what the callers are doing now 
(since they ignore the two ifnames if vethCreate returns < 0) but it 
protects against future changes.

> -        rc = -1;
> +        goto cleanup;
>       }
>
> +    rc = 0;
> +
> +cleanup:
>       return rc;
>   }
>