[RFC,v2,6/6] linux-user: Add '-native-bypass' option

Signed-off-by: Yeqi Fu <fufuyqqqqqq@gmail.com>
---
 include/qemu/envlist.h |  1 +
 linux-user/main.c      | 23 +++++++++++++++++
 util/envlist.c         | 56 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+)

On Wed, 07 Jun 2023 19:47, Yeqi Fu <fufuyqqqqqq@gmail.com> wrote:
>--- a/linux-user/main.c
>+++ b/linux-user/main.c
>+    /* Set the library for native bypass  */
>+    if (native_lib != NULL) {
>+        char *token = malloc(strlen(native_lib) + 12);

malloc() can fail (in rare circumstances). Check for the return value 
here. Or use g_malloc() which terminates on alloc failure.

>+        strcpy(token, "LD_PRELOAD=");
>+        strcat(token, native_lib);

(You could alternatively use snprintf() here)

> 
>diff --git a/util/envlist.c b/util/envlist.c
>index db937c0427..713d52497e 100644
>+int
>+envlist_appendenv(envlist_t *envlist, const char *env, const char *separator)
>+{
>+    struct envlist_entry *entry = NULL;
>+    const char *eq_sign;
>+    size_t envname_len;
>+
>+    if ((envlist == NULL) || (env == NULL)) {

separator must not be NULL as well.

>+        return (EINVAL);

Unnecessary parentheses here and in later returns.

>+    }
>+
>+    /* find out first equals sign in given env */
>+    eq_sign = strchr(env, '=');
>+    if (eq_sign == NULL) {

Perhaps you can return an error message to the user here also, and 
explain why it failed. You can do that by passing an error message 
pointer with the function arguments.

By the way, if strchr(strchr(env, '='), '=') != NULL, shouldn't this 
fail also?

Manos Pitsidianakis <manos.pitsidianakis@linaro.org> writes:

> On Wed, 07 Jun 2023 19:47, Yeqi Fu <fufuyqqqqqq@gmail.com> wrote:
>>--- a/linux-user/main.c
>>+++ b/linux-user/main.c
>>+    /* Set the library for native bypass  */
>>+    if (native_lib != NULL) {
>>+        char *token = malloc(strlen(native_lib) + 12);
>
> malloc() can fail (in rare circumstances). Check for the return value
> here. Or use g_malloc() which terminates on alloc failure.

We avoid malloc in favour of g_malloc(). You can use g_try_malloc for
certain cases (although this is not one of them). However you can make
this glibs problem with something like:

    /* Set the library for native bypass  */
    if (native_lib != NULL) {
        GString *lib = g_string_new(native_lib);
        lib = g_string_prepend(lib, "LD_PRELOAD=");
        if (envlist_appendenv(envlist, g_string_free(lib, false), ":") != 0) {
            usage(EXIT_FAILURE);
        }
    }


>
>>+        strcpy(token, "LD_PRELOAD=");
>>+        strcat(token, native_lib);
>
> (You could alternatively use snprintf() here)

We have a section on strings in the developer manual:

 https://qemu.readthedocs.io/en/latest/devel/style.html#string-manipulation

so we have things like pstrcat and pstrcpy. However this isn't criticl
performance path so GString provides a nice memory safe wrapper for all
this sort of manipulation.

<snip>

Yeqi Fu <fufuyqqqqqq@gmail.com> writes:

> Signed-off-by: Yeqi Fu <fufuyqqqqqq@gmail.com>
> ---
>  include/qemu/envlist.h |  1 +
>  linux-user/main.c      | 23 +++++++++++++++++
>  util/envlist.c         | 56 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 80 insertions(+)
>
> diff --git a/include/qemu/envlist.h b/include/qemu/envlist.h
> index 6006dfae44..865eb18e17 100644
> --- a/include/qemu/envlist.h
> +++ b/include/qemu/envlist.h
> @@ -7,6 +7,7 @@ envlist_t *envlist_create(void);
>  void envlist_free(envlist_t *);
>  int envlist_setenv(envlist_t *, const char *);
>  int envlist_unsetenv(envlist_t *, const char *);
> +int envlist_appendenv(envlist_t *, const char *, const char *);
>  int envlist_parse_set(envlist_t *, const char *);
>  int envlist_parse_unset(envlist_t *, const char *);
>  char **envlist_to_environ(const envlist_t *, size_t *);
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 5e6b2e1714..313c116b3b 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -125,6 +125,8 @@ static void usage(int exitcode);
>  static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
>  const char *qemu_uname_release;
>  
> +static const char *native_lib;
> +
>  #if !defined(TARGET_DEFAULT_STACK_SIZE)
>  /* XXX: on x86 MAP_GROWSDOWN only works if ESP <= address + 32, so
>     we allocate a bigger stack. Need a better solution, for example
> @@ -293,6 +295,13 @@ static void handle_arg_set_env(const char *arg)
>      free(r);
>  }
>  
> +#if defined(CONFIG_USER_ONLY)  && defined(CONFIG_USER_NATIVE_CALL)

CONFIG_USER_ONLY is a pointless check as by definition this file is
user-mode only.

> +static void handle_arg_native_bypass(const char *arg)
> +{
> +    native_lib = arg;

canonicalise the path and maybe verify it exists/is accessible?

> +}
> +#endif
> +
>  static void handle_arg_unset_env(const char *arg)
>  {
>      char *r, *p, *token;
> @@ -522,6 +531,10 @@ static const struct qemu_argument arg_table[] = {
>       "",           "Generate a /tmp/perf-${pid}.map file for perf"},
>      {"jitdump",    "QEMU_JITDUMP",     false, handle_arg_jitdump,
>       "",           "Generate a jit-${pid}.dump file for perf"},
> +#if defined(CONFIG_USER_ONLY)  && defined(CONFIG_USER_NATIVE_CALL)

see above re CONFIG_USER_ONLY.

> +    {"native-bypass", "QEMU_NATIVE_BYPASS", true, handle_arg_native_bypass,
> +     "",           "native bypass for library calls in user mode only."},
> +#endif
>      {NULL, NULL, false, NULL, NULL, NULL}
>  };
>  
> @@ -826,6 +839,16 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    /* Set the library for native bypass  */
> +    if (native_lib != NULL) {
> +        char *token = malloc(strlen(native_lib) + 12);
> +        strcpy(token, "LD_PRELOAD=");
> +        strcat(token, native_lib);
> +         if (envlist_appendenv(envlist, token, ":") != 0) {
> +            usage(EXIT_FAILURE);
> +        }
> +    }
> +
>      target_environ = envlist_to_environ(envlist, NULL);
>      envlist_free(envlist);
>  
> diff --git a/util/envlist.c b/util/envlist.c
> index db937c0427..713d52497e 100644
> --- a/util/envlist.c
> +++ b/util/envlist.c
> @@ -201,6 +201,62 @@ envlist_unsetenv(envlist_t *envlist, const char *env)
>      return (0);
>  }
>

I think adding this function should be a separate commit. It would be
nice to add some unittests for the functionality while we are at it.

> +/*
> + * Appends environment value to envlist. If the environment
> + * variable already exists, the new value is appended to the
> + * existing one.
> + *
> + * Returns 0 in success, errno otherwise.
> + */
> +int
> +envlist_appendenv(envlist_t *envlist, const char *env, const char *separator)
> +{
> +    struct envlist_entry *entry = NULL;
> +    const char *eq_sign;
> +    size_t envname_len;
> +
> +    if ((envlist == NULL) || (env == NULL)) {
> +        return (EINVAL);
> +    }
> +
> +    /* find out first equals sign in given env */
> +    eq_sign = strchr(env, '=');
> +    if (eq_sign == NULL) {
> +        return (EINVAL);
> +    }
> +    envname_len = eq_sign - env + 1;
> +
> +    /*
> +     * If there already exists variable with given name,
> +     * we append the new value to the existing one.
> +     */
> +    for (entry = envlist->el_entries.lh_first; entry != NULL;
> +        entry = entry->ev_link.le_next) {
> +        if (strncmp(entry->ev_var, env, envname_len) == 0) {
> +            break;
> +        }
> +    }
> +
> +    if (entry != NULL) {
> +        char *new_env_value = NULL;
> +        size_t new_env_len = strlen(entry->ev_var) + strlen(eq_sign)
> +            + strlen(separator) + 1;
> +        new_env_value = g_malloc(new_env_len);
> +        strcpy(new_env_value, entry->ev_var);
> +        strcat(new_env_value, separator);
> +        strcat(new_env_value, eq_sign + 1);

See other comments about string functions.

> +        g_free((char *)entry->ev_var);
> +        entry->ev_var = new_env_value;
> +    } else {
> +        envlist->el_count++;
> +        entry = g_malloc(sizeof(*entry));
> +        entry->ev_var = g_strdup(env);
> +        QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link);
> +    }
> +
> +    return (0);
> +}
> +
>  /*
>   * Returns given envlist as array of strings (in same form that
>   * global variable environ is).  Caller must free returned memory