[PATCH qemu] block: Stricter checks of VMDK descriptors but allow unquoted createdType

Christian Biere <[email protected]>
Newsgroups gmane.comp.emulators.qemu
Message-ID <[email protected]>
Veeam, for example, creates VMDK exports where the line declaring the 
createType property isn't surrounded by double-quotes. The resulting 
error message is cryptic but indicates that the parser of qemu-img 
doesn't check if createType is follow by a '=' character but just skips 
over it, the same goes for the initial quote. The patch adds checks for 
the '=' character for all properties but relaxes the requirement to quote 
the value of createType. I came across this due to a post in the Proxmox 
forum where the issue was reported 10 years ago before.

https://forum.proxmox.com/threads/import-disk-vmdk-qcow2-unsupported-image-type-onolithicflat.135903/

Signed-off-by: Christian Biere <[email protected]>
diff --git a/block/vmdk.c b/block/vmdk.c
index cd8b4ec7c8..8153e007b9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -305,7 +305,6 @@ vmdk_read_cid(BlockDriverState *bs, int parent, 
uint32_t *pcid)
     char *desc;
     uint32_t cid;
     const char *p_name, *cid_str;
-    size_t cid_str_size;
     BDRVVmdkState *s = bs->opaque;
     int ret;

@@ -316,11 +315,9 @@ vmdk_read_cid(BlockDriverState *bs, int parent, 
uint32_t *pcid)
     }

     if (parent) {
-        cid_str = "parentCID";
-        cid_str_size = sizeof("parentCID");
+        cid_str = "parentCID=";
     } else {
-        cid_str = "CID";
-        cid_str_size = sizeof("CID");
+        cid_str = "CID=";
     }

     desc[DESC_SIZE - 1] = '\0';
@@ -329,7 +326,7 @@ vmdk_read_cid(BlockDriverState *bs, int parent, 
uint32_t *pcid)
         ret = -EINVAL;
         goto out;
     }
-    p_name += cid_str_size;
+    p_name += strlen(cid_str);
     if (sscanf(p_name, "%" SCNx32, &cid) != 1) {
         ret = -EINVAL;
         goto out;
@@ -345,6 +342,7 @@ out:
static int coroutine_fn GRAPH_RDLOCK
vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
{
+    const char *substr;
     char *desc, *tmp_desc;
     char *p_name, *tmp_str;
     BDRVVmdkState *s = bs->opaque;
@@ -370,16 +368,18 @@ vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
     }

     desc[desc_buf_size - 1] = '\0';
-    tmp_str = strstr(desc, "parentCID");
+    substr = "parentCID=";
+    tmp_str = strstr(desc, substr);
     if (tmp_str == NULL) {
         ret = -EINVAL;
         goto out;
     }

     pstrcpy(tmp_desc, desc_buf_size, tmp_str);
-    p_name = strstr(desc, "CID");
+    substr = "CID=";
+    p_name = strstr(desc, substr);
     if (p_name != NULL) {
-        p_name += sizeof("CID");
+        p_name += strlen(substr);
         snprintf(p_name, desc_buf_size - (p_name - desc), "%" PRIx32 
"\n", cid);
         pstrcat(desc, desc_buf_size, tmp_desc);
     }
@@ -485,6 +485,7 @@ static void vmdk_reopen_abort(BDRVReopenState *state)

static int GRAPH_RDLOCK vmdk_parent_open(BlockDriverState *bs)
{
+    const char *substr;
     char *p_name;
     char *desc;
     BDRVVmdkState *s = bs->opaque;
@@ -496,11 +497,12 @@ static int GRAPH_RDLOCK 
vmdk_parent_open(BlockDriverState *bs)
         goto out;
     }

-    p_name = strstr(desc, "parentFileNameHint");
+    substr = "parentFileNameHint=\"";
+    p_name = strstr(desc, substr);
     if (p_name != NULL) {
         char *end_name;

-        p_name += sizeof("parentFileNameHint") + 1;
+        p_name += strlen(substr);
         end_name = strchr(p_name, '\"');
         if (end_name == NULL) {
             ret = -EINVAL;
@@ -516,6 +518,8 @@ static int GRAPH_RDLOCK 
vmdk_parent_open(BlockDriverState *bs)
                 bs->auto_backing_file);
         pstrcpy(bs->backing_format, sizeof(bs->backing_format),
                 "vmdk");
+    } else {
+        ret = -EINVAL;
     }

out:
@@ -1096,14 +1100,30 @@ static int vmdk_parse_description(const char 
*desc, const char *opt_name,
     if (!opt_pos) {
         return VMDK_ERROR;
     }
-    /* Skip "=\"" following opt_name */
-    opt_pos += strlen(opt_name) + 2;
-    if (opt_pos >= end) {
+    opt_pos += strlen(opt_name);
+    if (*opt_pos++ != '=') {
+        /* Skip "=" following opt_name */
         return VMDK_ERROR;
     }
-    opt_end = opt_pos;
-    while (opt_end < end && *opt_end != '"') {
-        opt_end++;
+    if (*opt_pos == '"') {
+        /* Skip "=" following opt_name */
+        opt_end = ++opt_pos;
+        while (opt_end < end && *opt_end != '"') {
+            opt_end++;
+        }
+    } else {
+        /* Assume the value is unquoted but line-terminated */
+        opt_end = opt_pos;
+        while (opt_end < end && *opt_end != '\n') {
+            opt_end++;
+        }
+        if (*opt_end == '\n' && opt_end[-1] == '\r') {
+            /* skip '\r' if windows line endings used. */
+            opt_end--;
+        }
+    }
+    if (opt_end >= end) {
+        return VMDK_ERROR;
     }
     if (opt_end == end || buf_size < opt_end - opt_pos + 1) {
         return VMDK_ERROR;
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.