Re: [PATCH V4] util/oslib-posix : qemu_init_exec_dir implementation for Mac

[Peter Maydell]

On Tue, 16 Jun 2020 at 17:12, David CARLIER <devnexen@gmail.com> wrote:
>
> From 7eef8b803cdb0e7148fdf894d2992052695c1ff8 Mon Sep 17 00:00:00 2001
> From: David Carlier <devnexen@gmail.com>
> Date: Tue, 26 May 2020 21:35:27 +0100
> Subject: [PATCH] util/oslib-posix : qemu_init_exec_dir implementation for Mac
>
> Using dyld API to get the full path of the current process.
>
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
>  util/oslib-posix.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 916f1be224..3612c2c80e 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -57,6 +57,10 @@
>  #include <lwp.h>
>  #endif
>
> +#ifdef __APPLE__
> +#include <mach-o/dyld.h>
> +#endif
> +
>  #include "qemu/mmap-alloc.h"
>
>  #ifdef CONFIG_DEBUG_STACK_USAGE
> @@ -375,6 +379,16 @@ void qemu_init_exec_dir(const char *argv0)
>              p = buf;
>          }
>      }
> +#elif defined(__APPLE__)
> +    {
> +        uint32_t len = sizeof(buf);
> +        if (_NSGetExecutablePath(buf, &len) == 0) {
> +     char fpath[PATH_MAX];

(Something seems to have gone wrong with the indentation here;
QEMU indent is always-spaces, 4-spaces-per-indent.)

> +            buf[len - 1] = 0;

_NSGetExecutablePath() will 0-terminate the string, so
you don't need to write this 0 here. (The function call
will fail if the buffer is not long enough for both the
path string and the terminating NUL character.)

> +      realpath(buf, fpath);

You need to check the return value from realpath() in
case it returns an error; compare the code just below
that handles calling realpath() on argv0.

> +            p = fpath;

This is setting p to a pointer to a buffer which goes
out of scope, and then using it after it's out of scope,
which is undefined behaviour. You can avoid that by
doing _NSGetExecutablePath() into the tightly-scoped
'fpath[]', and then doing realpath() into 'buf[]'
(which is still live to the end of the function).

> +        }
> +    }
>  #endif
>      /* If we don't have any way of figuring out the actual executable
>         location then try argv[0].  */

thanks
-- PMM