1. Patchew
  2. QEMU
  3. View series

[PATCH] qtest/migration: Fix potential NPD through getenv

xjdeng posted 1 patch 4 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250627024226.1767-1-micro6947@gmail.com
Maintainers: Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
tests/qtest/migration/migration-util.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
[PATCH] qtest/migration: Fix potential NPD through getenv
Posted by xjdeng 4 months, 3 weeks ago 
In `find_common_machine_version`, the code previously assumed that
`getenv(var1)` and `getenv(var2)` would always return non-NULL values.
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`.

Signed-off-by: xjdeng <micro6947@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));
+    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();
 }
 
-- 
2.27.0.windows.1
Re: [PATCH] qtest/migration: Fix potential NPD through getenv
Posted by Fabiano Rosas 4 months, 2 weeks ago 
xjdeng <micro6947@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 <micro6947@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();
>  }
Re: [PATCH] qtest/migration: Fix potential NPD through getenv
Posted by Xingjing Deng 4 months, 2 weeks ago 
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 <farosas@suse.de> 于2025年6月28日周六 04:52写道：

> xjdeng <micro6947@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 <micro6947@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();
> >  }
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.