[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] storage: Avoid memory leak on metadata fetching



On Thu, Jul 14, 2011 at 12:56:26 +0200, Michal Privoznik wrote:
> Getting metadata on storage allocates a memory (path) which need to
> be freed after use otherwise it gets leaked. This means after use of
> virStorageFileGetMetadataFromFD or virStorageFileGetMetadata one
> must call virStorageFileFreeMetadata to free it. This function frees
> structure internals and structure itself.
> ---
> diff to v1:
> -Eric's review suggestions taken in
> 
>  src/conf/domain_conf.c           |   17 ++++++++---
>  src/libvirt_private.syms         |    1 +
>  src/storage/storage_backend_fs.c |   53 +++++++++++++++++++++----------------
>  src/util/storage_file.c          |   16 +++++++++++
>  src/util/storage_file.h          |    2 +
>  5 files changed, 61 insertions(+), 28 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 39e59b9..c89c7c6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11055,6 +11055,12 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>      int ret = -1;
>      size_t depth = 0;
>      char *nextpath = NULL;
> +    virStorageFileMetadata *meta;
> +
> +    if (VIR_ALLOC(meta) < 0) {
> +        virReportOOMError();
> +        return ret;
> +    }
>  
>      if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
>          return 0;

You leak the allocated meta here. Let's move the allocation code after this
second if statement.

> @@ -11084,7 +11090,6 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>      paths = virHashCreate(5, NULL);
>  
>      do {
> -        virStorageFileMetadata meta;
>          const char *path = nextpath ? nextpath : disk->src;
>          int fd;
>  
> @@ -11112,7 +11117,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>              }
>          }
>  
> -        if (virStorageFileGetMetadataFromFD(path, fd, format, &meta) < 0) {
> +        if (virStorageFileGetMetadataFromFD(path, fd, format, meta) < 0) {
>              VIR_FORCE_CLOSE(fd);
>              goto cleanup;
>          }
> @@ -11126,16 +11131,17 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>              goto cleanup;
>  
>          depth++;
> -        nextpath = meta.backingStore;
> +        VIR_FREE(nextpath);
> +        nextpath = meta->backingStore;

You need to set meta->backingStore = NULL here, otherwise...

>  
>          /* Stop iterating if we reach a non-file backing store */
> -        if (nextpath && !meta.backingStoreIsFile) {
> +        if (nextpath && !meta->backingStoreIsFile) {
>              VIR_DEBUG("Stopping iteration on non-file backing store: %s",
>                        nextpath);
>              break;
>          }
>  
> -        format = meta.backingStoreFormat;
> +        format = meta->backingStoreFormat;
>  
>          if (format == VIR_STORAGE_FILE_AUTO &&
>              !allowProbing)
> @@ -11151,6 +11157,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>  cleanup:
>      virHashFree(paths);
>      VIR_FREE(nextpath);
> +    virStorageFileFreeMetadata(meta);

... the memory block referenced from meta->backingStore may be freed twice
here, once in nextpath and once in meta->backingStore.

>  
>      return ret;
>  }
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 3237d18..e06c7fc 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -940,6 +940,7 @@ virStorageGenerateQcowPassphrase;
>  # storage_file.h
>  virStorageFileFormatTypeFromString;
>  virStorageFileFormatTypeToString;
> +virStorageFileFreeMetadata;
>  virStorageFileGetMetadata;
>  virStorageFileGetMetadataFromFD;
>  virStorageFileIsSharedFS;
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index b30e01e..821efb7 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -61,8 +61,13 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
>                               unsigned long long *capacity,
>                               virStorageEncryptionPtr *encryption)
>  {
> -    int fd, ret;
> -    virStorageFileMetadata meta;
> +    int fd = -1, ret = -1;

Personally, I prefer
    int fd = -1;
    int ret = -1;

> +    virStorageFileMetadata *meta;
> +
> +    if (VIR_ALLOC(meta) < 0) {
> +        virReportOOMError();
> +        return ret;
> +    }
>  
>      *backingStore = NULL;
>      *backingStoreFormat = VIR_STORAGE_FILE_AUTO;
> @@ -71,36 +76,33 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
>  
>      if ((ret = virStorageBackendVolOpenCheckMode(target->path,
>                                          VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0)
> -        return ret; /* Take care to propagate ret, it is not always -1 */
> +        goto error; /* Take care to propagate ret, it is not always -1 */
>      fd = ret;
>  
>      if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd,
>                                                        allocation,
>                                                        capacity)) < 0) {
> -        VIR_FORCE_CLOSE(fd);
> -        return ret;
> +        goto error;
>      }
>  
> -    memset(&meta, 0, sizeof(meta));
> -
>      if ((target->format = virStorageFileProbeFormatFromFD(target->path, fd)) < 0) {
> -        VIR_FORCE_CLOSE(fd);
> -        return -1;
> +        ret = 1;
Looks like a typo here, it should be
           ret = -1;

> +        goto error;
>      }
>  
>      if (virStorageFileGetMetadataFromFD(target->path, fd,
>                                          target->format,
> -                                        &meta) < 0) {
> -        VIR_FORCE_CLOSE(fd);
> -        return -1;
> +                                        meta) < 0) {
> +        ret = -1;
> +        goto error;
>      }
>  
>      VIR_FORCE_CLOSE(fd);
>  
> -    if (meta.backingStore) {
> -        *backingStore = meta.backingStore;
> -        meta.backingStore = NULL;
> -        if (meta.backingStoreFormat == VIR_STORAGE_FILE_AUTO) {
> +    if (meta->backingStore) {
> +        *backingStore = meta->backingStore;
> +        meta->backingStore = NULL;
> +        if (meta->backingStoreFormat == VIR_STORAGE_FILE_AUTO) {
>              if ((ret = virStorageFileProbeFormat(*backingStore)) < 0) {
>                  /* If the backing file is currently unavailable, only log an error,
>                   * but continue. Returning -1 here would disable the whole storage
> @@ -114,18 +116,17 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
>                  ret = 0;
>              }
>          } else {
> -            *backingStoreFormat = meta.backingStoreFormat;
> +            *backingStoreFormat = meta->backingStoreFormat;
>              ret = 0;
>          }
>      } else {
> -        VIR_FREE(meta.backingStore);
>          ret = 0;
>      }
>  
> -    if (capacity && meta.capacity)
> -        *capacity = meta.capacity;
> +    if (capacity && meta->capacity)
> +        *capacity = meta->capacity;
>  
> -    if (encryption != NULL && meta.encrypted) {
> +    if (encryption != NULL && meta->encrypted) {
>          if (VIR_ALLOC(*encryption) < 0) {
>              virReportOOMError();
>              goto cleanup;
> @@ -147,11 +148,17 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
>           */
>      }
>  
> +    virStorageFileFreeMetadata(meta);
> +
>      return ret;
>  
> +error:
> +    VIR_FORCE_CLOSE(fd);
> +
>  cleanup:
> -    VIR_FREE(*backingStore);
> -    return -1;
> +    virStorageFileFreeMetadata(meta);
> +    return ret;
> +

To avoid code duplication, this could have been

        ...
    cleanup:
        virStorageFileFreeMetadata(meta);
        return ret;

    error:
        VIR_FORCE_CLOSE(fd);
        goto cleanup;

But it's just one line, so not a big deal (unless the cleanup code is
expanded, in which case it's easier to forgot to update both chunks).

...
> @@ -912,6 +916,18 @@ virStorageFileGetMetadata(const char *path,
>      return ret;
>  }
>  
> +/**
> + * virStorageFileFreeMetadata:
> + *
> + * Free pointers in passed structure, but not memory used by
> + * structure itself.
> + */
> +void ATTRIBUTE_NONNULL(1)
> +virStorageFileFreeMetadata(virStorageFileMetadata *meta)
> +{
> +    VIR_FREE(meta->backingStore);
> +    VIR_FREE(meta);
> +}

We like having free-like functions to work with NULL arguments, shouldn't we
follow that habit here as well?

> diff --git a/src/util/storage_file.h b/src/util/storage_file.h
> index f1bdd02..b362796 100644
> --- a/src/util/storage_file.h
> +++ b/src/util/storage_file.h
> @@ -70,6 +70,8 @@ int virStorageFileGetMetadataFromFD(const char *path,
>                                      int format,
>                                      virStorageFileMetadata *meta);
>  
> +void virStorageFileFreeMetadata(virStorageFileMetadata meta);
> +

On the other hand, if we insist on having ATTRIBUTE_NONNULL, we should say
that in the header file.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]