Re: [PATCH QEMU HCI-8.1 v4 15/31] 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() prepends to unset errp when put() returns positive
--

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

migration: vmstate_save_state_v: fix double error_setg

This change avoids calling error_setg() twice on the same errp in
vmstate_save_state_v() when a nested vmstate_save_state() or
vmstate_save_state_v() call fails. It sets the error only in the leaf put()
branch, and the outer failure path now uses error_prepend() instead of
error_setg().

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

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> --- 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");
> +                    }
>                  }

[Severity: Medium]
Can a put() callback return a positive value here? The new error_setg() is
guarded by ret < 0, but the check further down in vmstate_save_state_v() is
if (ret), so any non-zero return is treated as failure.

target/arm/machine.c:put_power() is an in-tree example. It returns 1 without
setting an Error when cpu->power_state is neither PSCI_ON nor PSCI_OFF:

    } else {
        return 1;
    }

In that case ret > 0, errp is left unset, and the code continues into the
failure path below.

>  
>                  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);

Can this error_prepend() dereference a NULL *errp? error_vprepend() only
returns early when errp itself is NULL:

    if (!errp) {
        return;
    }
    ...
    g_string_append(newmsg, (*errp)->msg);

If the caller passed a non-NULL errp, for example the one passed from
savevm.c, and put() returned a positive value, then *errp is still NULL and
(*errp)->msg is a NULL dereference. The old code called error_setg() here, so
this case did not crash before the patch.

The same thing can happen at each nesting level, since a positive ret
propagates up from the VMS_STRUCT and VMS_VSTRUCT branches with no Error set.

Would it be better to use if (ret) rather than if (ret < 0) around the
error_setg() after put()? Alternatively, the outer path could call
error_setg() when *errp is not yet set.

>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=15