[libvirt] [PATCH 0/2] virstoragefile cleanups
I found these cleanups while trying to solve the regression that John identified, where we are losing the correct capacity read from a qcow2 header when it comes time to dump a volume's xml. I still plan to get that regression fixed before 1.2.4 goes out the door, but this was worth posting in the meantime. Eric Blake (2): conf: avoid null deref during storage probe conf: drop extra storage probe src/libvirt_private.syms | 1 - src/storage/storage_backend_fs.c | 8 -------- src/storage/storage_backend_gluster.c | 5 ----- src/util/virstoragefile.c | 37 ++++++++++++++++------------------- src/util/virstoragefile.h | 7 ++----- 5 files changed, 19 insertions(+), 39 deletions(-) -- 1.9.0
Commit 5c43e2e introduced a NULL deref if there is a failure in virStorageFileGetMetadataInternal. * src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf): Fix error handling. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1ce0fa1..dcce1ef 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -999,21 +999,21 @@ virStorageFileGetMetadataFromBuf(const char *path, int *backingFormat) { virStorageSourcePtr ret = NULL; + virStorageSourcePtr meta = NULL; - if (!(ret = virStorageFileMetadataNew(path, format))) + if (!(meta = virStorageFileMetadataNew(path, format))) return NULL; - if (virStorageFileGetMetadataInternal(ret, buf, len, - backingFormat) < 0) { - virStorageSourceFree(ret); - ret = NULL; - } - - if (VIR_STRDUP(*backing, ret->backingStoreRaw) < 0) { - virStorageSourceFree(ret); - ret = NULL; - } + if (virStorageFileGetMetadataInternal(meta, buf, len, + backingFormat) < 0) + goto cleanup; + if (VIR_STRDUP(*backing, meta->backingStoreRaw) < 0) + goto cleanup; + ret = meta; + meta = NULL; + cleanup: + virStorageSourceFree(meta); return ret; } -- 1.9.0
On Mon, Apr 28, 2014 at 09:53:54PM -0600, Eric Blake wrote:
Commit 5c43e2e introduced a NULL deref if there is a failure in virStorageFileGetMetadataInternal.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf): Fix error handling.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
ACK, Martin
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1ce0fa1..dcce1ef 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -999,21 +999,21 @@ virStorageFileGetMetadataFromBuf(const char *path, int *backingFormat) { virStorageSourcePtr ret = NULL; + virStorageSourcePtr meta = NULL;
- if (!(ret = virStorageFileMetadataNew(path, format))) + if (!(meta = virStorageFileMetadataNew(path, format))) return NULL;
- if (virStorageFileGetMetadataInternal(ret, buf, len, - backingFormat) < 0) { - virStorageSourceFree(ret); - ret = NULL; - } - - if (VIR_STRDUP(*backing, ret->backingStoreRaw) < 0) { - virStorageSourceFree(ret); - ret = NULL; - } + if (virStorageFileGetMetadataInternal(meta, buf, len, + backingFormat) < 0) + goto cleanup; + if (VIR_STRDUP(*backing, meta->backingStoreRaw) < 0) + goto cleanup;
+ ret = meta; + meta = NULL; + cleanup: + virStorageSourceFree(meta); return ret; }
On 04/29/2014 02:01 AM, Martin Kletzander wrote:
On Mon, Apr 28, 2014 at 09:53:54PM -0600, Eric Blake wrote:
Commit 5c43e2e introduced a NULL deref if there is a failure in virStorageFileGetMetadataInternal.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf): Fix error handling.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virstoragefile.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
ACK,
Thanks; pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
All callers of virStorageFileGetMetadataFromBuf were first calling virStorageFileProbeFormatFromBuf, to learn what format to pass in. But this function is already wired to do the exact same probe if the incoming format is VIR_STORAGE_FILE_AUTO, so it's simpler to just refactor the probing into the central function. * src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf): Drop parameter. (virStorageFileProbeFormatFromBuf): Drop declaration. * src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf): Do probe here instead of in callers. (virStorageFileProbeFormatFromBuf): Make static. * src/libvirt_private.syms (virstoragefile.h): Drop function. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Update caller. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 - src/storage/storage_backend_fs.c | 8 -------- src/storage/storage_backend_gluster.c | 5 ----- src/util/virstoragefile.c | 17 +++++++---------- src/util/virstoragefile.h | 7 ++----- 5 files changed, 9 insertions(+), 29 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8bbe6e7..c95fa73 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1854,7 +1854,6 @@ virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; virStorageFileProbeFormat; -virStorageFileProbeFormatFromBuf; virStorageFileResize; virStorageIsFile; virStorageNetHostDefClear; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0f98853..f18fafe 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -97,16 +97,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto error; } - target->format = virStorageFileProbeFormatFromBuf(target->path, - header, len); - if (target->format < 0) { - ret = -1; - goto error; - } - if (!(meta = virStorageFileGetMetadataFromBuf(target->path, header, len, - target->format, backingStore, backingStoreFormat))) { ret = -1; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 5ab1e7e..20fb63e 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -293,12 +293,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if ((len = virStorageBackendGlusterReadHeader(fd, name, len, &header)) < 0) goto cleanup; - if ((vol->target.format = virStorageFileProbeFormatFromBuf(name, - header, - len)) < 0) - goto cleanup; if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, - vol->target.format, &vol->backingStore.path, &vol->backingStore.format))) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dcce1ef..5dbb6f0 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -713,7 +713,8 @@ virStorageIsFile(const char *backing) return true; } -int + +static int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen) @@ -971,16 +972,13 @@ virStorageFileMetadataNew(const char *path, * @path: name of file, for error messages * @buf: header bytes from @path * @len: length of @buf - * @format: expected image format * @backing: output malloc'd name of backing image, if any * @backingFormat: format of @backing * - * Extract metadata about the storage volume with the specified - * image format. If image format is VIR_STORAGE_FILE_AUTO, it - * will probe to automatically identify the format. Does not recurse. - * - * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a - * format, since a malicious guest can turn a raw file into any + * Extract metadata about the storage volume, including probing its + * format. Does not recurse. Callers are advised not to trust the + * learned format if a guest has ever used the volume when it was + * raw, since a malicious guest can turn a raw file into any * other non-raw format at will. * * If the returned @backingFormat is VIR_STORAGE_FILE_AUTO @@ -994,14 +992,13 @@ virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - int format, char **backing, int *backingFormat) { virStorageSourcePtr ret = NULL; virStorageSourcePtr meta = NULL; - if (!(meta = virStorageFileMetadataNew(path, format))) + if (!(meta = virStorageFileMetadataNew(path, VIR_STORAGE_FILE_AUTO))) return NULL; if (virStorageFileGetMetadataInternal(meta, buf, len, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 148776e..8592d13 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -260,8 +260,6 @@ struct _virStorageSource { # endif int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); -int virStorageFileProbeFormatFromBuf(const char *path, char *buf, - size_t buflen); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, @@ -273,11 +271,10 @@ virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - int format, char **backing, int *backingFormat) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) - ATTRIBUTE_NONNULL(6); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(5); int virStorageFileChainGetBroken(virStorageSourcePtr chain, char **broken_file); -- 1.9.0
On Mon, Apr 28, 2014 at 09:53:55PM -0600, Eric Blake wrote:
All callers of virStorageFileGetMetadataFromBuf were first calling virStorageFileProbeFormatFromBuf, to learn what format to pass in. But this function is already wired to do the exact same probe if the incoming format is VIR_STORAGE_FILE_AUTO, so it's simpler to just refactor the probing into the central function.
* src/util/virstoragefile.h (virStorageFileGetMetadataFromBuf): Drop parameter. (virStorageFileProbeFormatFromBuf): Drop declaration. * src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf): Do probe here instead of in callers. (virStorageFileProbeFormatFromBuf): Make static. * src/libvirt_private.syms (virstoragefile.h): Drop function. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Update caller. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 - src/storage/storage_backend_fs.c | 8 -------- src/storage/storage_backend_gluster.c | 5 ----- src/util/virstoragefile.c | 17 +++++++---------- src/util/virstoragefile.h | 7 ++----- 5 files changed, 9 insertions(+), 29 deletions(-)
ACK, Martin
On 04/29/2014 05:12 AM, Martin Kletzander wrote:
On Mon, Apr 28, 2014 at 09:53:55PM -0600, Eric Blake wrote:
All callers of virStorageFileGetMetadataFromBuf were first calling virStorageFileProbeFormatFromBuf, to learn what format to pass in. But this function is already wired to do the exact same probe if the incoming format is VIR_STORAGE_FILE_AUTO, so it's simpler to just refactor the probing into the central function.
src/libvirt_private.syms | 1 - src/storage/storage_backend_fs.c | 8 -------- src/storage/storage_backend_gluster.c | 5 ----- src/util/virstoragefile.c | 17 +++++++---------- src/util/virstoragefile.h | 7 ++----- 5 files changed, 9 insertions(+), 29 deletions(-)
ACK,
Pushed, now that 1.2.4 is out. I'm still working on further cleanups in the area as well. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Martin Kletzander