On 09/15/2014 10:22 AM, Peter Krempa wrote:
The backing store string location offset 0 determines that the file
isn't present. The string size shouldn't be then checked:
from qemu.git/docs/specs/qcow2.txt
== Header ==
The first cluster of a qcow2 image contains the file header:
Byte 0 - 3: magic
QCOW magic string ("QFI\xfb")
4 - 7: version
Version number (valid values are 2 and 3)
8 - 15: backing_file_offset
Offset into the image file at which the backing file name
is stored (NB: The string is not null terminated). 0 if the
image doesn't have a backing file.
16 - 19: backing_file_size
Length of the backing file name in bytes. Must not be
longer than 1023 bytes. Undefined if the image doesn't have
a backing file. ^^^^^^^^^
This patch intentionally leaves the backing file string size check in
place in case a malformatted file would be presented to libvirt. Also
according to the docs the string size is maximum 1023 bytes, thus this
patch adds a check to verify that.
I was also able to verify that the check was done the same way in the
legacy qcow fromat (in qemu's code).
---
src/util/virstoragefile.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 5db9184..13056a7 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -385,15 +385,22 @@ qcowXGetBackingStore(char **res,
offset = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET);
if (offset > buf_size)
return BACKING_STORE_INVALID;
+
+ if (offset == 0) {
+ if (format)
+ *format = VIR_STORAGE_FILE_NONE;
+ return BACKING_STORE_OK;
+ }
+
size = virReadBufInt32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE);
if (size == 0) {
if (format)
*format = VIR_STORAGE_FILE_NONE;
return BACKING_STORE_OK;
}
- if (offset + size > buf_size || offset + size < offset)
+ if (size > 1023)
return BACKING_STORE_INVALID;
- if (size + 1 == 0)
+ if (offset + size > buf_size || offset + size < offset)
return BACKING_STORE_INVALID;
if (VIR_ALLOC_N(*res, size + 1) < 0)
return BACKING_STORE_ERROR;
ACK
And yes, Coverity is fine with this - I will remove patch 3 from my
other series before pushing...
Tks,
John