diff mbox

[6/6] migration: catch unknown flags in ram_load

Message ID 53A00AEF.2090304@kamp.de
State New
Headers show

Commit Message

Peter Lieven June 17, 2014, 9:31 a.m. UTC 
On 16.06.2014 19:31, Eric Blake wrote:
> On 06/15/2014 09:15 PM, Juan Quintela wrote:
>> From: Peter Lieven <pl@kamp.de>
>>
>> if a saved vm has unknown flags in the memory data qemu
>> currently simply ignores this flag and continues which
>> yields in an unpredictable result.
>>
>> This patch catches all unknown flags and aborts the
>> loading of the vm. Additionally error reports are thrown
>> if the migration aborts abnormally.
> This patch is a strict improvement, so I'm glad it went in.  However, I
> still feel that we aren't doing a good job of silently ignoring
> unexpected combinations of flag bits, and had suggestions in the
> original thread on further followups that are worth having before the
> 2.1 release.
>
>> -
>> -        if (flags & RAM_SAVE_FLAG_COMPRESS) {
>> +        } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
>>               void *host;
> Among other things, switching from a chain of if-else to a switch might
> make it easier to document explicit supported combinations of flags and
> reject other values from an invalid stream.
>

Is this what you have in mind?

Comments

Eric Blake June 17, 2014, 8:32 p.m. UTC | #1 
On 06/17/2014 03:31 AM, Peter Lieven wrote:
>> Among other things, switching from a chain of if-else to a switch might
>> make it easier to document explicit supported combinations of flags and
>> reject other values from an invalid stream.
>>
> 
> Is this what you have in mind?
> 
> diff --git a/arch_init.c b/arch_init.c
> index 8ddaf35..925cc66 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1039,7 +1039,7 @@ void ram_handle_compressed(void *host, uint8_t ch,
> uint64_t size)
>  static int ram_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      ram_addr_t addr;
> -    int flags, ret = 0;
> +    int flags = 0, ret = 0;
>      static uint64_t seq_iter;
> 
>      seq_iter++;
> @@ -1048,97 +1048,96 @@ static int ram_load(QEMUFile *f, void *opaque,
> int version_id)
>          ret = -EINVAL;
>      }
> 
> -    while (!ret) {
> +    while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {

> -                            ret =  -EINVAL;
> +        if (flags & RAM_SAVE_FLAG_HOOK) {
> +            ram_control_load_hook(f, flags);

Can RAM_SAVE_FLAG_HOOK | RAM_SAVE_FLAG_EOS ever be true?  If so, you
abort the loop early. (One possibility: solve it by adding 'flags = 0;'
here.)

Also, this patch was harder to read than necessary, because it
reindented everything, and threw off git's diff algorithm.  Use 'git
diff --patience' to make it a bit more legible as a bulk delete/replace
instead of dividing into hunks around common but unrelated context
lines.  Or even rewrite it slightly so that the bulk of the code is
still at the same indentation, perhaps by rearranging this if statement
to instead appear inside the default label of the switch:

while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
    flags = ...;
    switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
 ...
    default:
        if (flags & RAM_SAVE_FLAG_HOOK) {
            ram_control_load_hook(f, flags);
            flags = 0;
        } else {
            report error on unknown flags
        }
        break;
    }
    ret = qemu_file_get_error(f);
}

But yes, this is what I had in mind - propose it as a formal patch with
S-o-B, and I'll review it.
diff mbox

Patch

diff --git a/arch_init.c b/arch_init.c
index 8ddaf35..925cc66 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1039,7 +1039,7 @@  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size)
  static int ram_load(QEMUFile *f, void *opaque, int version_id)
  {
      ram_addr_t addr;
-    int flags, ret = 0;
+    int flags = 0, ret = 0;
      static uint64_t seq_iter;

      seq_iter++;
@@ -1048,97 +1048,96 @@  static int ram_load(QEMUFile *f, void *opaque, int version_id)
          ret = -EINVAL;
      }

-    while (!ret) {
+    while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
          addr = qemu_get_be64(f);

          flags = addr & ~TARGET_PAGE_MASK;
          addr &= TARGET_PAGE_MASK;

-        if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
-            /* Synchronize RAM block list */
-            char id[256];
-            ram_addr_t length;
-            ram_addr_t total_ram_bytes = addr;
-
-            while (total_ram_bytes) {
-                RAMBlock *block;
-                uint8_t len;
-
-                len = qemu_get_byte(f);
-                qemu_get_buffer(f, (uint8_t *)id, len);
-                id[len] = 0;
-                length = qemu_get_be64(f);
-
-                QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-                    if (!strncmp(id, block->idstr, sizeof(id))) {
-                        if (block->length != length) {
-                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
-                                         " in != " RAM_ADDR_FMT, id, length,
-                                         block->length);
-                            ret =  -EINVAL;
+        if (flags & RAM_SAVE_FLAG_HOOK) {
+            ram_control_load_hook(f, flags);
+        } else {
+            ram_addr_t total_ram_bytes;
+            void *host;
+            uint8_t ch;
+            switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
+            case RAM_SAVE_FLAG_MEM_SIZE:
+                /* Synchronize RAM block list */
+                total_ram_bytes = addr;
+                while (total_ram_bytes) {
+                    RAMBlock *block;
+                    uint8_t len;
+                    char id[256];
+                    ram_addr_t length;
+
+                    len = qemu_get_byte(f);
+                    qemu_get_buffer(f, (uint8_t *)id, len);
+                    id[len] = 0;
+                    length = qemu_get_be64(f);
+
+                    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
+                        if (!strncmp(id, block->idstr, sizeof(id))) {
+                            if (block->length != length) {
+                                error_report("Length mismatch: %s: " RAM_ADDR_FMT
+                                             " in != " RAM_ADDR_FMT, id, length,
+                                             block->length);
+                                ret =  -EINVAL;
+                            }
+                            continue;
                          }
-                        break;
                      }
+                    if (!block) {
+                        error_report("Unknown ramblock \"%s\", cannot "
+                                     "accept migration", id);
+                        ret = -EINVAL;
+                    }
+                    if (ret) {
+                        continue;
+                    }
+                    total_ram_bytes -= length;
                  }
-
-                if (!block) {
-                    error_report("Unknown ramblock \"%s\", cannot "
-                                 "accept migration", id);
+                break;
+            case RAM_SAVE_FLAG_COMPRESS:
+                host = host_from_stream_offset(f, addr, flags);
+                if (!host) {
+                    error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
                      ret = -EINVAL;
+                    continue;
                  }
-                if (ret) {
-                    break;
-                }
-
-                total_ram_bytes -= length;
-            }
-        } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
-            void *host;
-            uint8_t ch;
-
-            host = host_from_stream_offset(f, addr, flags);
-            if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
-                ret = -EINVAL;
+                ch = qemu_get_byte(f);
+                ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
                  break;
-            }
-
-            ch = qemu_get_byte(f);
-            ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
-        } else if (flags & RAM_SAVE_FLAG_PAGE) {
-            void *host;
-
-            host = host_from_stream_offset(f, addr, flags);
-            if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
-                ret = -EINVAL;
+            case RAM_SAVE_FLAG_PAGE:
+                host = host_from_stream_offset(f, addr, flags);
+                if (!host) {
+                    error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+                    ret = -EINVAL;
+                    continue;
+                }
+                qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
                  break;
-            }
-
-            qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
-        } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
-            void *host = host_from_stream_offset(f, addr, flags);
-            if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
-                ret = -EINVAL;
+            case RAM_SAVE_FLAG_XBZRLE:
+                host = host_from_stream_offset(f, addr, flags);
+                if (!host) {
+                    error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+                    ret = -EINVAL;
+                    continue;
+                }
+                if (load_xbzrle(f, addr, host) < 0) {
+                    error_report("Failed to decompress XBZRLE page at "
+                                 RAM_ADDR_FMT, addr);
+                    ret = -EINVAL;
+                    continue;
+                }
                  break;
-            }
-
-            if (load_xbzrle(f, addr, host) < 0) {
-                error_report("Failed to decompress XBZRLE page at "
-                             RAM_ADDR_FMT, addr);
+            case RAM_SAVE_FLAG_EOS:
+                /* normal exit */
+                continue;
+            default:
+                error_report("Unknown migration flags: %#x", flags);
                  ret = -EINVAL;
-                break;
+                continue;
              }
-        } else if (flags & RAM_SAVE_FLAG_HOOK) {
-            ram_control_load_hook(f, flags);
-        } else if (flags & RAM_SAVE_FLAG_EOS) {
-            /* normal exit */
-            break;
-        } else {
-            error_report("Unknown migration flags: %#x", flags);
-            ret = -EINVAL;
-            break;
          }
          ret = qemu_file_get_error(f);
      }