Re: [PATCH QEMU HCI-8.1 v6 06/34] migration/cpr-exec: close the persisted state if the channel fails to open

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration/cpr-exec: val used after g_unsetenv() in cpr_exec_unpersist_state()
--

commit 2730ea087674f1b28d9aa6e1ca063cdd1155b572
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration/cpr-exec: close the persisted state if the channel fails to open

This patch makes qmp_migrate_finish() call cpr_exec_unpersist_state() when
the outgoing channel fails to open. That releases the dup'd memfd and the
environment variable that cpr_exec_persist_state() created. It also makes
cpr_exec_unpersist_state() return early when the variable isn't set.

> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c

[ ... ]

> @@ -83,8 +83,10 @@ void cpr_exec_unpersist_state(void)
>      int mfd;
>      const char *val = g_getenv(CPR_EXEC_STATE_NAME);
>  
> +    if (!val) {
> +        return;
> +    }
>      g_unsetenv(CPR_EXEC_STATE_NAME);
> -    assert(val);
>      assert(!qemu_strtoi(val, NULL, 10, &mfd));
>      close(mfd);
>  }

[Severity: Low]
Is it safe to parse val after g_unsetenv() has removed the entry?

In cpr_exec_unpersist_state(), val comes from g_getenv() and points into
the environment string created by g_setenv() in cpr_exec_persist_state():

    const char *val = g_getenv(CPR_EXEC_STATE_NAME);
    ...
    g_unsetenv(CPR_EXEC_STATE_NAME);
    assert(!qemu_strtoi(val, NULL, 10, &mfd));

GLib documents that the string returned by g_getenv() may be invalidated
by a later g_setenv() or g_unsetenv(). On a libc that frees setenv()
allocated entries in unsetenv(), such as musl, qemu_strtoi() would read
freed memory. The result could be an assert failure or a wrong fd passed
to close(). On glibc the string is only unlinked, so it works there.

This ordering was already in cpr_exec_unpersist_state() and in
cpr_exec_find_state(). The new call in qmp_migrate_finish() runs in the
process that called g_setenv(), which is the case where the string is
heap-allocated. In cpr_exec_find_state() the entry comes from the
inherited environment block after exec(), so it is much less likely to be
freed.

I did not check the musl source, so this is speculative.

Could val be parsed into mfd, or copied with g_strdup(), before calling
g_unsetenv()? The same reordering would apply to cpr_exec_find_state().

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -2413,6 +2413,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
>          cpr_state_close();
> +        cpr_exec_unpersist_state();
>          migration_connect_set_error(s, local_err);
>          error_propagate(errp, local_err);
>          return;

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=6