diff mbox

[4/5] qemu-file: Fix qemu_put_compression_data flaw

Message ID 1462333259-3237-5-git-send-email-liang.z.li@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liang Li May 4, 2016, 3:40 a.m. UTC
Current qemu_put_compression_data can only work with no writable
QEMUFile, and can't work with the writable QEMUFile. But it does
not provide any measure to prevent users from using it with a
writable QEMUFile.

We should fix this flaw to make it works with writable QEMUFile.

Suggested-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/qemu-file.c | 23 +++++++++++++++++++++--
 migration/ram.c       |  6 +++++-
 2 files changed, 26 insertions(+), 3 deletions(-)

Comments

Juan Quintela May 4, 2016, 9:30 a.m. UTC | #1
Liang Li <liang.z.li@intel.com> wrote:
> Current qemu_put_compression_data can only work with no writable
> QEMUFile, and can't work with the writable QEMUFile. But it does
> not provide any measure to prevent users from using it with a
> writable QEMUFile.
>
> We should fix this flaw to make it works with writable QEMUFile.
>
> Suggested-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>

Code is not enough.

> ---
>  migration/qemu-file.c | 23 +++++++++++++++++++++--
>  migration/ram.c       |  6 +++++-
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 6f4a129..b0ef1f3 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f)
>      return v;
>  }
>  
> -/* compress size bytes of data start at p with specific compression
> +/* Compress size bytes of data start at p with specific compression
>   * level and store the compressed data to the buffer of f.
> + *
> + * When f is not writable, return -1 if f has no space to save the
> + * compressed data.
> + * When f is wirtable and it has no space to save the compressed data,
> + * do fflush first, if f still has no space to save the compressed
> + * data, return -1.
>   */
>  
>  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
> @@ -617,7 +623,14 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>  
>      if (blen < compressBound(size)) {
> -        return 0;
> +        if (!qemu_file_is_writable(f)) {
> +            return -1;
> +        }

I can't yet understand why we can be writting to a qemu_file that is not writable.
But well, no harm.


> +        qemu_fflush(f);
> +        blen = IO_BUF_SIZE - sizeof(int32_t);
> +        if (blen < compressBound(size)) {
> +            return -1;
> +        }
>      }
>      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
>                    (Bytef *)p, size, level) != Z_OK) {
> @@ -625,7 +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>          return 0;
>      }
>      qemu_put_be32(f, blen);
> +    if (f->ops->writev_buffer) {
> +        add_to_iovec(f, f->buf + f->buf_index, blen);
> +    }



> +    }
>      return blen + sizeof(int32_t);
>  }
>  

Ok with the changes in this function.


> diff --git a/migration/ram.c b/migration/ram.c
> index bc34bc5..7e62d8d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -821,7 +821,11 @@ static int do_compress_ram_page(CompressParam *param)
>                                    RAM_SAVE_FLAG_COMPRESS_PAGE);
>      blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE,
>                                       migrate_compress_level());
> -    bytes_sent += blen;
> +    if (blen < 0) {
> +        error_report("Insufficient buffer for compressed data!");
> +    } else {
> +        bytes_sent += blen;
> +    }
>  
>      return bytes_sent;
>  }


Are you sure that this code is ok?

1st- there are two callers od do_compress_ram_page(), only one of it
     uses the return value
2nd- if we were not able to write the compressed page, we continue
     without a single error.  I think that if blen is < 0, we should
     just abort the migration at this point (we have just sent a header
     and we haven't been able to send the contents under that header.
     Without lot of changes, I can't see how to fix it).

Thanks, Juan.
Liang Li May 4, 2016, 2:32 p.m. UTC | #2
> Liang Li <liang.z.li@intel.com> wrote:
> > Current qemu_put_compression_data can only work with no writable
> > QEMUFile, and can't work with the writable QEMUFile. But it does not
> > provide any measure to prevent users from using it with a writable
> > QEMUFile.
> >
> > We should fix this flaw to make it works with writable QEMUFile.
> >
> > Suggested-by: Juan Quintela <quintela@redhat.com>
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> 
> Code is not enough.
> 
> > ---
> >  migration/qemu-file.c | 23 +++++++++++++++++++++--
> >  migration/ram.c       |  6 +++++-
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c index
> > 6f4a129..b0ef1f3 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f)
> >      return v;
> >  }
> >
> > -/* compress size bytes of data start at p with specific compression
> > +/* Compress size bytes of data start at p with specific compression
> >   * level and store the compressed data to the buffer of f.
> > + *
> > + * When f is not writable, return -1 if f has no space to save the
> > + * compressed data.
> > + * When f is wirtable and it has no space to save the compressed
> > + data,
> > + * do fflush first, if f still has no space to save the compressed
> > + * data, return -1.
> >   */
> >
> >  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p,
> > size_t size, @@ -617,7 +623,14 @@ ssize_t
> qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
> >      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
> >
> >      if (blen < compressBound(size)) {
> > -        return 0;
> > +        if (!qemu_file_is_writable(f)) {
> > +            return -1;
> > +        }
> 
> I can't yet understand why we can be writting to a qemu_file that is not
> writable.
> But well, no harm.
> 
> 
> > +        qemu_fflush(f);
> > +        blen = IO_BUF_SIZE - sizeof(int32_t);
> > +        if (blen < compressBound(size)) {
> > +            return -1;
> > +        }
> >      }
> >      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
> >                    (Bytef *)p, size, level) != Z_OK) { @@ -625,7
> > +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const
> uint8_t *p, size_t size,
> >          return 0;
> >      }
> >      qemu_put_be32(f, blen);
> > +    if (f->ops->writev_buffer) {
> > +        add_to_iovec(f, f->buf + f->buf_index, blen);
> > +    }
> 
> 
> 
> > +    }
> >      return blen + sizeof(int32_t);
> >  }
> >
> 
> Ok with the changes in this function.
> 
> 
> > diff --git a/migration/ram.c b/migration/ram.c index bc34bc5..7e62d8d
> > 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -821,7 +821,11 @@ static int
> do_compress_ram_page(CompressParam *param)
> >                                    RAM_SAVE_FLAG_COMPRESS_PAGE);
> >      blen = qemu_put_compression_data(param->file, p,
> TARGET_PAGE_SIZE,
> >                                       migrate_compress_level());
> > -    bytes_sent += blen;
> > +    if (blen < 0) {
> > +        error_report("Insufficient buffer for compressed data!");
> > +    } else {
> > +        bytes_sent += blen;
> > +    }
> >
> >      return bytes_sent;
> >  }
> 
> 
> Are you sure that this code is ok?

No.
> 
> 1st- there are two callers od do_compress_ram_page(), only one of it
>      uses the return value
> 2nd- if we were not able to write the compressed page, we continue
>      without a single error.  I think that if blen is < 0, we should
>      just abort the migration at this point (we have just sent a header
>      and we haven't been able to send the contents under that header.
>      Without lot of changes, I can't see how to fix it).
> 

Abort the migration is the right choice,  use qemu_file_set_error() to set the error flag is enough? 
Thanks

Liang

> Thanks, Juan.
diff mbox

Patch

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 6f4a129..b0ef1f3 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -607,8 +607,14 @@  uint64_t qemu_get_be64(QEMUFile *f)
     return v;
 }
 
-/* compress size bytes of data start at p with specific compression
+/* Compress size bytes of data start at p with specific compression
  * level and store the compressed data to the buffer of f.
+ *
+ * When f is not writable, return -1 if f has no space to save the
+ * compressed data.
+ * When f is wirtable and it has no space to save the compressed data,
+ * do fflush first, if f still has no space to save the compressed
+ * data, return -1.
  */
 
 ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
@@ -617,7 +623,14 @@  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
     ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
 
     if (blen < compressBound(size)) {
-        return 0;
+        if (!qemu_file_is_writable(f)) {
+            return -1;
+        }
+        qemu_fflush(f);
+        blen = IO_BUF_SIZE - sizeof(int32_t);
+        if (blen < compressBound(size)) {
+            return -1;
+        }
     }
     if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
                   (Bytef *)p, size, level) != Z_OK) {
@@ -625,7 +638,13 @@  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
         return 0;
     }
     qemu_put_be32(f, blen);
+    if (f->ops->writev_buffer) {
+        add_to_iovec(f, f->buf + f->buf_index, blen);
+    }
     f->buf_index += blen;
+    if (f->buf_index == IO_BUF_SIZE) {
+        qemu_fflush(f);
+    }
     return blen + sizeof(int32_t);
 }
 
diff --git a/migration/ram.c b/migration/ram.c
index bc34bc5..7e62d8d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -821,7 +821,11 @@  static int do_compress_ram_page(CompressParam *param)
                                   RAM_SAVE_FLAG_COMPRESS_PAGE);
     blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE,
                                      migrate_compress_level());
-    bytes_sent += blen;
+    if (blen < 0) {
+        error_report("Insufficient buffer for compressed data!");
+    } else {
+        bytes_sent += blen;
+    }
 
     return bytes_sent;
 }