Re: [PATCH v2] linux-user: Clean up arg_start/arg_end confusion

[Laurent Vivier <laurent@vivier.eu>]

Le 27/04/2022 à 04:51, Richard Henderson a écrit :
> We had two sets of variables: arg_start/arg_end, and
> arg_strings/env_strings.  In linuxload.c, we set the
> first pair to the bounds of the argv strings, but in
> elfload.c, we set the first pair to the bounds of the
> argv pointers and the second pair to the bounds of
> the argv strings.
> 
> Remove arg_start/arg_end, replacing them with the standard
> argc/argv/envc/envp values.  Retain arg_strings/env_strings
> with the meaning we were using in elfload.c.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/714
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/qemu.h             | 12 ++++++++----
>   linux-user/elfload.c          | 10 ++++++----
>   linux-user/linuxload.c        | 12 ++++++++++--
>   linux-user/main.c             |  4 ++--
>   semihosting/arm-compat-semi.c |  4 ++--
>   5 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 46550f5e21..7d90de1b15 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -40,15 +40,19 @@ struct image_info {
>           abi_ulong       data_offset;
>           abi_ulong       saved_auxv;
>           abi_ulong       auxv_len;
> -        abi_ulong       arg_start;
> -        abi_ulong       arg_end;
> -        abi_ulong       arg_strings;
> -        abi_ulong       env_strings;
> +        abi_ulong       argc;
> +        abi_ulong       argv;
> +        abi_ulong       envc;
> +        abi_ulong       envp;
>           abi_ulong       file_string;
>           uint32_t        elf_flags;
>           int             personality;
>           abi_ulong       alignment;
>   
> +        /* Generic semihosting knows about these pointers. */
> +        abi_ulong       arg_strings;   /* strings for argv */
> +        abi_ulong       env_strings;   /* strings for envp; ends arg_strings */
> +
>           /* The fields below are used in FDPIC mode.  */
>           abi_ulong       loadmap_addr;
>           uint16_t        nsegs;
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 61063fd974..8c0765dd4b 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1516,8 +1516,8 @@ static inline void init_thread(struct target_pt_regs *regs,
>       regs->iaoq[0] = infop->entry;
>       regs->iaoq[1] = infop->entry + 4;
>       regs->gr[23] = 0;
> -    regs->gr[24] = infop->arg_start;
> -    regs->gr[25] = (infop->arg_end - infop->arg_start) / sizeof(abi_ulong);
> +    regs->gr[24] = infop->argv;
> +    regs->gr[25] = infop->argc;
>       /* The top-of-stack contains a linkage buffer.  */
>       regs->gr[30] = infop->start_stack + 64;
>       regs->gr[31] = infop->entry;
> @@ -2120,8 +2120,10 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
>       u_envp = u_argv + (argc + 1) * n;
>       u_auxv = u_envp + (envc + 1) * n;
>       info->saved_auxv = u_auxv;
> -    info->arg_start = u_argv;
> -    info->arg_end = u_argv + argc * n;
> +    info->argc = argc;
> +    info->envc = envc;
> +    info->argv = u_argv;
> +    info->envp = u_envp;
>   
>       /* This is correct because Linux defines
>        * elf_addr_t as Elf32_Off / Elf64_Off
> diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
> index 2ed5fc45ed..745cce70ab 100644
> --- a/linux-user/linuxload.c
> +++ b/linux-user/linuxload.c
> @@ -92,6 +92,11 @@ abi_ulong loader_build_argptr(int envc, int argc, abi_ulong sp,
>       envp = sp;
>       sp -= (argc + 1) * n;
>       argv = sp;
> +    ts->info->envp = envp;
> +    ts->info->envc = envc;
> +    ts->info->argv = argv;
> +    ts->info->argc = argc;
> +
>       if (push_ptr) {
>           /* FIXME - handle put_user() failures */
>           sp -= n;
> @@ -99,19 +104,22 @@ abi_ulong loader_build_argptr(int envc, int argc, abi_ulong sp,
>           sp -= n;
>           put_user_ual(argv, sp);
>       }
> +
>       sp -= n;
>       /* FIXME - handle put_user() failures */
>       put_user_ual(argc, sp);
> -    ts->info->arg_start = stringp;
> +
> +    ts->info->arg_strings = stringp;
>       while (argc-- > 0) {
>           /* FIXME - handle put_user() failures */
>           put_user_ual(stringp, argv);
>           argv += n;
>           stringp += target_strlen(stringp) + 1;
>       }
> -    ts->info->arg_end = stringp;
>       /* FIXME - handle put_user() failures */
>       put_user_ual(0, argv);
> +
> +    ts->info->env_strings = stringp;
>       while (envc-- > 0) {
>           /* FIXME - handle put_user() failures */
>           put_user_ual(stringp, envp);
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 7ca48664e4..651e32f5f2 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -878,9 +878,9 @@ int main(int argc, char **argv, char **envp)
>               fprintf(f, "entry       0x" TARGET_ABI_FMT_lx "\n",
>                       info->entry);
>               fprintf(f, "argv_start  0x" TARGET_ABI_FMT_lx "\n",
> -                    info->arg_start);
> +                    info->argv);
>               fprintf(f, "env_start   0x" TARGET_ABI_FMT_lx "\n",
> -                    info->arg_end + (abi_ulong)sizeof(abi_ulong));
> +                    info->envp);
>               fprintf(f, "auxv_start  0x" TARGET_ABI_FMT_lx "\n",
>                       info->saved_auxv);
>               qemu_log_unlock(f);
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 7a51fd0737..b6ddaf863a 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -1106,7 +1106,7 @@ target_ulong do_common_semihosting(CPUState *cs)
>   #else
>               unsigned int i;
>   
> -            output_size = ts->info->arg_end - ts->info->arg_start;
> +            output_size = ts->info->env_strings - ts->info->arg_strings;
>               if (!output_size) {
>                   /*
>                    * We special-case the "empty command line" case (argc==0).
> @@ -1146,7 +1146,7 @@ target_ulong do_common_semihosting(CPUState *cs)
>                   goto out;
>               }
>   
> -            if (copy_from_user(output_buffer, ts->info->arg_start,
> +            if (copy_from_user(output_buffer, ts->info->arg_strings,
>                                  output_size)) {
>                   errno = EFAULT;
>                   status = set_swi_errno(cs, -1);

Applied to my linux-user-for-7.1 branch.

Thanks,
Laurent