diff mbox series

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

Message ID 20220427025129.160184-1-richard.henderson@linaro.org
State New
Headers show
Series [v2] linux-user: Clean up arg_start/arg_end confusion | expand

Commit Message

Richard Henderson April 27, 2022, 2:51 a.m. UTC
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(-)

Comments

Richard Henderson May 21, 2022, 5:19 a.m. UTC | #1
Ping?

r~

On 4/26/22 19:51, Richard Henderson wrote:
> 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);
Philippe Mathieu-Daudé May 22, 2022, 9:46 p.m. UTC | #2
On 27/4/22 04:51, Richard Henderson wrote:
> 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(-)

Nice simplification.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Laurent Vivier May 23, 2022, 6:14 a.m. UTC | #3
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
diff mbox series

Patch

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);