multifd_file_recv_data() stores the return value of qio_channel_pread()
(ssize_t) in a size_t variable.  On I/O error the -1 return value wraps
to SIZE_MAX, producing a nonsensical read size in the error message.

More critically, a short read (0 <= ret < data->size) is possible when
the migration file is truncated.  In that case qio_channel_pread()
returns a non-negative value without setting *errp.  The function then
calls error_prepend(errp, ...) which dereferences *errp -- a NULL
pointer -- crashing QEMU.

Fix both issues by changing ret to ssize_t and splitting the error
handling: use error_prepend() only when qio_channel_pread() itself
has populated *errp (ret < 0), and error_setg() for the short-read
case where *errp has not been set.  Add ERRP_GUARD() so that
error_prepend() works correctly even when errp is &error_fatal or
NULL.

Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 migration/file.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/migration/file.c b/migration/file.c
index 5618aced49..78b274dc32 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -254,15 +254,21 @@ int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
 
 int multifd_file_recv_data(MultiFDRecvParams *p, Error **errp)
 {
+    ERRP_GUARD();
     MultiFDRecvData *data = p->data;
-    size_t ret;
+    ssize_t ret;
 
     ret = qio_channel_pread(p->c, (char *) data->opaque,
                             data->size, data->file_offset, errp);
+    if (ret < 0) {
+        error_prepend(errp, "multifd recv (%u): ", p->id);
+        return -1;
+    }
+
     if (ret != data->size) {
-        error_prepend(errp,
-                      "multifd recv (%u): read 0x%zx, expected 0x%zx",
-                      p->id, ret, data->size);
+        error_setg(errp,
+                   "multifd recv (%u): read 0x%zx, expected 0x%zx",
+                   p->id, (size_t)ret, data->size);
         return -1;
     }
 
-- 
2.43.0

On Mon, Mar 16, 2026 at 04:46:18PM +0800, Junjie Cao wrote:
> multifd_file_recv_data() stores the return value of qio_channel_pread()
> (ssize_t) in a size_t variable.  On I/O error the -1 return value wraps
> to SIZE_MAX, producing a nonsensical read size in the error message.
> 
> More critically, a short read (0 <= ret < data->size) is possible when
> the migration file is truncated.  In that case qio_channel_pread()
> returns a non-negative value without setting *errp.  The function then
> calls error_prepend(errp, ...) which dereferences *errp -- a NULL
> pointer -- crashing QEMU.
> 
> Fix both issues by changing ret to ssize_t and splitting the error
> handling: use error_prepend() only when qio_channel_pread() itself
> has populated *errp (ret < 0), and error_setg() for the short-read
> case where *errp has not been set.  Add ERRP_GUARD() so that
> error_prepend() works correctly even when errp is &error_fatal or
> NULL.

Indeed, this seems problematic.

But is it possible to get partial reads?  I don't see why it won't happen..
do we need to fix it too (e.g. introduce qio_channel_pread_all_eof())?

CC Dan.

Thanks,

> 
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>  migration/file.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/file.c b/migration/file.c
> index 5618aced49..78b274dc32 100644
> --- a/migration/file.c
> +++ b/migration/file.c
> @@ -254,15 +254,21 @@ int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
>  
>  int multifd_file_recv_data(MultiFDRecvParams *p, Error **errp)
>  {
> +    ERRP_GUARD();
>      MultiFDRecvData *data = p->data;
> -    size_t ret;
> +    ssize_t ret;
>  
>      ret = qio_channel_pread(p->c, (char *) data->opaque,
>                              data->size, data->file_offset, errp);
> +    if (ret < 0) {
> +        error_prepend(errp, "multifd recv (%u): ", p->id);
> +        return -1;
> +    }
> +
>      if (ret != data->size) {
> -        error_prepend(errp,
> -                      "multifd recv (%u): read 0x%zx, expected 0x%zx",
> -                      p->id, ret, data->size);
> +        error_setg(errp,
> +                   "multifd recv (%u): read 0x%zx, expected 0x%zx",
> +                   p->id, (size_t)ret, data->size);
>          return -1;
>      }
>  
> -- 
> 2.43.0
> 

-- 
Peter Xu

On Mon, Mar 16, 2026 at 04:41:24PM -0400, Peter Xu wrote:
> On Mon, Mar 16, 2026 at 04:46:18PM +0800, Junjie Cao wrote:
> > multifd_file_recv_data() stores the return value of qio_channel_pread()
> > (ssize_t) in a size_t variable.  On I/O error the -1 return value wraps
> > to SIZE_MAX, producing a nonsensical read size in the error message.
> > 
> > More critically, a short read (0 <= ret < data->size) is possible when
> > the migration file is truncated.  In that case qio_channel_pread()
> > returns a non-negative value without setting *errp.  The function then
> > calls error_prepend(errp, ...) which dereferences *errp -- a NULL
> > pointer -- crashing QEMU.
> > 
> > Fix both issues by changing ret to ssize_t and splitting the error
> > handling: use error_prepend() only when qio_channel_pread() itself
> > has populated *errp (ret < 0), and error_setg() for the short-read
> > case where *errp has not been set.  Add ERRP_GUARD() so that
> > error_prepend() works correctly even when errp is &error_fatal or
> > NULL.
> 
> Indeed, this seems problematic.
> 
> But is it possible to get partial reads?  I don't see why it won't happen..
> do we need to fix it too (e.g. introduce qio_channel_pread_all_eof())?

It is wise to assume a short read is possible. Even if it might be
safe today, based on assumptions of the callers, future refactoring
may change things.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|