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

Junjie Cao posted 3 patches 2 weeks, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260318140113.434-1-junjie.cao@intel.com
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
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(-)
[PATCH v2 0/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data
Posted by Junjie Cao 2 weeks, 5 days ago
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
Re: [PATCH v2 0/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data
Posted by Junjie Cao 1 week, 6 days ago
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 0/3] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data
Posted by Daniel P. Berrangé 1 week, 6 days ago
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
Posted by Peter Xu 1 week, 4 days ago
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