Patchwork [3/4] savevm: improve subsections detection on load

login
register
mail settings
Submitter Juan Quintela
Date Oct. 4, 2011, 2:38 p.m.
Message ID <e102d670b1d5eba2256bc125ba0ec4a34f0983b0.1317738629.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/117632/
State New
Headers show

Comments

Juan Quintela - Oct. 4, 2011, 2:38 p.m.
We add qemu_peek_buffer, that is identical to qemu_get_buffer, just
that it don't update f->buf_index.

We add a paramenter to qemu_peek_byte() to be able to peek more than
one byte.

Once this is done, to see if we have a subsection we look:
- 1st byte is QEMU_VM_SUBSECTION
- 2nd byte is a length, and is bigger than section name
- 3rd element is a string that starts with section_name

So, we shouldn't have false positives (yes, content could still get us
wrong but probabilities are really low).

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 savevm.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 60 insertions(+), 11 deletions(-)
Anthony Liguori - Oct. 5, 2011, 7:45 p.m.
On 10/04/2011 09:38 AM, Juan Quintela wrote:
> We add qemu_peek_buffer, that is identical to qemu_get_buffer, just
> that it don't update f->buf_index.
>
> We add a paramenter to qemu_peek_byte() to be able to peek more than
> one byte.
>
> Once this is done, to see if we have a subsection we look:
> - 1st byte is QEMU_VM_SUBSECTION
> - 2nd byte is a length, and is bigger than section name
> - 3rd element is a string that starts with section_name
>
> So, we shouldn't have false positives (yes, content could still get us
> wrong but probabilities are really low).
>
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   savevm.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>   1 files changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 5fee4e2..db6ea12 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -532,6 +532,37 @@ void qemu_put_byte(QEMUFile *f, int v)
>           qemu_fflush(f);
>   }
>
> +static int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size1, int offset)
> +{
> +    int size, l;
> +    int index = f->buf_index + offset;
> +
> +    if (f->is_write) {
> +        abort();
> +    }
> +
> +    size = size1;
> +    while (size>  0) {
> +        l = f->buf_size - index;
> +        if (l == 0) {
> +            qemu_fill_buffer(f);
> +            index = f->buf_index + offset;
> +            l = f->buf_size - index;
> +            if (l == 0) {
> +                break;
> +            }
> +        }
> +        if (l>  size) {
> +            l = size;
> +        }
> +        memcpy(buf, f->buf + index, l);
> +        index += l;
> +        buf += l;
> +        size -= l;
> +    }
> +    return size1 - size;
> +}
> +
>   int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
>   {
>       int size, l;

Can we implement get_buffer in terms of peek_buffer and just increment 
f->buf_index in get_buffer?

> @@ -561,19 +592,22 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
>       return size1 - size;
>   }
>
> -static int qemu_peek_byte(QEMUFile *f)
> +static int qemu_peek_byte(QEMUFile *f, int offset)
>   {
> +    int index = f->buf_index + offset;
> +
>       if (f->is_write) {
>           abort();
>       }
>
> -    if (f->buf_index>= f->buf_size) {
> +    if (index>= f->buf_size) {
>           qemu_fill_buffer(f);
> -        if (f->buf_index>= f->buf_size) {
> +        index = f->buf_index + offset;
> +        if (index>= f->buf_size) {
>               return 0;
>           }
>       }
> -    return f->buf[f->buf_index];
> +    return f->buf[index];
>   }
>
>   int qemu_get_byte(QEMUFile *f)
> @@ -1687,22 +1721,37 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>           return 0;
>       }
>
> -    while (qemu_peek_byte(f) == QEMU_VM_SUBSECTION) {
> +    while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
>           char idstr[256];
>           int ret;
> -        uint8_t version_id, len;
> +        uint8_t version_id, len, size;
>           const VMStateDescription *sub_vmsd;
>
> -        qemu_get_byte(f); /* subsection */
> -        len = qemu_get_byte(f);
> -        qemu_get_buffer(f, (uint8_t *)idstr, len);
> -        idstr[len] = 0;
> -        version_id = qemu_get_be32(f);
> +        len = qemu_peek_byte(f, 1);
> +        if (len<  strlen(vmsd->name) + 1) {
> +            /* subsection name has be be "section_name/a" */
> +            return 0;
> +        }
> +        size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
> +        if (size != len) {
> +            return 0;
> +        }
> +        idstr[size] = 0;


Regards,

Anthony Liguori
Paolo Bonzini - Oct. 6, 2011, 8:38 a.m.
On 10/05/2011 09:45 PM, Anthony Liguori wrote:
>>
>>   int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
>>   {
>>       int size, l;
>
> Can we implement get_buffer in terms of peek_buffer and just increment
> f->buf_index in get_buffer?

No, get_buffer works even if size1 is greater than the size of the 
buffer, peek_buffer doesn't.

Paolo

Patch

diff --git a/savevm.c b/savevm.c
index 5fee4e2..db6ea12 100644
--- a/savevm.c
+++ b/savevm.c
@@ -532,6 +532,37 @@  void qemu_put_byte(QEMUFile *f, int v)
         qemu_fflush(f);
 }

+static int qemu_peek_buffer(QEMUFile *f, uint8_t *buf, int size1, int offset)
+{
+    int size, l;
+    int index = f->buf_index + offset;
+
+    if (f->is_write) {
+        abort();
+    }
+
+    size = size1;
+    while (size > 0) {
+        l = f->buf_size - index;
+        if (l == 0) {
+            qemu_fill_buffer(f);
+            index = f->buf_index + offset;
+            l = f->buf_size - index;
+            if (l == 0) {
+                break;
+            }
+        }
+        if (l > size) {
+            l = size;
+        }
+        memcpy(buf, f->buf + index, l);
+        index += l;
+        buf += l;
+        size -= l;
+    }
+    return size1 - size;
+}
+
 int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
 {
     int size, l;
@@ -561,19 +592,22 @@  int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size1)
     return size1 - size;
 }

-static int qemu_peek_byte(QEMUFile *f)
+static int qemu_peek_byte(QEMUFile *f, int offset)
 {
+    int index = f->buf_index + offset;
+
     if (f->is_write) {
         abort();
     }

-    if (f->buf_index >= f->buf_size) {
+    if (index >= f->buf_size) {
         qemu_fill_buffer(f);
-        if (f->buf_index >= f->buf_size) {
+        index = f->buf_index + offset;
+        if (index >= f->buf_size) {
             return 0;
         }
     }
-    return f->buf[f->buf_index];
+    return f->buf[index];
 }

 int qemu_get_byte(QEMUFile *f)
@@ -1687,22 +1721,37 @@  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         return 0;
     }

-    while (qemu_peek_byte(f) == QEMU_VM_SUBSECTION) {
+    while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
         char idstr[256];
         int ret;
-        uint8_t version_id, len;
+        uint8_t version_id, len, size;
         const VMStateDescription *sub_vmsd;

-        qemu_get_byte(f); /* subsection */
-        len = qemu_get_byte(f);
-        qemu_get_buffer(f, (uint8_t *)idstr, len);
-        idstr[len] = 0;
-        version_id = qemu_get_be32(f);
+        len = qemu_peek_byte(f, 1);
+        if (len < strlen(vmsd->name) + 1) {
+            /* subsection name has be be "section_name/a" */
+            return 0;
+        }
+        size = qemu_peek_buffer(f, (uint8_t *)idstr, len, 2);
+        if (size != len) {
+            return 0;
+        }
+        idstr[size] = 0;

+        if (strncmp(vmsd->name, idstr, strlen(vmsd->name)) != 0) {
+            /* it don't have a valid subsection name */
+            return 0;
+        }
         sub_vmsd = vmstate_get_subsection(sub, idstr);
         if (sub_vmsd == NULL) {
             return -ENOENT;
         }
+        qemu_get_byte(f); /* subsection */
+        qemu_get_byte(f); /* len  */
+        qemu_get_buffer(f, (uint8_t *)idstr, len);
+        idstr[len] = 0;
+        version_id = qemu_get_be32(f);
+
         assert(!sub_vmsd->subsections);
         ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
         if (ret) {