9:42 a.m.
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
9:42 a.m.
New subject: [libvirt] [PATCH 1/2] util: storage: Clean up label use in virStorageFileGetMetadataInternal
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(a)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
9:42 a.m.
New subject: [libvirt] [PATCH 2/2] util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata
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(a)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
9:52 a.m.
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(a)redhat.com>
Jano
9:53 a.m.
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(a)redhat.com>
Michal
1954
days inactive
1954
days old
4 comments
3 participants
participants (3)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa