On Tue, Nov 18, 2025 at 08:44:32AM +0100, Markus Armbruster wrote:
> Peter Xu <[email protected]> writes:
>
> > On Sat, Nov 15, 2025 at 09:34:57AM +0100, Markus Armbruster wrote:
> >> Maintainers decide what to take for 10.2, if anything.
> >>
> >> Let me know if you'd prefer the "perhaps should take ownership" idea
> >> in PATCH 1's commit message.
> >
> > I recall I had such patch previously, so I digged it out:
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > I found that I dropped it in v3's post of that series, where I mentioned in
> > the change log that either way is not clean, so I dropped that until I
> > could have a better understanding:
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > I think at that time I should have hit an use case where the caller forgot
> > to error_copy(), hence passing it in causing an UAF.
>
> Use-after-free can happen if the caller holds on to its reference until
> after the stored Error is freed. In other words, holding on to the
> reference is safe only until the stored Error is freed, and any safety
> argument will have to reason about the stored Error's lifetime. No idea
> how difficult or brittle such an argument would be.
>
> > Then I thought memory
> > leak was better in error path comparing to UAF if the API was used wrong
> > either way.
>
> Fair point.
>
> > But feel free to share your thoughts. We can definitely revisit this.
>
> migrate_set_error(s, err) stores a copy of @err in @s unless @s already
> has an Error stored.
>
> I see 26 calls of migrate_set_error().
>
> * 11 call error_free() immediately, and 2 call it via g_autoptr(). 3
> neglect to call it. My patch fixes them. Total is 16 out of 26.
Yes, I appreciate that!
>
> * 6 report and free with error_report_err(), i.e. we store the error for
> later *and* report it now. Gives me an uneasy feeling. How is the
> stored error handled? Will it be reported again? That would likely
> be wrong.
Migration as a module outlives me working as a developer on QEMU.. so I can
only share my two cents that still resides in my memory, which may or may
not always be the truth..
I think one issue is migration used to error_report() things all over the
places, but then we thought it a good idea to try remember the error so
that libvirt can query at any time if necessary. With that, starting from
QEMU 2.7 we introduced error-desc into query-migrate results. That's what
s->error was about.
But then, could we drop the error_report() / error_report_err()s? Likely
not, because there might be user relying on the printed errors to see what
happened.. Making query-migrate the only source of error report adds
complexity to those users and may cause confusions.. Hence the extra
references sometimes needed after migrate_set_error(), almost for keeping
behaviors to print it out as the old times (I didn't check each of them,
though).
>
> * 3 wrap migrate_set_error():
>
> - multifd_send_set_error()
>
> Its callers both call error_free() immediately.
>
> - migration_connect_set_error()
>
> 3 callers.
>
> qmp_migrate() and qmp_migrate_finish() propagate to their callers,
> i.e. we store the error for later *and* have callers handle it now.
> Same uneasy feeling as above.
>
> One of migration_connect()'s callers passes NULL, the other calls
> error_free() immediately.
>
> - multifd_recv_terminate_threads()
>
> Two callers pass NULL, one calls error_free() immediately, and
> multifd_recv_new_channel() propagates. Uneasy feeling again.
>
> * qemu_savevm_state_setup() confuses me. It sets @errp on failure, and
> stores some (uneasy feeling), but not all of these errors with
> migrate_set_error(). See below.
Yes, I also agree qemu_savevm_state_setup() is confusing on error
handlings. I'll comment below.
>
> Summary:
>
> * We're prone to leaking the Error passed to migrate_set_error().
>
> * If we replaced it by a function that takes ownership, we may become
> prone to use-after-free. I write "may" because I'm unsure when
> exactly a use after calling this ownership-taking function would be
> unsafe.
>
> * The "forked" Error handling makes me uneasy. I'm sure we do it for
> reasons. Can you help me understand them?
Hope above would provide some context. In short, IMHO it's a mixture
demand of "print the error immediately like the old behavior" and "remember
the error too when query".
>
> > I queued the series for this release, thanks Markus.
>
> Thanks!
>
>
> Bonus content:
>
> int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
> {
> ERRP_GUARD();
> MigrationState *ms = migrate_get_current();
> JSONWriter *vmdesc = ms->vmdesc;
> SaveStateEntry *se;
> int ret = 0;
>
> if (vmdesc) {
> json_writer_int64(vmdesc, "page_size", qemu_target_page_size());
> json_writer_start_array(vmdesc, "devices");
> }
>
> trace_savevm_state_setup();
> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>
> The function does work in a loop.
>
> This work can fail in two places, setting @errp. When it does, we
> return.
>
> if (se->vmsd && se->vmsd->early_setup) {
>
> First one:
>
> ret = vmstate_save(f, se, vmdesc, errp);
> if (ret) {
>
> vmstate_save() set @errp and returned an error code.
>
> migrate_set_error(ms, *errp);
> qemu_file_set_error(f, ret);
>
> Store @errp in @ms, and the error code in @f.
>
> Aside: storing error state in two places feels like one too many.
Correct. It's once again legacy of migration code where it used to set
error onto fds (e.g. that'll start to fail all qemufile APIs on this fd,
and qemufile API is not well designed IMHO.. which is another story), but
then we start to have string-based Errors and I guess we didn't try harder
to unify both.
>
> break;
>
> This break jumps to if (ret) { return ret; }, so it's a roundabout way
> to return ret. Meh.
IIUC some people prefer such way so the function returns at a single point
(easier to add common cleanup codes, for example). But I'd confess I also
prefer a direct return. :)
>
> }
> continue;
> }
>
> if (!se->ops || !se->ops->save_setup) {
> continue;
> }
> if (se->ops->is_active) {
> if (!se->ops->is_active(se->opaque)) {
> continue;
> }
> }
> save_section_header(f, se, QEMU_VM_SECTION_START);
>
> Second one:
>
> ret = se->ops->save_setup(f, se->opaque, errp);
> save_section_footer(f, se);
> if (ret < 0) {
>
> ->save_setup() set @errp and returned an error code.
>
> qemu_file_set_error(f, ret);
>
> This time we store only the error code. Why not @errp?
IIUC migrate_set_error() above was redundant instead, because
qemu_savevm_state_setup()'s caller will do migrate_set_error().
Said that, we shouldn't need to randomly call qemu_file_set_error() either
deep in this function.. may need some cleanups.
>
> break;
>
> Again, a roundabout way to return ret.
>
> }
>
> Obviously ret >= 0 here. I believe @errp is not set.
I don't think in this path ret can be >0.. but yeah this is pretty
unobvious even if true. That's also why I liked QEMU's current preferences
of "bool fn(..., Error **errp)".. at least in new codes. Maybe I should
ping Vladimir on his recent work here?
https://lore.kernel.org/r/[email protected]
That'll be part of such cleanup effort (and yes unfortunately many
migration related cleanups will need a lot of code churns...).
>
> }
>
> We get here either via break or normal loop termination.
>
> If via break, ret != 0 and @errp set.
>
> If via normal loop termination, ret >= 0 and @errp not set.
>
> if (ret) {
> return ret;
>
> If via normal loop termination, and ret > 0, we return failure (I think)
> without setting @errp. I hope this can't happen, because ->save_setup()
> never returns > 0. But it's unclean even then.
>
> }
>
> I trust @errp is still unset here. This is the case if we take the
> return right above when @errp has been set.
>
> /* TODO: Should we check that errp is set in case of failure ? */
> return precopy_notify(PRECOPY_NOTIFY_SETUP, errp);
>
> This time we store nothing. Why?
I think the clean way is we do not store anything when this function
provided Error**.
>
> }
>
> I think this function would be easier to understand if we replaced break
> by return.
I'll see what I can do with this function.
Thanks!
--
Peter Xu
