[libvirt] [PATCH 0/2] qemu: blockPeek: Fix invalid buffer usage and
See 1/2 for the bugfix. Peter Krempa (2): qemu: blockPeek: Fix filling of the return buffer qemu: blockPeek: Enforce buffer filling src/qemu/qemu_driver.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) -- 2.14.1
Commit 3956af495e broke the blockPeek API since virStorageFileRead allocates a return buffer and fills it with the data, while the API fills a user-provided buffer. This did not get caught by the compiler since the API prototype uses a 'void *'. Fix it by transferring the data from the allocated buffer to the user provided buffer. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1491217 --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e1a0dd553..93a1c6061 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11415,6 +11415,7 @@ qemuDomainBlockPeek(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainDiskDefPtr disk = NULL; virDomainObjPtr vm; + char *tmpbuf = NULL; int ret = -1; virCheckFlags(0, -1); @@ -11444,12 +11445,15 @@ qemuDomainBlockPeek(virDomainPtr dom, if (virStorageFileRead(disk->src, offset, size, buffer) < 0) goto cleanup; + memcpy(buffer, tmpbuf, size); + ret = 0; cleanup: if (disk) virStorageFileDeinit(disk->src); virDomainObjEndAPI(&vm); + VIR_FREE(tmpbuf); return ret; } -- 2.14.1
On 09/18/2017 09:11 AM, Peter Krempa wrote:
Commit 3956af495e broke the blockPeek API since virStorageFileRead allocates a return buffer and fills it with the data, while the API fills a user-provided buffer. This did not get caught by the compiler since the API prototype uses a 'void *'.
Fix it by transferring the data from the allocated buffer to the user provided buffer.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1491217 --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e1a0dd553..93a1c6061 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11415,6 +11415,7 @@ qemuDomainBlockPeek(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainDiskDefPtr disk = NULL; virDomainObjPtr vm; + char *tmpbuf = NULL; int ret = -1;
virCheckFlags(0, -1); @@ -11444,12 +11445,15 @@ qemuDomainBlockPeek(virDomainPtr dom, if (virStorageFileRead(disk->src, offset, size, buffer) < 0) goto cleanup;
+ memcpy(buffer, tmpbuf, size);
Umm, where is tmpbuf actually set to a non-null pointer? Shouldn't the virStorageFileRead() call also be updated? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Mon, Sep 18, 2017 at 12:29:57 -0500, Eric Blake wrote:
On 09/18/2017 09:11 AM, Peter Krempa wrote:
Commit 3956af495e broke the blockPeek API since virStorageFileRead allocates a return buffer and fills it with the data, while the API fills a user-provided buffer. This did not get caught by the compiler since the API prototype uses a 'void *'.
Fix it by transferring the data from the allocated buffer to the user provided buffer.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1491217 --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e1a0dd553..93a1c6061 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11415,6 +11415,7 @@ qemuDomainBlockPeek(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; virDomainDiskDefPtr disk = NULL; virDomainObjPtr vm; + char *tmpbuf = NULL; int ret = -1;
virCheckFlags(0, -1); @@ -11444,12 +11445,15 @@ qemuDomainBlockPeek(virDomainPtr dom, if (virStorageFileRead(disk->src, offset, size, buffer) < 0) goto cleanup;
+ memcpy(buffer, tmpbuf, size);
Umm, where is tmpbuf actually set to a non-null pointer? Shouldn't the virStorageFileRead() call also be updated?
Oh, I messed up splitting of the two changes in this series. I obviously tested only both patches.
Documentation states: "'offset' and 'size' represent an area which must lie entirely within the device or file." Enforce the that the buffer lies within fully. --- src/qemu/qemu_driver.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 93a1c6061..bddba6b71 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11416,6 +11416,7 @@ qemuDomainBlockPeek(virDomainPtr dom, virDomainDiskDefPtr disk = NULL; virDomainObjPtr vm; char *tmpbuf = NULL; + ssize_t nread; int ret = -1; virCheckFlags(0, -1); @@ -11442,9 +11443,16 @@ qemuDomainBlockPeek(virDomainPtr dom, if (qemuDomainStorageFileInit(driver, vm, disk->src) < 0) goto cleanup; - if (virStorageFileRead(disk->src, offset, size, buffer) < 0) + if ((nread = virStorageFileRead(disk->src, offset, size, &tmpbuf)) < 0) goto cleanup; + if (nread < size) { + virReportError(VIR_ERR_INVALID_ARG, + _("'%s' starting from %llu has only %zd bytes available"), + path, offset, nread); + goto cleanup; + } + memcpy(buffer, tmpbuf, size); ret = 0; -- 2.14.1
participants (2)
-
Eric Blake -
Peter Krempa