| Message ID | 20241015170437.310358-1-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] migration: Ensure vmstate_save() sets errp | expand |
On Tue, Oct 15, 2024 at 07:04:37PM +0200, Hanna Czenczek wrote: > migration/savevm.c contains some calls to vmstate_save() that are > followed by migrate_set_error() if the integer return value indicates an > error. migrate_set_error() requires that the `Error *` object passed to > it is set. Therefore, vmstate_save() is assumed to always set *errp on > error. > > Right now, that assumption is not met: vmstate_save_state_v() (called > internally by vmstate_save()) will not set *errp if > vmstate_subsection_save() or vmsd->post_save() fail. Fix that by adding > an *errp parameter to vmstate_subsection_save(), and by generating a > generic error in case post_save() fails (as is already done for > pre_save()). > > Without this patch, qemu will crash after vmstate_subsection_save() or > post_save() have failed inside of a vmstate_save() call (unless > migrate_set_error() then happen to discard the new error because > s->error is already set). This happens e.g. when receiving the state > from a virtio-fs back-end (virtiofsd) fails. > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > v2: As suggested by Peter, after vmsd->post_save(), change the condition > from `if (!ret)` to `if (!ret && ps_ret)` so we will not create an > error object in case of success (that would then be leaked, most > likely). > --- > migration/vmstate.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) queued, thanks!
diff --git a/migration/vmstate.c b/migration/vmstate.c index ff5d589a6d..fa002b24e8 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -22,7 +22,8 @@ #include "trace.h" static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, - void *opaque, JSONWriter *vmdesc); + void *opaque, JSONWriter *vmdesc, + Error **errp); static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, void *opaque); @@ -441,12 +442,13 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, json_writer_end_array(vmdesc); } - ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc); + ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp); if (vmsd->post_save) { int ps_ret = vmsd->post_save(opaque); - if (!ret) { + if (!ret && ps_ret) { ret = ps_ret; + error_setg(errp, "post-save failed: %s", vmsd->name); } } return ret; @@ -518,7 +520,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, } static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, - void *opaque, JSONWriter *vmdesc) + void *opaque, JSONWriter *vmdesc, + Error **errp) { const VMStateDescription * const *sub = vmsd->subsections; bool vmdesc_has_subsections = false; @@ -546,7 +549,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd, qemu_put_byte(f, len); qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len); qemu_put_be32(f, vmsdsub->version_id); - ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc); + ret = vmstate_save_state_with_err(f, vmsdsub, opaque, vmdesc, errp); if (ret) { return ret; }
migration/savevm.c contains some calls to vmstate_save() that are followed by migrate_set_error() if the integer return value indicates an error. migrate_set_error() requires that the `Error *` object passed to it is set. Therefore, vmstate_save() is assumed to always set *errp on error. Right now, that assumption is not met: vmstate_save_state_v() (called internally by vmstate_save()) will not set *errp if vmstate_subsection_save() or vmsd->post_save() fail. Fix that by adding an *errp parameter to vmstate_subsection_save(), and by generating a generic error in case post_save() fails (as is already done for pre_save()). Without this patch, qemu will crash after vmstate_subsection_save() or post_save() have failed inside of a vmstate_save() call (unless migrate_set_error() then happen to discard the new error because s->error is already set). This happens e.g. when receiving the state from a virtio-fs back-end (virtiofsd) fails. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- v2: As suggested by Peter, after vmsd->post_save(), change the condition from `if (!ret)` to `if (!ret && ps_ret)` so we will not create an error object in case of success (that would then be leaked, most likely). --- migration/vmstate.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)