[PATCH] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[PATCH] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Junjie Cao]

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

Re: [PATCH] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Peter Xu]

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

Re: [PATCH] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Daniel P. Berrangé]

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 :|

[PATCH v2 0/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Junjie Cao]

multifd_file_recv_data() has two bugs: a size_t/ssize_t type mismatch
that garbles error messages, and a NULL pointer dereference when a
short read occurs on a truncated migration file.

v1 fixed the symptoms by splitting the error handling.  Peter and
Daniel pointed out that short reads should be retried rather than
just reported, so v2 introduces qio_channel_pread{v,}_all() and
preadv_all_eof() (following the existing read_all pattern), uses them
in the migration code, and adds unit tests covering the key semantics.

Note: qemu_get_buffer_at() in migration/qemu-file.c has a similar
type mismatch and does not retry on short reads either.  That will
be addressed in a follow-up series.

v1: https://lore.kernel.org/qemu-devel/20260316084618.52-1-junjie.cao@intel.com/

v1 -> v2:
  - [NEW] Patch 1/3: introduce qio_channel_pread{v,}_all() and
    preadv_all_eof() helpers in io/channel that retry on short reads,
    advancing the file offset automatically (suggested by Peter Xu).
    All three are marked coroutine_mixed_fn, consistent with
    existing _all helpers.
  - Patch 2/3: switch multifd_file_recv_data() from qio_channel_pread()
    to qio_channel_pread_all(), eliminating the manual byte-count check
    and fixing both the type mismatch and the NULL deref.
  - [NEW] Patch 3/3: add five unit tests for the new pread_all
    helpers covering success, clean EOF, partial EOF, and the
    strict-wrapper EOF-as-error semantics.

Junjie Cao (3):
  io/channel: introduce qio_channel_pread{v,}_all() and preadv_all_eof()
  migration/file: fix type mismatch and NULL deref in
    multifd_file_recv_data
  tests/unit: add pread_all and preadv_all tests for io channel file

 include/io/channel.h              |  63 ++++++++++++++
 io/channel.c                      |  85 ++++++++++++++++++
 migration/file.c                  |  17 ++--
 tests/unit/test-io-channel-file.c | 137 ++++++++++++++++++++++++++++++
 4 files changed, 293 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] io/channel: introduce qio_channel_pread{v, }_all() and preadv_all_eof()

[Junjie Cao]

qio_channel_pread() and qio_channel_preadv() perform a single read
and may return a short result.  Callers that need all bytes currently
have to open-code a retry loop or simply treat a short read as an
error.

Introduce three new helpers following the existing read_all / readv_all
pattern:

  qio_channel_preadv_all_eof()  – retry loop; returns 1 on success,
                                   0 on clean EOF, -1 on error.
  qio_channel_preadv_all()      – wraps _eof; treats early EOF as
                                   error; returns 0 / -1.
  qio_channel_pread_all()       – single-buffer convenience wrapper
                                   around preadv_all().

These advance the file offset internally after each partial read, so
callers only need a single call.  All three are marked
coroutine_mixed_fn, consistent with the existing _all helpers.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 include/io/channel.h | 63 ++++++++++++++++++++++++++++++++
 io/channel.c         | 85 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+)

diff --git a/include/io/channel.h b/include/io/channel.h
index 1b02350437..d5b14c8c77 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -634,6 +634,69 @@ ssize_t qio_channel_preadv(QIOChannel *ioc, const struct iovec *iov,
 ssize_t qio_channel_pread(QIOChannel *ioc, void *buf, size_t buflen,
                           off_t offset, Error **errp);
 
+/**
+ * qio_channel_preadv_all_eof:
+ * @ioc: the channel object
+ * @iov: the array of memory regions to read data into
+ * @niov: the length of the @iov array
+ * @offset: the starting offset in the channel to read from
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Reads data contiguously from the channel into @iov starting at
+ * @offset, retrying on short reads until all requested bytes have
+ * been read.  The offset is advanced internally after each partial
+ * read.
+ *
+ * Returns: 1 if all bytes were read, 0 if no data could be read
+ *          before encountering end-of-file, or -1 on error
+ */
+int coroutine_mixed_fn qio_channel_preadv_all_eof(QIOChannel *ioc,
+                                                  const struct iovec *iov,
+                                                  size_t niov,
+                                                  off_t offset,
+                                                  Error **errp);
+
+/**
+ * qio_channel_preadv_all:
+ * @ioc: the channel object
+ * @iov: the array of memory regions to read data into
+ * @niov: the length of the @iov array
+ * @offset: the starting offset in the channel to read from
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Reads data contiguously from the channel into @iov starting at
+ * @offset, retrying on short reads until all requested bytes have
+ * been read.  Unlike qio_channel_preadv_all_eof(), end-of-file
+ * before all data has been read is treated as an error.
+ *
+ * Returns: 0 if all bytes were read, or -1 on error
+ */
+int coroutine_mixed_fn qio_channel_preadv_all(QIOChannel *ioc,
+                                              const struct iovec *iov,
+                                              size_t niov,
+                                              off_t offset,
+                                              Error **errp);
+
+/**
+ * qio_channel_pread_all:
+ * @ioc: the channel object
+ * @buf: the memory region to read data into
+ * @buflen: the number of bytes to read into @buf
+ * @offset: the starting offset in the channel to read from
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Reads @buflen bytes contiguously from the channel at @offset
+ * into @buf, retrying on short reads.  End-of-file before all data
+ * has been read is treated as an error.
+ *
+ * Returns: 0 if all bytes were read, or -1 on error
+ */
+int coroutine_mixed_fn qio_channel_pread_all(QIOChannel *ioc,
+                                             void *buf,
+                                             size_t buflen,
+                                             off_t offset,
+                                             Error **errp);
+
 /**
  * qio_channel_shutdown:
  * @ioc: the channel object
diff --git a/io/channel.c b/io/channel.c
index cc02d997a4..e18c40aa0b 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -507,6 +507,91 @@ ssize_t qio_channel_pread(QIOChannel *ioc, void *buf, size_t buflen,
     return qio_channel_preadv(ioc, &iov, 1, offset, errp);
 }
 
+int coroutine_mixed_fn qio_channel_preadv_all_eof(QIOChannel *ioc,
+                                                  const struct iovec *iov,
+                                                  size_t niov,
+                                                  off_t offset,
+                                                  Error **errp)
+{
+    int ret = -1;
+    struct iovec *local_iov = g_new(struct iovec, niov);
+    struct iovec *local_iov_head = local_iov;
+    unsigned int nlocal_iov = niov;
+    bool partial = false;
+
+    nlocal_iov = iov_copy(local_iov, nlocal_iov,
+                          iov, niov,
+                          0, iov_size(iov, niov));
+
+    while (nlocal_iov > 0) {
+        ssize_t len;
+        len = qio_channel_preadv(ioc, local_iov, nlocal_iov, offset, errp);
+
+        if (len == QIO_CHANNEL_ERR_BLOCK) {
+            qio_channel_wait_cond(ioc, G_IO_IN);
+            continue;
+        }
+
+        if (len == 0) {
+            if (!partial) {
+                ret = 0;
+                goto cleanup;
+            }
+            error_setg(errp,
+                       "Unexpected end-of-file before all data were read");
+            goto cleanup;
+        }
+
+        if (len < 0) {
+            goto cleanup;
+        }
+
+        partial = true;
+        offset += len;
+        iov_discard_front(&local_iov, &nlocal_iov, len);
+    }
+
+    ret = 1;
+
+ cleanup:
+    g_free(local_iov_head);
+    return ret;
+}
+
+int coroutine_mixed_fn qio_channel_preadv_all(QIOChannel *ioc,
+                                              const struct iovec *iov,
+                                              size_t niov,
+                                              off_t offset,
+                                              Error **errp)
+{
+    int ret = qio_channel_preadv_all_eof(ioc, iov, niov, offset, errp);
+
+    if (ret == 0) {
+        error_setg(errp,
+                   "Unexpected end-of-file before all data were read");
+        return -1;
+    }
+    if (ret == 1) {
+        return 0;
+    }
+
+    return ret;
+}
+
+int coroutine_mixed_fn qio_channel_pread_all(QIOChannel *ioc,
+                                             void *buf,
+                                             size_t buflen,
+                                             off_t offset,
+                                             Error **errp)
+{
+    struct iovec iov = {
+        .iov_base = buf,
+        .iov_len = buflen
+    };
+
+    return qio_channel_preadv_all(ioc, &iov, 1, offset, errp);
+}
+
 int qio_channel_shutdown(QIOChannel *ioc,
                          QIOChannelShutdown how,
                          Error **errp)
-- 
2.43.0

[PATCH v2 2/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Junjie Cao]

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 switching to qio_channel_pread_all() introduced in
the previous commit, which retries on short reads and treats
end-of-file as an error, so the caller no longer needs to check the
byte count manually.  Add ERRP_GUARD() so that error_prepend() works
correctly even when errp is &error_fatal or NULL.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 migration/file.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/migration/file.c b/migration/file.c
index 5618aced49..b0d3affa4c 100644
--- a/migration/file.c
+++ b/migration/file.c
@@ -254,16 +254,15 @@ 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;
-
-    ret = qio_channel_pread(p->c, (char *) data->opaque,
-                            data->size, data->file_offset, errp);
-    if (ret != data->size) {
-        error_prepend(errp,
-                      "multifd recv (%u): read 0x%zx, expected 0x%zx",
-                      p->id, ret, data->size);
-        return -1;
+    int ret;
+
+    ret = qio_channel_pread_all(p->c, (char *) data->opaque,
+                                data->size, data->file_offset, errp);
+    if (ret < 0) {
+        error_prepend(errp, "multifd recv (%u): ", p->id);
+        return ret;
     }
 
     return 0;
-- 
2.43.0

[PATCH v2 3/3] tests/unit: add pread_all and preadv_all tests for io channel file

[Junjie Cao]

Add five targeted test cases for the new qio_channel_pread{v,}_all()
and preadv_all_eof() helpers introduced earlier in this series:

  preadv_all:          read from a non-zero offset into two iovecs
  pread_all:           single-buffer wrapper smoke test
  preadv_all_eof/clean:   clean EOF (offset == file length) returns 0
  preadv_all_eof/partial: partial data then EOF returns -1
  preadv_all/eof-is-error: strict wrapper turns clean EOF into -1

All tests create a 16-byte file with known content and exercise the
read helpers against it, verifying return values, error state, and
(where applicable) buffer contents.  The tests are guarded by
CONFIG_PREADV since the underlying preadv support is optional.

Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 tests/unit/test-io-channel-file.c | 137 ++++++++++++++++++++++++++++++
 1 file changed, 137 insertions(+)

diff --git a/tests/unit/test-io-channel-file.c b/tests/unit/test-io-channel-file.c
index 1977006ce9..97a2a5b5cb 100644
--- a/tests/unit/test-io-channel-file.c
+++ b/tests/unit/test-io-channel-file.c
@@ -102,6 +102,131 @@ static void test_io_channel_fd(void)
 }
 
 
+#ifdef CONFIG_PREADV
+#define TEST_PREAD_FILE "tests/test-io-channel-pread.txt"
+#define TEST_PREAD_PATTERN "ABCDEFGHIJKLMNOP"  /* 16 bytes */
+#define TEST_PREAD_LEN 16
+
+static QIOChannel *create_pread_test_file(void)
+{
+    int fd;
+
+    fd = open(TEST_PREAD_FILE, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+    g_assert_cmpint(fd, >=, 0);
+    g_assert_cmpint(write(fd, TEST_PREAD_PATTERN, TEST_PREAD_LEN),
+                    ==, TEST_PREAD_LEN);
+    close(fd);
+
+    return QIO_CHANNEL(qio_channel_file_new_path(
+                           TEST_PREAD_FILE, O_RDONLY, 0,
+                           &error_abort));
+}
+
+static void test_io_channel_preadv_all(void)
+{
+    QIOChannel *ioc;
+    char buf1[4], buf2[4];
+    struct iovec iov[2] = {
+        { .iov_base = buf1, .iov_len = sizeof(buf1) },
+        { .iov_base = buf2, .iov_len = sizeof(buf2) },
+    };
+    int ret;
+
+    ioc = create_pread_test_file();
+
+    /* Read 8 bytes from offset 4 into two iovecs: "EFGH" + "IJKL" */
+    ret = qio_channel_preadv_all(ioc, iov, 2, 4, &error_abort);
+    g_assert_cmpint(ret, ==, 0);
+    g_assert_cmpmem(buf1, 4, "EFGH", 4);
+    g_assert_cmpmem(buf2, 4, "IJKL", 4);
+
+    unlink(TEST_PREAD_FILE);
+    object_unref(OBJECT(ioc));
+}
+
+static void test_io_channel_pread_all(void)
+{
+    QIOChannel *ioc;
+    char buf[8];
+    int ret;
+
+    ioc = create_pread_test_file();
+
+    /* Read 8 bytes from offset 8: "IJKLMNOP" */
+    ret = qio_channel_pread_all(ioc, buf, sizeof(buf), 8, &error_abort);
+    g_assert_cmpint(ret, ==, 0);
+    g_assert_cmpmem(buf, 8, "IJKLMNOP", 8);
+
+    unlink(TEST_PREAD_FILE);
+    object_unref(OBJECT(ioc));
+}
+
+static void test_io_channel_preadv_all_eof_clean(void)
+{
+    QIOChannel *ioc;
+    Error *err = NULL;
+    char buf[8];
+    struct iovec iov = { .iov_base = buf, .iov_len = sizeof(buf) };
+    int ret;
+
+    ioc = create_pread_test_file();
+
+    /* Read from offset == file length: clean EOF, expect 0 and no error */
+    ret = qio_channel_preadv_all_eof(ioc, &iov, 1, TEST_PREAD_LEN, &err);
+    g_assert_cmpint(ret, ==, 0);
+    g_assert_null(err);
+
+    unlink(TEST_PREAD_FILE);
+    object_unref(OBJECT(ioc));
+}
+
+static void test_io_channel_preadv_all_eof_partial(void)
+{
+    QIOChannel *ioc;
+    Error *err = NULL;
+    char buf[8];
+    struct iovec iov = { .iov_base = buf, .iov_len = sizeof(buf) };
+    int ret;
+
+    ioc = create_pread_test_file();
+
+    /*
+     * Read 8 bytes from offset 12: only 4 bytes available before EOF.
+     * Expect -1 (partial data then EOF is an error) and err set.
+     */
+    ret = qio_channel_preadv_all_eof(ioc, &iov, 1, 12, &err);
+    g_assert_cmpint(ret, ==, -1);
+    g_assert_nonnull(err);
+    error_free(err);
+
+    unlink(TEST_PREAD_FILE);
+    object_unref(OBJECT(ioc));
+}
+
+static void test_io_channel_preadv_all_eof_is_error(void)
+{
+    QIOChannel *ioc;
+    Error *err = NULL;
+    char buf[8];
+    struct iovec iov = { .iov_base = buf, .iov_len = sizeof(buf) };
+    int ret;
+
+    ioc = create_pread_test_file();
+
+    /*
+     * Clean EOF through the strict wrapper: should be translated to -1.
+     */
+    ret = qio_channel_preadv_all(ioc, &iov, 1, TEST_PREAD_LEN, &err);
+    g_assert_cmpint(ret, ==, -1);
+    g_assert_nonnull(err);
+    error_free(err);
+
+    unlink(TEST_PREAD_FILE);
+    object_unref(OBJECT(ioc));
+}
+#endif /* CONFIG_PREADV */
+
+
 #ifndef _WIN32
 static void test_io_channel_pipe(bool async)
 {
@@ -147,6 +272,18 @@ int main(int argc, char **argv)
     g_test_add_func("/io/channel/file", test_io_channel_file);
     g_test_add_func("/io/channel/file/rdwr", test_io_channel_file_rdwr);
     g_test_add_func("/io/channel/file/fd", test_io_channel_fd);
+#ifdef CONFIG_PREADV
+    g_test_add_func("/io/channel/file/preadv-all",
+                    test_io_channel_preadv_all);
+    g_test_add_func("/io/channel/file/pread-all",
+                    test_io_channel_pread_all);
+    g_test_add_func("/io/channel/file/preadv-all-eof/clean",
+                    test_io_channel_preadv_all_eof_clean);
+    g_test_add_func("/io/channel/file/preadv-all-eof/partial",
+                    test_io_channel_preadv_all_eof_partial);
+    g_test_add_func("/io/channel/file/preadv-all/eof-is-error",
+                    test_io_channel_preadv_all_eof_is_error);
+#endif
 #ifndef _WIN32
     g_test_add_func("/io/channel/pipe/sync", test_io_channel_pipe_sync);
     g_test_add_func("/io/channel/pipe/async", test_io_channel_pipe_async);
-- 
2.43.0

Re: [PATCH v2 0/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Junjie Cao]

Hi Peter, Daniel,

I wanted to follow up on this series.  Looking back, I went
straight to adding new public API in io/channel.  I should 
have confirmed my approach and whether the Suggested-by
tag was appropriate here with you before sending.  Sorry for that.

I see two possible paths and would appreciate your guidance:

  (a) Keep it purely a fix in migration/file.c: add a local
      retry loop around qio_channel_pread() in
      multifd_file_recv_data(), fix the ssize_t/size_t
      mismatch, and use ERRP_GUARD() so error_prepend()
      is safe.

  (b) If pread_all() retry helpers are worth having as a
      general facility in io/channel, I'm happy to propose
      them as a separate RFC so we can discuss the API design
      on its own merits.

I'm leaning toward (a) and can send a v3 quickly.
Does that sound right?

Thanks,
Junjie

Re: [PATCH v2 1/3] io/channel: introduce qio_channel_pread{v, }_all() and preadv_all_eof()

[Daniel P. Berrangé via qemu development]

On Wed, Mar 18, 2026 at 10:01:11PM +0800, Junjie Cao wrote:
> qio_channel_pread() and qio_channel_preadv() perform a single read
> and may return a short result.  Callers that need all bytes currently
> have to open-code a retry loop or simply treat a short read as an
> error.
> 
> Introduce three new helpers following the existing read_all / readv_all
> pattern:
> 
>   qio_channel_preadv_all_eof()  – retry loop; returns 1 on success,
>                                    0 on clean EOF, -1 on error.
>   qio_channel_preadv_all()      – wraps _eof; treats early EOF as
>                                    error; returns 0 / -1.
>   qio_channel_pread_all()       – single-buffer convenience wrapper
>                                    around preadv_all().
> 
> These advance the file offset internally after each partial read, so
> callers only need a single call.  All three are marked
> coroutine_mixed_fn, consistent with the existing _all helpers.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>  include/io/channel.h | 63 ++++++++++++++++++++++++++++++++
>  io/channel.c         | 85 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 148 insertions(+)

So currently we have:

   qio_channel_read
   qio_channel_read_all
   qio_channel_read_all_eof

   qio_channel_readv
   qio_channel_readv_all
   qio_channel_readv_all_eof

   qio_channel_readv_full
   qio_channel_readv_full_all
   qio_channel_readv_full_all_eof

   qio_channel_pread
   qio_channel_preadv

The obvious gaps in this API design pattern are

   qio_channel_pread_all
   qio_channel_pread_all_eof
   qio_channel_preadv_all
   qio_channel_preadv_all_eof

Your patch is fixing 3 of those 4 gaps. Can you also ensure
it adds qio_channel_pread_all_eof so we complete the set.


On the write side we have

   qio_channel_write
   qio_channel_write_all

   qio_channel_writev
   qio_channel_writev_all

   qio_channel_writev_full
   qio_channel_writev_full_all

   qio_channel_pwrite
   qio_channel_pwritev

Thus we're missing

   qio_channel_pwrite_all
   qio_channel_pwritev_all

Your patch only adds read variants, can you add the corresponding
write variants too.

The write variants can be a separate commit from the read
variants.

> diff --git a/include/io/channel.h b/include/io/channel.h
> index 1b02350437..d5b14c8c77 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -634,6 +634,69 @@ ssize_t qio_channel_preadv(QIOChannel *ioc, const struct iovec *iov,
>  ssize_t qio_channel_pread(QIOChannel *ioc, void *buf, size_t buflen,
>                            off_t offset, Error **errp);
>  
> +/**
> + * qio_channel_preadv_all_eof:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data into
> + * @niov: the length of the @iov array
> + * @offset: the starting offset in the channel to read from
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads data contiguously from the channel into @iov starting at
> + * @offset, retrying on short reads until all requested bytes have
> + * been read.  The offset is advanced internally after each partial
> + * read.
> + *
> + * Returns: 1 if all bytes were read, 0 if no data could be read
> + *          before encountering end-of-file, or -1 on error

Keep the descriptive text matching the other APIs as closely
as possible. eg

 * Reads @iov, possibly blocking or (if the channel is non-blocking)
 * yielding from the current coroutine multiple times until the entire
 * content is read. If end-of-file occurs immediately it is not an
 * error, but if it occurs after data has been read it will return
 * an error rather than a short-read. Otherwise behaves as
 * qio_channel_preadv().
 *
 * Returns: 1 if all bytes were read, 0 if end-of-file occurs
 *          without data, or -1 on error


And similarly copy the docs for the other APIs as closely
as possible.

> + */
> +int coroutine_mixed_fn qio_channel_preadv_all_eof(QIOChannel *ioc,
> +                                                  const struct iovec *iov,
> +                                                  size_t niov,
> +                                                  off_t offset,
> +                                                  Error **errp);
> +
> +/**
> + * qio_channel_preadv_all:
> + * @ioc: the channel object
> + * @iov: the array of memory regions to read data into
> + * @niov: the length of the @iov array
> + * @offset: the starting offset in the channel to read from
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads data contiguously from the channel into @iov starting at
> + * @offset, retrying on short reads until all requested bytes have
> + * been read.  Unlike qio_channel_preadv_all_eof(), end-of-file
> + * before all data has been read is treated as an error.
> + *
> + * Returns: 0 if all bytes were read, or -1 on error
> + */
> +int coroutine_mixed_fn qio_channel_preadv_all(QIOChannel *ioc,
> +                                              const struct iovec *iov,
> +                                              size_t niov,
> +                                              off_t offset,
> +                                              Error **errp);
> +
> +/**
> + * qio_channel_pread_all:
> + * @ioc: the channel object
> + * @buf: the memory region to read data into
> + * @buflen: the number of bytes to read into @buf
> + * @offset: the starting offset in the channel to read from
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads @buflen bytes contiguously from the channel at @offset
> + * into @buf, retrying on short reads.  End-of-file before all data
> + * has been read is treated as an error.
> + *
> + * Returns: 0 if all bytes were read, or -1 on error
> + */
> +int coroutine_mixed_fn qio_channel_pread_all(QIOChannel *ioc,
> +                                             void *buf,
> +                                             size_t buflen,
> +                                             off_t offset,
> +                                             Error **errp);
> +
>  /**
>   * qio_channel_shutdown:
>   * @ioc: the channel object
> diff --git a/io/channel.c b/io/channel.c
> index cc02d997a4..e18c40aa0b 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -507,6 +507,91 @@ ssize_t qio_channel_pread(QIOChannel *ioc, void *buf, size_t buflen,
>      return qio_channel_preadv(ioc, &iov, 1, offset, errp);
>  }
>  
> +int coroutine_mixed_fn qio_channel_preadv_all_eof(QIOChannel *ioc,
> +                                                  const struct iovec *iov,
> +                                                  size_t niov,
> +                                                  off_t offset,
> +                                                  Error **errp)
> +{
> +    int ret = -1;
> +    struct iovec *local_iov = g_new(struct iovec, niov);
> +    struct iovec *local_iov_head = local_iov;
> +    unsigned int nlocal_iov = niov;
> +    bool partial = false;
> +
> +    nlocal_iov = iov_copy(local_iov, nlocal_iov,
> +                          iov, niov,
> +                          0, iov_size(iov, niov));
> +
> +    while (nlocal_iov > 0) {
> +        ssize_t len;
> +        len = qio_channel_preadv(ioc, local_iov, nlocal_iov, offset, errp);
> +
> +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            qio_channel_wait_cond(ioc, G_IO_IN);
> +            continue;
> +        }
> +
> +        if (len == 0) {
> +            if (!partial) {
> +                ret = 0;
> +                goto cleanup;
> +            }
> +            error_setg(errp,
> +                       "Unexpected end-of-file before all data were read");
> +            goto cleanup;
> +        }
> +
> +        if (len < 0) {
> +            goto cleanup;
> +        }
> +
> +        partial = true;
> +        offset += len;
> +        iov_discard_front(&local_iov, &nlocal_iov, len);
> +    }
> +
> +    ret = 1;
> +
> + cleanup:
> +    g_free(local_iov_head);
> +    return ret;
> +}
> +
> +int coroutine_mixed_fn qio_channel_preadv_all(QIOChannel *ioc,
> +                                              const struct iovec *iov,
> +                                              size_t niov,
> +                                              off_t offset,
> +                                              Error **errp)
> +{
> +    int ret = qio_channel_preadv_all_eof(ioc, iov, niov, offset, errp);
> +
> +    if (ret == 0) {
> +        error_setg(errp,
> +                   "Unexpected end-of-file before all data were read");
> +        return -1;
> +    }
> +    if (ret == 1) {
> +        return 0;
> +    }
> +
> +    return ret;
> +}
> +
> +int coroutine_mixed_fn qio_channel_pread_all(QIOChannel *ioc,
> +                                             void *buf,
> +                                             size_t buflen,
> +                                             off_t offset,
> +                                             Error **errp)
> +{
> +    struct iovec iov = {
> +        .iov_base = buf,
> +        .iov_len = buflen
> +    };
> +
> +    return qio_channel_preadv_all(ioc, &iov, 1, offset, errp);
> +}
> +
>  int qio_channel_shutdown(QIOChannel *ioc,
>                           QIOChannelShutdown how,
>                           Error **errp)
> -- 
> 2.43.0
> 

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 :|

Re: [PATCH v2 2/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Daniel P. Berrangé]

On Wed, Mar 18, 2026 at 10:01:12PM +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 switching to qio_channel_pread_all() introduced in
> the previous commit, which retries on short reads and treats
> end-of-file as an error, so the caller no longer needs to check the
> byte count manually.  Add ERRP_GUARD() so that error_prepend() works
> correctly even when errp is &error_fatal or NULL.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>  migration/file.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

Re: [PATCH v2 0/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Daniel P. Berrangé]

On Tue, Mar 24, 2026 at 04:27:38PM +0800, Junjie Cao wrote:
> Hi Peter, Daniel,
> 
> I wanted to follow up on this series.  Looking back, I went
> straight to adding new public API in io/channel.  I should 
> have confirmed my approach and whether the Suggested-by
> tag was appropriate here with you before sending.  Sorry for that.
> 
> I see two possible paths and would appreciate your guidance:
> 
>   (a) Keep it purely a fix in migration/file.c: add a local
>       retry loop around qio_channel_pread() in
>       multifd_file_recv_data(), fix the ssize_t/size_t
>       mismatch, and use ERRP_GUARD() so error_prepend()
>       is safe.
> 
>   (b) If pread_all() retry helpers are worth having as a
>       general facility in io/channel, I'm happy to propose
>       them as a separate RFC so we can discuss the API design
>       on its own merits.
> 
> I'm leaning toward (a) and can send a v3 quickly.
> Does that sound right?

Doing (b) is the right approach. We want these helpers in the
general code, so we don't create technical debt that has to be
fixed next time something else wants the same APIs.

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 :|

Re: [PATCH v2 0/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Peter Xu]

On Tue, Mar 24, 2026 at 10:54:38AM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 24, 2026 at 04:27:38PM +0800, Junjie Cao wrote:
> > Hi Peter, Daniel,
> > 
> > I wanted to follow up on this series.  Looking back, I went
> > straight to adding new public API in io/channel.  I should 
> > have confirmed my approach and whether the Suggested-by
> > tag was appropriate here with you before sending.  Sorry for that.
> > 
> > I see two possible paths and would appreciate your guidance:
> > 
> >   (a) Keep it purely a fix in migration/file.c: add a local
> >       retry loop around qio_channel_pread() in
> >       multifd_file_recv_data(), fix the ssize_t/size_t
> >       mismatch, and use ERRP_GUARD() so error_prepend()
> >       is safe.
> > 
> >   (b) If pread_all() retry helpers are worth having as a
> >       general facility in io/channel, I'm happy to propose
> >       them as a separate RFC so we can discuss the API design
> >       on its own merits.
> > 
> > I'm leaning toward (a) and can send a v3 quickly.
> > Does that sound right?
> 
> Doing (b) is the right approach. We want these helpers in the
> general code, so we don't create technical debt that has to be
> fixed next time something else wants the same APIs.

Sorry for a late response, Junjie.  I agree with Dan.

-- 
Peter Xu