Re: [PATCH QEMU HCI-8.1 v3 13/28] migration: vmstate_save_state_v: fix double error_setg

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: vmstate_save_state_v() sets Error only for ret < 0 but prepends for any ret != 0
--

commit e68e95a3b4262b7b5d21222eb858ca4b5d64cc07
Author: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

migration: vmstate_save_state_v: fix double error_setg

This patch avoids calling error_setg() twice on the same errp in
vmstate_save_state_v(). It adds error_setg() after a failing put()
callback, and it changes the later "Save of field" error_setg() to
error_prepend().

Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-2-vsementsov@yandex-team.ru

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index aea41c099463..04903131257b 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c

[ ... ]

> @@ -523,6 +523,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  } else {
>                      ret = inner_field->info->put(f, curr_elem, size,
>                                                   inner_field, vmdesc_loop);
> +                    if (ret < 0) {
> +                        error_setg(errp, "put failed");
> +                    }
>                  }
>  
>                  written_bytes = qemu_file_transferred(f) - old_offset;
> @@ -535,8 +538,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>  
>                  if (ret) {
> -                    error_setg(errp, "Save of field %s/%s failed",
> -                                vmsd->name, field->name);
> +                    error_prepend(errp, "Save of field %s/%s failed: ",
> +                                  vmsd->name, field->name);
>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }

[Severity: Medium]
Can error_prepend() dereference a NULL *errp here?

The new error_setg(errp, "put failed") after put() is guarded by
"ret < 0", but the failure branch below is guarded by "if (ret)".  For a
positive non-zero ret, nothing sets *errp before:

    if (ret) {
        error_prepend(errp, "Save of field %s/%s failed: ", ...);

and error_vprepend() only checks !errp before using the message:

util/error.c:error_vprepend() {
    ...
    if (!errp) {
        return;
    }
    newmsg = g_string_new(NULL);
    g_string_vprintf(newmsg, fmt, ap);
    g_string_append(newmsg, (*errp)->msg);
    ...
}

Before this patch every non-zero ret went through error_setg(), so this
asymmetry is new.

A positive value could reach this point through the wrapper callbacks
that return the nested vmstate_save_state() result unchanged while
passing a local Error instead of the outer errp, for example:

migration/vmstate-types.c:put_tmp() {
    ...
    ret = vmstate_save_state(f, vmsd, tmp, vmdesc, &local_err);
    if (ret) {
        error_report_err(local_err);
    }
    g_free(tmp);
    return ret;
}

put_gtree() has the same shape and returns capsule.ret.  The nested
vmstate_save_state_v() returns any non-zero pre_save or post_save result:

    ret = vmsd->pre_save(opaque);
    ...
    if (ret) {
        error_setg(errp, "pre-save failed: %s", vmsd->name);
        return ret;
    }

so a VMStateDescription whose pre_save or post_save returns a positive
value would leave the outer *errp unset and crash in error_prepend().

The same crash would also occur for a negative ret if errp were
&error_warn, since error_handle() leaves *errp NULL in that case.  I did
not find a caller that passes error_warn.

I did not find an in-tree put() callback that returns a positive value
today, so this looks speculative, but I did not audit every pre_save,
post_save or target/ put callback.  The later patches in the series do
not change migration/vmstate.c or migration/vmstate-types.c.

Would it make sense to make the two guards consistent, for example by
calling error_setg() for any non-zero ret, or by checking
"if (ret && !*errp)" before error_setg()?  Using error_propagate_prepend()
semantics is another option.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=13