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

["Anthony PERARD" <anthony.perard@vates.tech>]

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