[XEN PATCH 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[XEN PATCH 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Hongbo]

`QEMU_XEN_PATH` will be configured as `qemu-system-i386` with no clue where, if
`--with-system-qemu` is set without giving a path (as matched in the case `yes`
but not `*`). However, the existence of the executable is checked by `access()`,
that will not look for anywhere in $PATH but the current directory. And since it
is possible for `qemu-system-i386` (or any other configured values) to be
executed from PATH later, we'd better find that in PATH and return the full path
for the caller to check against.

Signed-off-by: Hongbo <hehongbo@mail.com>

---

This patch is from the maintenance team of the Xen Project Hypervisor at NixOS.
We encountered this and thought it was an edge case, and came up with this while
maintaining the package and module of the Hypervisor at NixOS.

According to `xen.git/tools/configure.ac`, `QEMU_XEN_PATH` will be configured as
`qemu-system-i386` (relative) if `--with-system-qemu` is set (as `yes`) but
without an absolute path. However, it will execute `qemu-system-i386` from the
`PATH` only if a file is called `qemu-system-i386` in the *current directory*.
That is because the existence of the device model executable file, in this case
`qemu-system-i386`, is checked via `access()` without concatenating it with
current PATHs. And `access()` is not tailored for executables, it will not
search for the PATHs for us.

See `libxl__domain_build_info_setdefault()` at
`xen.git/tools/libs/light/libxl_create.c`. It reads `dm` from
`libxl__domain_device_model()` and then uses `access()` on it. If that fails, it
will modify the `device_model_version` to qemu-traditional. Then, in
`libxl__spawn_local_dm()` at `xen.git/tools/lib/light/libxl_dm.c`, it reads from
`libxl__domain_device_model()` again, and `access()` is used again to detect the
file's existence. In my investigations, if I comment out these 2 existence
checks then it will run `qemu-system-i386` from the current PATH without issues.
I guess if it's not blocked by those 2 checks, it will finally reach
`libxl__exec()`. Then, inside the `libxl__exec()` the device model executable
will be executed by `execvp()`, which can certainly call the executable from
both absolute paths and current PATHs.

Since the device model executable will be checked twice, both of which will call
`libxl__domain_device_model()` to get its location, I think the preferred
solution would be patching the `libxl__domain_device_model()` function itself to
tell if we're referring to an executable in PATHs, and resolve to the full path
of it for the caller to check against.

It's indeed an edge case. But why would we need this? Because in Nix (the
package manager) and NixOS, we use Nix expressions to declare dependencies on
the dependents, and we ran into the issue of circular dependency - to build
QEMU with Xen support, we should give the Xen header and libraries into the
building process of QEMU, that makes Xen (`pkgs.xen`) a dependency of QEMU
(`pkgs.qemu_xen`), which prevents us from using `pkgs.qemu_xen` in the building
process of Xen, and in `--with-system-qemu=` argument in particular. It is very
different compared to those distros and package managers that follow the
Filesystem Hierarchy Standard (FHS), in which Xen can be built with
`--with-system-qemu=` points to a non-existent FHS location of
`qemu-system-i386`, and then use these Xen libraries from the artifacts to build
QEMU afterward. So we decide to build Xen with `--with-system-qemu` but not
including an executable path, taking advantage of the fact that `QEMU_XEN_PATH`
can be configured as a relative `qemu-system-i386` when omitted, as declared as
the `yes` case in `xen.git/tools/configure.ac`, and that results in we finding
the aforementioned "current directory" issue, and submitting this patch.

In the patch, I'm using the existence of slash (`/`) to tell if `QEMU_XEN_PATH`
is relative, and begin to search in PATH if it is. I'm sort of iffy on this,
would it make more sense if we do this on inputs starting with a slash instead?
And should we notify the user if it's not found anywhere in the PATH thus
proceeding with the value configured in `QEMU_XEN_PATH` as-is?

Let me know if it's appropriate and if further changes are needed.

Best regards,
Hongbo

---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_dm.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 1f2f5bd97a..db05f20a5b 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -332,7 +332,39 @@ const char *libxl__domain_device_model(libxl__gc *gc,
             dm = libxl__abs_path(gc, "qemu-dm", libxl__private_bindir_path());
             break;
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            dm = qemu_xen_path(gc);
+            const char *configured_dm = qemu_xen_path(gc);
+            if (strchr(configured_dm, '/'))
+                dm = libxl__strdup(gc, configured_dm);
+            else
+            {
+                const char *path_env = getenv("PATH");
+                if (!path_env)
+                    dm = libxl__strdup(gc, configured_dm);
+                else
+                {
+                    char *path_dup = libxl__strdup(gc, path_env);
+                    char *saveptr;
+
+                    char *path = strtok_r(path_dup, ":", &saveptr);
+                    char fullpath[PATH_MAX];
+                    bool dm_found = false;
+                    while (path)
+                    {
+                        snprintf(fullpath, sizeof(fullpath), "%s/%s", path,
+                                 configured_dm);
+                        if (access(fullpath, X_OK) == 0)
+                        {
+                            dm = libxl__strdup(gc, fullpath);
+                            dm_found = true;
+                            break;
+                        }
+                        path = strtok_r(NULL, ":", &saveptr);
+                    }
+
+                    if (!dm_found)
+                        dm = libxl__strdup(gc, configured_dm);
+                }
+            }
             break;
         default:
             LOG(ERROR, "invalid device model version %d",
--
2.39.5 (Apple Git-154)

Re: [XEN PATCH 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Anthony PERARD]

On Mon, Dec 23, 2024 at 11:17:53AM +0800, Hongbo wrote:
> `QEMU_XEN_PATH` will be configured as `qemu-system-i386` with no clue where, if
> `--with-system-qemu` is set without giving a path (as matched in the case `yes`
> but not `*`). However, the existence of the executable is checked by `access()`,
> that will not look for anywhere in $PATH but the current directory. And since it
> is possible for `qemu-system-i386` (or any other configured values) to be
> executed from PATH later, we'd better find that in PATH and return the full path
> for the caller to check against.
> 
> Signed-off-by: Hongbo <hehongbo@mail.com>
> 
> ---
> 
> This patch is from the maintenance team of the Xen Project Hypervisor at NixOS.
> We encountered this and thought it was an edge case, and came up with this while
> maintaining the package and module of the Hypervisor at NixOS.
> 
> According to `xen.git/tools/configure.ac`, `QEMU_XEN_PATH` will be configured as
> `qemu-system-i386` (relative) if `--with-system-qemu` is set (as `yes`) but
> without an absolute path. However, it will execute `qemu-system-i386` from the
> `PATH` only if a file is called `qemu-system-i386` in the *current directory*.
> That is because the existence of the device model executable file, in this case
> `qemu-system-i386`, is checked via `access()` without concatenating it with
> current PATHs. And `access()` is not tailored for executables, it will not
> search for the PATHs for us.
> 
> See `libxl__domain_build_info_setdefault()` at
> `xen.git/tools/libs/light/libxl_create.c`. It reads `dm` from
> `libxl__domain_device_model()` and then uses `access()` on it. If that fails, it
> will modify the `device_model_version` to qemu-traditional. Then, in
> `libxl__spawn_local_dm()` at `xen.git/tools/lib/light/libxl_dm.c`, it reads from
> `libxl__domain_device_model()` again, and `access()` is used again to detect the
> file's existence. In my investigations, if I comment out these 2 existence
> checks then it will run `qemu-system-i386` from the current PATH without issues.
> I guess if it's not blocked by those 2 checks, it will finally reach
> `libxl__exec()`. Then, inside the `libxl__exec()` the device model executable
> will be executed by `execvp()`, which can certainly call the executable from
> both absolute paths and current PATHs.
> 
> Since the device model executable will be checked twice, both of which will call
> `libxl__domain_device_model()` to get its location, I think the preferred
> solution would be patching the `libxl__domain_device_model()` function itself to
> tell if we're referring to an executable in PATHs, and resolve to the full path
> of it for the caller to check against.
> 
> It's indeed an edge case. But why would we need this? Because in Nix (the
> package manager) and NixOS, we use Nix expressions to declare dependencies on
> the dependents, and we ran into the issue of circular dependency - to build
> QEMU with Xen support, we should give the Xen header and libraries into the
> building process of QEMU, that makes Xen (`pkgs.xen`) a dependency of QEMU
> (`pkgs.qemu_xen`), which prevents us from using `pkgs.qemu_xen` in the building
> process of Xen, and in `--with-system-qemu=` argument in particular. It is very
> different compared to those distros and package managers that follow the
> Filesystem Hierarchy Standard (FHS), in which Xen can be built with
> `--with-system-qemu=` points to a non-existent FHS location of
> `qemu-system-i386`, and then use these Xen libraries from the artifacts to build
> QEMU afterward. So we decide to build Xen with `--with-system-qemu` but not
> including an executable path, taking advantage of the fact that `QEMU_XEN_PATH`
> can be configured as a relative `qemu-system-i386` when omitted, as declared as
> the `yes` case in `xen.git/tools/configure.ac`, and that results in we finding
> the aforementioned "current directory" issue, and submitting this patch.

Thanks. Sounds good, and also the feature seems half-way there already
with the default to a binary name and the use of execvp. The patch looks
mostly fine, I still have a few comments.

> In the patch, I'm using the existence of slash (`/`) to tell if `QEMU_XEN_PATH`
> is relative, and begin to search in PATH if it is. I'm sort of iffy on this,
> would it make more sense if we do this on inputs starting with a slash instead?
> And should we notify the user if it's not found anywhere in the PATH thus
> proceeding with the value configured in `QEMU_XEN_PATH` as-is?

Well, on one hand, that's how execvp() works, if there's a '/' in the
path, search in $PATH is skipped. But that would be a buggy scenario
because when you reboot a guest, the working directory isn't the same
anymore, at least with `xl`. So if `pwd`/dir/qemu-dm exist when you
execute `xl create`, but then run `reboot` in a guest, there's an "xl
daemon" that would try to reboot the guest with /dir/qemu-dm because
`pwd` would be just /.

So I think checking that QEMU_XEN_PATH is absolute would be enough here.

> Let me know if it's appropriate and if further changes are needed.
> 
> Best regards,
> Hongbo
> 
> ---
> CC: Anthony PERARD <anthony.perard@vates.tech>
> CC: Juergen Gross <jgross@suse.com>
> ---
>  tools/libs/light/libxl_dm.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> index 1f2f5bd97a..db05f20a5b 100644
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -332,7 +332,39 @@ const char *libxl__domain_device_model(libxl__gc *gc,
>              dm = libxl__abs_path(gc, "qemu-dm", libxl__private_bindir_path());
>              break;
>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> -            dm = qemu_xen_path(gc);
> +            const char *configured_dm = qemu_xen_path(gc);

Here, `clang` complained about that new variable, it expect an
expression. Can you put the "case" in a block {} ?

> +            if (strchr(configured_dm, '/'))

As discuste before, just checking for absolute path should be good
enough here.

> +                dm = libxl__strdup(gc, configured_dm);

The function used to return the value from qemu_xen_path(gc), so you can
keep returning `configured_dm`, no need for an strdup, here or the two
other strdup(configured_dm) below.

Also as the `else` part have a block {}, it would be better to have a
block {} for the "true" part of the `if` as well.


> +            else
> +            {
> +                const char *path_env = getenv("PATH");
> +                if (!path_env)
> +                    dm = libxl__strdup(gc, configured_dm);
> +                else
> +                {
> +                    char *path_dup = libxl__strdup(gc, path_env);
> +                    char *saveptr;
> +
> +                    char *path = strtok_r(path_dup, ":", &saveptr);
> +                    char fullpath[PATH_MAX];
> +                    bool dm_found = false;
> +                    while (path)
> +                    {
> +                        snprintf(fullpath, sizeof(fullpath), "%s/%s", path,
> +                                 configured_dm);
> +                        if (access(fullpath, X_OK) == 0)
> +                        {
> +                            dm = libxl__strdup(gc, fullpath);
> +                            dm_found = true;
> +                            break;
> +                        }
> +                        path = strtok_r(NULL, ":", &saveptr);
> +                    }
> +
> +                    if (!dm_found)
> +                        dm = libxl__strdup(gc, configured_dm);
> +                }
> +            }
>              break;
>          default:
>              LOG(ERROR, "invalid device model version %d",

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[XEN PATCH v2 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Hongbo]

`QEMU_XEN_PATH` will be configured as `qemu-system-i386` with no clue where, if
`--with-system-qemu` is set without giving a path (as matched in the case `yes`
but not `*`). However, the existence of the executable is checked by `access()`,
that will not look for anywhere in $PATH but the current directory. And since it
is possible for `qemu-system-i386` (or any other configured values) to be
executed from PATH later, we'd better find that in PATH and return the full path
for the caller to check against.

Signed-off-by: Hongbo <hehongbo@mail.com>
---
v2:
 - Identify absolute/relative paths with their first char (being `/` or not).
 - Put the case inside a block `{}` to address `clang` warnings about the new
   variable.
 - Avoid unnecessary string duplications.
 - Parity of using `{}` block on both sides of `if` statements.
 - Use `libxl__abs_path()` to get absolute paths.

Updated the patch as requested.

Also, I just realized that there is a `libxl__abs_path()` (occurred just above
my patched hunk), and I should utilize that instead of doing the `%s/%s`
`snprintf` thing myself.

Let me know if further changes are needed.

Best regards,
Hongbo
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_dm.c | 38 +++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index b193a5dc37..8f0bbd5d64 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -331,9 +331,43 @@ const char *libxl__domain_device_model(libxl__gc *gc,
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
             dm = libxl__abs_path(gc, "qemu-dm", libxl__private_bindir_path());
             break;
-        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            dm = qemu_xen_path(gc);
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: {
+            const char *configured_dm = qemu_xen_path(gc);
+            if (configured_dm[0] == '/')
+            {
+                dm = configured_dm;
+            }
+            else
+            {
+                const char *path_env = getenv("PATH");
+                if (!path_env)
+                {
+                    dm = configured_dm;
+                }
+                else
+                {
+                    char *path_dup = libxl__strdup(gc, path_env);
+                    char *saveptr;
+
+                    char *path = strtok_r(path_dup, ":", &saveptr);
+                    dm = NULL;
+                    while (path)
+                    {
+                        char *candidate = libxl__abs_path(gc, configured_dm, path);
+                        if (access(candidate, X_OK) == 0)
+                        {
+                            dm = candidate;
+                            break;
+                        }
+                        path = strtok_r(NULL, ":", &saveptr);
+                    }
+
+                    if (!dm)
+                        dm = configured_dm;
+                }
+            }
             break;
+        }
         default:
             LOG(ERROR, "invalid device model version %d",
                 info->device_model_version);
--
2.39.5 (Apple Git-154)

Re: [XEN PATCH v2 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Anthony PERARD]

On Mon, Mar 31, 2025 at 12:03:04AM +0800, Hongbo wrote:
> `QEMU_XEN_PATH` will be configured as `qemu-system-i386` with no clue where, if
> `--with-system-qemu` is set without giving a path (as matched in the case `yes`
> but not `*`). However, the existence of the executable is checked by `access()`,
> that will not look for anywhere in $PATH but the current directory. And since it
> is possible for `qemu-system-i386` (or any other configured values) to be
> executed from PATH later, we'd better find that in PATH and return the full path
> for the caller to check against.
> 
> Signed-off-by: Hongbo <hehongbo@mail.com>
> ---
> v2:
>  - Identify absolute/relative paths with their first char (being `/` or not).
>  - Put the case inside a block `{}` to address `clang` warnings about the new
>    variable.
>  - Avoid unnecessary string duplications.
>  - Parity of using `{}` block on both sides of `if` statements.
>  - Use `libxl__abs_path()` to get absolute paths.

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

 | Vates

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH v2 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Jan Beulich]

On 30.03.2025 18:03, Hongbo wrote:
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -331,9 +331,43 @@ const char *libxl__domain_device_model(libxl__gc *gc,
>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
>              dm = libxl__abs_path(gc, "qemu-dm", libxl__private_bindir_path());
>              break;
> -        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> -            dm = qemu_xen_path(gc);
> +        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: {
> +            const char *configured_dm = qemu_xen_path(gc);

What about this returning a filename with a path, just not an absolute one? $PATH
shouldn't be searched in such a case, should it?

> +            if (configured_dm[0] == '/')
> +            {
> +                dm = configured_dm;
> +            }

Why is this and ...

> +            else
> +            {
> +                const char *path_env = getenv("PATH");
> +                if (!path_env)
> +                {
> +                    dm = configured_dm;
> +                }

... this needed, when at the bottom dm is defaulted to dm_configured anyway?
You could set dm to dm_configured uniformly up front. Furthermore there's
then no real need for dm_configured then.

> +                else
> +                {
> +                    char *path_dup = libxl__strdup(gc, path_env);
> +                    char *saveptr;
> +
> +                    char *path = strtok_r(path_dup, ":", &saveptr);

Main reason I'm replying here is this one though, where CI found gcc to
object:

libxl_dm.c: In function 'libxl__domain_device_model':
libxl_dm.c:356:31: error: 'saveptr' may be used uninitialized in this function [-Werror=maybe-uninitialized]
                         char *candidate = libxl__abs_path(gc, configured_dm, path);
                               ^
cc1: all warnings being treated as errors

The compiler can't know that path_dup is guaranteed non-NULL. Hence, if it
can see (part of) the implementation of strtok_r(), it would observe that
it's possible that the continuation-invocation path is taken, where
saveptr necessarily is consumed.

Taking together all the issues I think I'll revert this, for you to make
another attempt. It may have been a mistake to commit such a change anyway;
I might better have left that to Anthony.

Jan

Re: [XEN PATCH v2 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Anthony PERARD]

On Tue, Apr 08, 2025 at 12:38:16PM +0200, Jan Beulich wrote:
> On 30.03.2025 18:03, Hongbo wrote:
> > +                {
> > +                    char *path_dup = libxl__strdup(gc, path_env);
> > +                    char *saveptr;
> > +
> > +                    char *path = strtok_r(path_dup, ":", &saveptr);
> 
> Main reason I'm replying here is this one though, where CI found gcc to
> object:
> 
> libxl_dm.c: In function 'libxl__domain_device_model':
> libxl_dm.c:356:31: error: 'saveptr' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>                          char *candidate = libxl__abs_path(gc, configured_dm, path);
>                                ^
> cc1: all warnings being treated as errors
> 
> The compiler can't know that path_dup is guaranteed non-NULL. Hence, if it
> can see (part of) the implementation of strtok_r(), it would observe that
> it's possible that the continuation-invocation path is taken, where
> saveptr necessarily is consumed.

I'll recommit this patch again with `saveptr` initialised to NULL. My
man page says some implementation requires this anyway.

Cheers,

-- 
Anthony PERARD

[XEN PATCH v2 RESEND 1/1] tools/libxl: search PATH for QEMU if `QEMU_XEN_PATH` is not absolute

[Hongbo]

`QEMU_XEN_PATH` will be configured as `qemu-system-i386` with no clue where, if
`--with-system-qemu` is set without giving a path (as matched in the case `yes`
but not `*`). However, the existence of the executable is checked by `access()`,
that will not look for anywhere in $PATH but the current directory. And since it
is possible for `qemu-system-i386` (or any other configured values) to be
executed from PATH later, we'd better find that in PATH and return the full path
for the caller to check against.

Signed-off-by: Hongbo <hehongbo@mail.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
---
No changes since v2. Resending to add `Reviewed-by` from Anthony PERARD.

Best regards,
Hongbo
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libs/light/libxl_dm.c | 38 +++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index b193a5dc37..8f0bbd5d64 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -331,9 +331,43 @@ const char *libxl__domain_device_model(libxl__gc *gc,
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
             dm = libxl__abs_path(gc, "qemu-dm", libxl__private_bindir_path());
             break;
-        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            dm = qemu_xen_path(gc);
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: {
+            const char *configured_dm = qemu_xen_path(gc);
+            if (configured_dm[0] == '/')
+            {
+                dm = configured_dm;
+            }
+            else
+            {
+                const char *path_env = getenv("PATH");
+                if (!path_env)
+                {
+                    dm = configured_dm;
+                }
+                else
+                {
+                    char *path_dup = libxl__strdup(gc, path_env);
+                    char *saveptr;
+
+                    char *path = strtok_r(path_dup, ":", &saveptr);
+                    dm = NULL;
+                    while (path)
+                    {
+                        char *candidate = libxl__abs_path(gc, configured_dm, path);
+                        if (access(candidate, X_OK) == 0)
+                        {
+                            dm = candidate;
+                            break;
+                        }
+                        path = strtok_r(NULL, ":", &saveptr);
+                    }
+
+                    if (!dm)
+                        dm = configured_dm;
+                }
+            }
             break;
+        }
         default:
             LOG(ERROR, "invalid device model version %d",
                 info->device_model_version);
--
2.39.5 (Apple Git-154)