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