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.
* 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.
* 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.
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?
> 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.
break;
This break jumps to if (ret) { return ret; }, so it's a roundabout way
to return ret. Meh.
}
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?
break;
Again, a roundabout way to return ret.
}
Obviously ret >= 0 here. I believe @errp is not set.
}
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 this function would be easier to understand if we replaced break
by return.
