storage: Avoid memory leak on metadata fetching

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 --git a/cfg.mk b/cfg.mk
index a35a6816250d09359b4090fb4700b6b3c90966ff..7c3e2d2224f68a278b128c54c6706a07fb7e2261 100644 (file)
--- a/cfg.mk
+++ b/cfg.mk
@@ -156,6 +156,7 @@ useless_free_options =                              \
   --name=virSecretDefFree                      \
   --name=virStorageEncryptionFree              \
   --name=virStorageEncryptionSecretFree                \
+  --name=virStorageFileFreeMetadata            \
   --name=virStoragePoolDefFree                 \
   --name=virStoragePoolObjFree                 \
   --name=virStoragePoolSourceFree              \

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 932f83ac3b28687785452961e388f0344dbb2d1e..a78996fbb8cb4013027db41ea973e3f3a520b950 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11049,10 +11049,16 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
     int ret = -1;
     size_t depth = 0;
     char *nextpath = NULL;
+    virStorageFileMetadata *meta;
 
     if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
         return 0;
 
+    if (VIR_ALLOC(meta) < 0) {
+        virReportOOMError();
+        return ret;
+    }
+
     if (disk->driverType) {
         const char *formatStr = disk->driverType;
         if (STREQ(formatStr, "aio"))
@@ -11078,7 +11084,6 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
     paths = virHashCreate(5, NULL);
 
     do {
-        virStorageFileMetadata meta;
         const char *path = nextpath ? nextpath : disk->src;
         int fd;
 
@@ -11106,7 +11111,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;
         }
@@ -11120,16 +11125,18 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
             goto cleanup;
 
         depth++;
-        nextpath = meta.backingStore;
+        VIR_FREE(nextpath);
+        nextpath = meta->backingStore;
+        meta->backingStore = NULL;
 
         /* 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)
@@ -11145,6 +11152,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
 cleanup:
     virHashFree(paths);
     VIR_FREE(nextpath);
+    virStorageFileFreeMetadata(meta);
 
     return ret;
 }

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3237d186fc285b279a7cb69c531f714108436ebb..e06c7fcad45fe020684c8572c4fd9d2dc7d10b0d 100644 (file)
--- 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 b30e01edfdc675f3db83d13558d619d6320c07e8..8d6f76da0d7633442f3f2d4b3a00c1de12360601 100644 (file)
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -61,8 +61,14 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
                              unsigned long long *capacity,
                              virStorageEncryptionPtr *encryption)
 {
-    int fd, ret;
-    virStorageFileMetadata meta;
+    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 +77,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;
+        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 +117,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 +149,17 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
          */
     }
 
+    virStorageFileFreeMetadata(meta);
+
     return ret;
 
+error:
+    VIR_FORCE_CLOSE(fd);
+
 cleanup:
-    VIR_FREE(*backingStore);
-    return -1;
+    virStorageFileFreeMetadata(meta);
+    return ret;
+
 }
 
 #if WITH_STORAGE_FS

diff --git a/src/util/storage_file.c b/src/util/storage_file.c
index 06cabc8b5a3c5239b3ed2302b23e9a9ba71b4818..d4460d8c5acf0542d2db1b9d15ea598f8d2b91d5 100644 (file)
--- a/src/util/storage_file.c
+++ b/src/util/storage_file.c
@@ -819,6 +819,8 @@ virStorageFileProbeFormat(const char *path)
  * it indicates the image didn't specify an explicit format for its
  * backing store. Callers are advised against probing for the
  * backing store format in this case.
+ *
+ * Caller MUST free @meta after use via virStorageFileFreeMetadata.
  */
 int
 virStorageFileGetMetadataFromFD(const char *path,
@@ -892,6 +894,8 @@ cleanup:
  * it indicates the image didn't specify an explicit format for its
  * backing store. Callers are advised against probing for the
  * backing store format in this case.
+ *
+ * Caller MUST free @meta after use via virStorageFileFreeMetadata.
  */
 int
 virStorageFileGetMetadata(const char *path,
@@ -912,6 +916,20 @@ virStorageFileGetMetadata(const char *path,
     return ret;
 }
 
+/**
+ * virStorageFileFreeMetadata:
+ *
+ * Free pointers in passed structure and structure itself.
+ */
+void
+virStorageFileFreeMetadata(virStorageFileMetadata *meta)
+{
+    if (!meta)
+        return;
+
+    VIR_FREE(meta->backingStore);
+    VIR_FREE(meta);
+}
 
 #ifdef __linux__
 

diff --git a/src/util/storage_file.h b/src/util/storage_file.h
index f1bdd024729c7f407a56ace96185e2f3f4076438..b8920d0d6982e218cf0745f1838db4cf79dd6541 100644 (file)
--- 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);
+
 enum {
     VIR_STORAGE_FILE_SHFS_NFS = (1 << 0),
     VIR_STORAGE_FILE_SHFS_GFS2 = (1 << 1),