[libvirt] [PATCH 3/3] vbox: snapshot: Avoid memleaks in functions dealing with disk arrays

[Yohan Belleguic]

Le Tuesday 17 June 2014 11:24:09, Peter Krempa a écrit :
> In virVBoxSnapshotConfRemoveFakeDisks and
> virVBoxSnapshotConfDiskIsInMediaRegistry the disk array constructed from
> all the disks would be leaked at the end of the function and on
> allocation errors. Also the temporary disk list would be leaked.
> 
> Add a cleanup section and free the memory properly.
> 
> Found by coverity.
> ---
>  src/vbox/vbox_snapshot_conf.c | 54
> +++++++++++++++++++++++++++++++++++-------- 1 file changed, 44
> insertions(+), 10 deletions(-)
> 
> diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c
> index 69c7e42..49fd2b6 100644
> --- a/src/vbox/vbox_snapshot_conf.c
> +++ b/src/vbox/vbox_snapshot_conf.c
> @@ -1471,28 +1471,45 @@
> virVBoxSnapshotConfRemoveFakeDisks(virVBoxSnapshotConfMachinePtr machine)
> size_t diskSize = 0;
>      virVBoxSnapshotConfHardDiskPtr *tempList = NULL;
>      virVBoxSnapshotConfHardDiskPtr *diskList = NULL;
> +
>      if (VIR_ALLOC_N(diskList, 0) < 0)
> -        return ret;
> +        return -1;
> 
>      for (i = 0; i < machine->mediaRegistry->ndisks; i++) {
>          tempSize =
> virVBoxSnapshotConfAllChildren(machine->mediaRegistry->disks[i],
> &tempList); if (VIR_EXPAND_N(diskList, diskSize, tempSize) < 0)
> -            return ret;
> -        for (j = 0; j < tempSize; j++) {
> +            goto cleanup;
> +
> +        for (j = 0; j < tempSize; j++)
>              diskList[diskSize - tempSize + j] = tempList[j];
> -        }
> +
> +        VIR_FREE(tempList);
>      }
> +
>      for (i = 0; i < diskSize; i++) {
>          if (strstr(diskList[i]->location, "fake") != NULL) {
>              if (virVBoxSnapshotConfRemoveHardDisk(machine->mediaRegistry,
> diskList[i]->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("Unable to remove hard disk %s from media
> registry"), diskList[i]->location);
> -                return ret;
> +                goto cleanup;
>              }
>          }
>      }
> +
>      ret = 0;
> +
> + cleanup:
> +    for (i = 0; i < diskSize; i++)
> +        virVboxSnapshotConfHardDiskFree(diskList[i]);
> +    VIR_FREE(diskList);
> +
> +    if (tempList) {
> +        for (i = 0; i < tempSize; i++)
> +            virVboxSnapshotConfHardDiskFree(tempList[i]);
> +        VIR_FREE(tempList);
> +    }
Same comments than the previous patch
> +
>      return ret;
>  }
> 
> @@ -1513,24 +1530,41 @@
> virVBoxSnapshotConfDiskIsInMediaRegistry(virVBoxSnapshotConfMachinePtr
> machine, size_t diskSize = 0;
>      virVBoxSnapshotConfHardDiskPtr *tempList = NULL;
>      virVBoxSnapshotConfHardDiskPtr *diskList = NULL;
> +
>      if (VIR_ALLOC_N(diskList, 0) < 0)
> -        return ret;
> +        return -1;
> 
>      for (i = 0; i < machine->mediaRegistry->ndisks; i++) {
>          tempSize =
> virVBoxSnapshotConfAllChildren(machine->mediaRegistry->disks[i],
> &tempList); if (VIR_EXPAND_N(diskList, diskSize, tempSize) < 0)
> -            return ret;
> -        for (j = 0; j < tempSize; j++) {
> +            goto cleanup;
> +
> +        for (j = 0; j < tempSize; j++)
>              diskList[diskSize - tempSize + j] = tempList[j];
> -        }
> +
> +        VIR_FREE(tempList);
>      }
> +
>      for (i = 0; i < diskSize; i++) {
>          if (STREQ(diskList[i]->location, location)) {
>              ret = 1;
> -            return ret;
> +            goto cleanup;
>          }
>      }
> +
>      ret = 0;
> +
> + cleanup:
> +    for (i = 0; i < diskSize; i++)
> +        virVboxSnapshotConfHardDiskFree(diskList[i]);
> +    VIR_FREE(diskList);
> +
> +    if (tempList) {
> +        for (i = 0; i < tempSize; i++)
> +            virVboxSnapshotConfHardDiskFree(tempList[i]);
> +        VIR_FREE(tempList);
> +    }
Same comments than the previous patch
> +
>      return ret;
>  }

-- 
Yohan BELLEGUIC
Software Engineer - diateam : Architectes de l'information
Phone : +33 (0)2 98 050 050 Fax : +33 (0)2 98 050 05