[libvirt] [PATCH 0/2] util: Fix memleak after (legacy) blockjob (blockdev-add saga spin-off)
While verifying that legacy block jobs are not broken I've found a memleak. Peter Krempa (2): util: storage: Clean up label use in virStorageFileGetMetadataInternal util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata src/util/virstoragefile.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) -- 2.21.0
The function does not do any cleanup, so replace the 'cleanup' label with return of -1 and the 'done' label with return of 0. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 269d0050fd..4e2e7540f1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -973,7 +973,6 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int *backingFormat) { int dummy; - int ret = -1; size_t i; if (!backingFormat) @@ -989,7 +988,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, meta->format >= VIR_STORAGE_FILE_LAST) { virReportSystemError(EINVAL, _("unknown storage file meta->format %d"), meta->format); - goto cleanup; + return -1; } if (fileTypeInfo[meta->format].cryptInfo != NULL) { @@ -999,7 +998,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int expt_fmt = fileTypeInfo[meta->format].cryptInfo[i].format; if (!meta->encryption) { if (VIR_ALLOC(meta->encryption) < 0) - goto cleanup; + return -1; meta->encryption->format = expt_fmt; } else { @@ -1008,7 +1007,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, _("encryption format %d doesn't match " "expected format %d"), meta->encryption->format, expt_fmt); - goto cleanup; + return -1; } } meta->encryption->payload_offset = @@ -1021,12 +1020,12 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, * code into this method, for non-magic files */ if (!fileTypeInfo[meta->format].magic) - goto done; + return 0; /* Optionally extract capacity from file */ if (fileTypeInfo[meta->format].sizeOffset != -1) { if ((fileTypeInfo[meta->format].sizeOffset + 8) > len) - goto done; + return 0; if (fileTypeInfo[meta->format].endian == LV_LITTLE_ENDIAN) meta->capacity = virReadBufInt64LE(buf + @@ -1037,7 +1036,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, /* Avoid unlikely, but theoretically possible overflow */ if (meta->capacity > (ULLONG_MAX / fileTypeInfo[meta->format].sizeMultiplier)) - goto done; + return 0; meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier; } @@ -1047,25 +1046,21 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, backingFormat, buf, len); if (store == BACKING_STORE_INVALID) - goto done; + return 0; if (store == BACKING_STORE_ERROR) - goto cleanup; + return -1; } if (fileTypeInfo[meta->format].getFeatures != NULL && fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0) - goto cleanup; + return -1; if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features && VIR_STRDUP(meta->compat, "1.1") < 0) - goto cleanup; - - done: - ret = 0; + return -1; - cleanup: - return ret; + return 0; } -- 2.21.0
When querying storage metadata after a block job we re-run virStorageFileGetMetadata on the top level storage file. This means that the workers (virStorageFileGetMetadataInternal) must not overwrite any pointers without freeing them. This was not considered for src->compat and src->features. Fix it and add a comment mentioning that. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 4e2e7540f1..a6de6a1e45 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -965,7 +965,11 @@ virStorageFileGetEncryptionPayloadOffset(const struct FileEncryptionInfo *info, * assuming it has the given FORMAT, populate information into META * with information about the file and its backing store. Return format * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be - * pre-populated in META */ + * pre-populated in META. + * + * Note that this function may be called repeatedly on @meta, so it must + * clean up any existing allocated memory which would be overwritten. + */ int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, @@ -1052,10 +1056,13 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, return -1; } + virBitmapFree(meta->features); + meta->features = NULL; if (fileTypeInfo[meta->format].getFeatures != NULL && fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0) return -1; + VIR_FREE(meta->compat); if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features && VIR_STRDUP(meta->compat, "1.1") < 0) return -1; -- 2.21.0
On Thu, Jul 18, 2019 at 04:42:00PM +0200, Peter Krempa wrote:
While verifying that legacy block jobs are not broken I've found a memleak.
Peter Krempa (2): util: storage: Clean up label use in virStorageFileGetMetadataInternal util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata
src/util/virstoragefile.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 7/18/19 4:42 PM, Peter Krempa wrote:
While verifying that legacy block jobs are not broken I've found a memleak.
Peter Krempa (2): util: storage: Clean up label use in virStorageFileGetMetadataInternal util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata
src/util/virstoragefile.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa