From 85aa40e26d00a64453653c32dc08d25b65e851d5 Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Thu, 14 Jul 2011 12:53:45 +0200 Subject: [PATCH] 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. --- cfg.mk | 1 + src/conf/domain_conf.c | 18 ++++++++--- src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 54 ++++++++++++++++++-------------- src/util/storage_file.c | 18 +++++++++++ src/util/storage_file.h | 2 ++ 6 files changed, 66 insertions(+), 28 deletions(-) diff --git a/cfg.mk b/cfg.mk index a35a681625..7c3e2d2224 100644 --- 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 932f83ac3b..a78996fbb8 100644 --- 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 3237d186fc..e06c7fcad4 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 b30e01edfd..8d6f76da0d 100644 --- 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 06cabc8b5a..d4460d8c5a 100644 --- 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 f1bdd02472..b8920d0d69 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); + enum { VIR_STORAGE_FILE_SHFS_NFS = (1 << 0), VIR_STORAGE_FILE_SHFS_GFS2 = (1 << 1),