Hi, thank you for getting back to me.
During my data mining, I came across the following operation:
migration_get_env, which is called after find_common_machine_version. Based
on this, I initially suspected there might be a bug. Here's the relevant
code inside migration_get_env.
env->qemu_src = getenv(QEMU_ENV_SRC);
env->qemu_dst = getenv(QEMU_ENV_DST);
/*
* The default QTEST_QEMU_BINARY must always be provided because
* that is what helpers use to query the accel type and
* architecture.
*/
if (env->qemu_src && env->qemu_dst) {
g_test_message("Only one of %s, %s is allowed",
QEMU_ENV_SRC, QEMU_ENV_DST);
exit(1);
}
However, after considering your explanation, I believe you've convinced me
otherwise. I appreciate your input, and I’m sorry for the delayed response
and any inconvenience caused.
Fabiano Rosas <faro...@suse.de> 于2025年6月28日周六 04:52写道：
> xjdeng <micro6...@gmail.com> writes:
>
> Hi, thanks for the interest in fixing this. However, the analysis it not
> quite right:
>
> > In `find_common_machine_version`, the code previously assumed that
> > `getenv(var1)` and `getenv(var2)` would always return non-NULL values.
>
> That's not true. qtest_qemu_binary() has:
>
> if (var) {
> qemu_bin = getenv(var);
> if (qemu_bin) { <--- HERE
> return qemu_bin;
> }
> }
>
> > However, if either environment variable is not set, `getenv` returns
> > NULL, which could lead to a null pointer dereference.
> >
> > Tracing upstream usage: `find_common_machine_version` is called by
> > `resolve_machine_version` with `QEMU_ENV_SRC` and `QEMU_ENV_DST`.
> > `resolve_machine_version` is used by `migrate_start`, which is called
> > by `migrate_postcopy_prepare`, and ultimately by `test_postcopy_common`.
> >
> > In `test_postcopy_common`, after `migrate_postcopy_prepare`, the
> > function `migrate_postcopy_complete` is called. Inside,
> > `migration_get_env` checks if `QEMU_ENV_SRC` and `QEMU_ENV_DST` are
> > set before use. Thus, these variables can be NULL, leading to a
> > potential null pointer dereference in `find_common_machine_version`.
>
> There's no dereference happening anywhere, just a g_test_message(),
> which would show "(null)".
>
> >
> > Signed-off-by: xjdeng <micro6...@gmail.com>
> > ---
> > tests/qtest/migration/migration-util.c | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qtest/migration/migration-util.c
> b/tests/qtest/migration/migration-util.c
> > index 642cf50c8d..45c9e164e2 100644
> > --- a/tests/qtest/migration/migration-util.c
> > +++ b/tests/qtest/migration/migration-util.c
> > @@ -203,8 +203,25 @@ char *find_common_machine_version(const char
> *mtype, const char *var1,
> > return g_strdup(type2);
> > }
> >
> > - g_test_message("No common machine version for machine type '%s'
> between "
> > - "binaries %s and %s", mtype, getenv(var1),
> getenv(var2));
>
> I don't think we'll ever reach here in case one var is NULL. It would
> have been replaced with QTEST_QEMU_BINARY and either asserted or exited
> in the if below:
>
> g_autofree char *type1 = qtest_resolve_machine_alias(var1, mtype);
> g_autofree char *type2 = qtest_resolve_machine_alias(var2, mtype);
>
> g_assert(type1 && type2);
>
> if (g_str_equal(type1, type2)) {
> /* either can be used */
> return g_strdup(type1);
> }
>
> Can you provide a test command line that exposes the issue? Something
> like:
>
> QTEST_QEMU_BINARY=./qemu-system-<arch> QTEST_QEMU_BINARY_DST=<some other
> qemu version here> ../tests/qtest/migration-test --full
>
> Thanks
>
> > + char *varstring1 = getenv(var1);
> > + char *varstring2 = getenv(var2);
> > + if (varstring1 && varstring2) {
> > + g_test_message("No common machine version for machine type '%s'
> "
> > + "between binaries %s and %s",
> > + mtype, varstring1, varstring2);
> > + } else if (varstring1) {
> > + g_test_message("No common machine version for machine type '%s'
> "
> > + "between binary %s and environment variable %s",
> > + mtype, varstring1, var2);
> > + } else if (varstring2) {
> > + g_test_message("No common machine version for machine type '%s'
> "
> > + "between binary %s and environment variable %s",
> > + mtype, varstring2, var1);
> > + } else {
> > + g_test_message("No common machine version for machine type '%s'
> "
> > + "between environment variables %s and %s",
> > + mtype, var1, var2);
> > + }
> > g_assert_not_reached();
> > }
>
