| Message ID | 8693b4ab-4be5-4d99-b19e-f3b42641c67a@HUB2.rwth-ad.de |
|---|---|
| State | New |
| Headers | show |
On Thursday 27 August 2015 21:35:41 Stefan Brüns wrote: PING! > Instead of creating a temporary copy for the whole environment and > the arguments, directly copy everything to the target stack. > > For this to work, we have to change the order of stack creation and > copying the arguments. > > v2: fixed scratch pointer type, fixed checkpatch issues. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
On 30 August 2015 at 16:52, Stefan Bruens <stefan.bruens@rwth-aachen.de> wrote: > On Thursday 27 August 2015 21:35:41 Stefan Brüns wrote: > > PING! Hi. This is on my to-review list, but so are a lot of other things. It might take me a week or so to get to it. thanks -- PMM
On 27 August 2015 at 20:35, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > Instead of creating a temporary copy for the whole environment and > the arguments, directly copy everything to the target stack. > For this to work, we have to change the order of stack creation and > copying the arguments. > > v2: fixed scratch pointer type, fixed checkpatch issues. This sort of 'v1 to v2 diffs' comment should go below the '---' line, so it doesn't end up in the final git commit message. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > linux-user/elfload.c | 110 ++++++++++++++++++++++++------------------------- > linux-user/linuxload.c | 7 +--- > linux-user/qemu.h | 7 ---- > linux-user/syscall.c | 6 --- > 4 files changed, 56 insertions(+), 74 deletions(-) Still doesn't compile: /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c: In function ‘loader_exec’: /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c:138:9: error: unused variable ‘i’ [-Werror=unused-variable] int i; ^ Mostly this looks good; I have a few review comments below. > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index 9c999ac..991dd35 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > + int bytes_to_copy = (len > offset) ? offset : len; > + tmp -= bytes_to_copy; > + p -= bytes_to_copy; > + offset -= bytes_to_copy; > + len -= bytes_to_copy; > + > + if (bytes_to_copy == 1) { > + *(scratch + offset) = *tmp; > + } else { > + memcpy_fromfs(scratch + offset, tmp, bytes_to_copy); Why bother to special case the 1 byte case? > } > - else { > - int bytes_to_copy = (len > offset) ? offset : len; > - tmp -= bytes_to_copy; > - p -= bytes_to_copy; > - offset -= bytes_to_copy; > - len -= bytes_to_copy; > - memcpy_fromfs(pag + offset, tmp, bytes_to_copy + 1); > + > + if (offset == 0) { > + memcpy_to_target(p, scratch, top - p); > + top = p; > + offset = TARGET_PAGE_SIZE; > } > } > } > + if (offset) { > + memcpy_to_target(p, scratch + offset, top - p); > + } > + > return p; > } > > -static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, > +static abi_ulong setup_arg_pages(struct linux_binprm *bprm, > struct image_info *info) > { > - abi_ulong stack_base, size, error, guard; > - int i; > + abi_ulong size, error, guard; > + > + /* Linux kernel limits argument/environment space to 1/4th of stack size, > + but also has a floor of 32 pages. Mimic this behaviour. */ > + #define MAX_ARG_PAGES 32 This sounds like a minimum, not a maximum, so the name is very misleading. The comment says "limit to 1/4 of stack size" but the code doesn't seem to do anything like that. Please move the #define to top-level in the file, not hidden inside a function definition. > > - /* Create enough stack to hold everything. If we don't use > - it for args, we'll use it for something else. */ > size = guest_stack_size; > - if (size < MAX_ARG_PAGES*TARGET_PAGE_SIZE) { > - size = MAX_ARG_PAGES*TARGET_PAGE_SIZE; > + if (size < MAX_ARG_PAGES * TARGET_PAGE_SIZE) { > + size = MAX_ARG_PAGES * TARGET_PAGE_SIZE; > } > guard = TARGET_PAGE_SIZE; > if (guard < qemu_real_host_page_size) { > @@ -1442,19 +1446,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, > target_mprotect(error, guard, PROT_NONE); > > info->stack_limit = error + guard; > - stack_base = info->stack_limit + size - MAX_ARG_PAGES*TARGET_PAGE_SIZE; > - p += stack_base; > - > - for (i = 0 ; i < MAX_ARG_PAGES ; i++) { > - if (bprm->page[i]) { > - info->rss++; I think your patch has lost the calculation of info->rss. (We don't actually use info->rss anywhere, though, so if you wanted to add a patch in front of this one explicitly removing it as useless that would be an OK way to handle this.) > - /* FIXME - check return value of memcpy_to_target() for failure */ > - memcpy_to_target(stack_base, bprm->page[i], TARGET_PAGE_SIZE); > - g_free(bprm->page[i]); > - } > - stack_base += TARGET_PAGE_SIZE; > - } > - return p; > + > + return info->stack_limit + size - sizeof(void *); > } > > /* Map and zero the bss. We need to explicitly zero any fractional pages > diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c > index 506e837..09df934 100644 > --- a/linux-user/linuxload.c > +++ b/linux-user/linuxload.c > @@ -137,8 +137,7 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp, > int retval; > int i; > > - bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int); > - memset(bprm->page, 0, sizeof(bprm->page)); > + bprm->p = 0; Nothing actually uses this value -- both the elfload and the flatload code paths now either ignore bprm->p or set it themselves. It would be better to delete this and also the dead assignment "p = bprm->p" at the top of load_flt_binary(). > bprm->fd = fdexec; > bprm->filename = (char *)filename; > bprm->argc = count(argv); thanks -- PMM
On Tuesday, September 01, 2015 15:29:26 you wrote: > On 27 August 2015 at 20:35, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote: > > Instead of creating a temporary copy for the whole environment and > > the arguments, directly copy everything to the target stack. > > > > For this to work, we have to change the order of stack creation and > > copying the arguments. > > > > v2: fixed scratch pointer type, fixed checkpatch issues. > > This sort of 'v1 to v2 diffs' comment should go below the '---' > line, so it doesn't end up in the final git commit message. > > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > --- > > > > linux-user/elfload.c | 110 > > ++++++++++++++++++++++++------------------------- linux-user/linuxload.c > > | 7 +--- > > linux-user/qemu.h | 7 ---- > > linux-user/syscall.c | 6 --- > > 4 files changed, 56 insertions(+), 74 deletions(-) > > Still doesn't compile: > > /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c: In > function ‘loader_exec’: > /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c:138:9: > error: unused variable ‘i’ [-Werror=unused-variable] > int i; > ^ > > Mostly this looks good; I have a few review comments below. > > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > > index 9c999ac..991dd35 100644 > > --- a/linux-user/elfload.c > > +++ b/linux-user/elfload.c > > > > + int bytes_to_copy = (len > offset) ? offset : len; > > + tmp -= bytes_to_copy; > > + p -= bytes_to_copy; > > + offset -= bytes_to_copy; > > + len -= bytes_to_copy; > > + > > + if (bytes_to_copy == 1) { > > + *(scratch + offset) = *tmp; > > + } else { > > + memcpy_fromfs(scratch + offset, tmp, bytes_to_copy); > > Why bother to special case the 1 byte case? Taken from the old code. Probably not worth the extra code, will remove it. > > } > > > > - else { > > - int bytes_to_copy = (len > offset) ? offset : len; > > - tmp -= bytes_to_copy; > > - p -= bytes_to_copy; > > - offset -= bytes_to_copy; > > - len -= bytes_to_copy; > > - memcpy_fromfs(pag + offset, tmp, bytes_to_copy + 1); > > + > > + if (offset == 0) { > > + memcpy_to_target(p, scratch, top - p); > > + top = p; > > + offset = TARGET_PAGE_SIZE; > > > > } > > > > } > > > > } > > > > + if (offset) { > > + memcpy_to_target(p, scratch + offset, top - p); > > + } > > + > > > > return p; > > > > } > > > > -static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, > > +static abi_ulong setup_arg_pages(struct linux_binprm *bprm, > > > > struct image_info *info) > > > > { > > > > - abi_ulong stack_base, size, error, guard; > > - int i; > > + abi_ulong size, error, guard; > > + > > + /* Linux kernel limits argument/environment space to 1/4th of stack > > size, + but also has a floor of 32 pages. Mimic this behaviour. */ > > + #define MAX_ARG_PAGES 32 > > This sounds like a minimum, not a maximum, so the name is very > misleading. > > The comment says "limit to 1/4 of stack size" but the code doesn't > seem to do anything like that. > > Please move the #define to top-level in the file, not hidden > inside a function definition. MAX_ARG_PAGES is the name used in old kernels (and current, when there is no MMU), so it is useful to keep this name (maybe in a comment). What about: --- /* Older linux kernels provide up to MAX_ARG_PAGES (default: 32) of * argument/environment space. Newer kernels (>2.6.33) provide up to * 1/4th of stack size, but guarantee at least 32 pages for backwards * compatibility. */ #define STACK_LOWER_LIMIT (32 * TARGET_PAGE_SIZE) --- The 1/4th is not enforced, as our stack has to be big enough to hold args/env coming from the kernel, but is no problem if we provide more space. Thinking about it, maybe we should provide some extra space, as qemu-linux- user puts some more stuff (e.g. auxv) between env/args and user stack. > > - /* Create enough stack to hold everything. If we don't use > > - it for args, we'll use it for something else. */ > > > > size = guest_stack_size; > > > > - if (size < MAX_ARG_PAGES*TARGET_PAGE_SIZE) { > > - size = MAX_ARG_PAGES*TARGET_PAGE_SIZE; > > + if (size < MAX_ARG_PAGES * TARGET_PAGE_SIZE) { > > + size = MAX_ARG_PAGES * TARGET_PAGE_SIZE; > > > > } > > guard = TARGET_PAGE_SIZE; > > if (guard < qemu_real_host_page_size) { > > > > @@ -1442,19 +1446,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, > > struct linux_binprm *bprm,> > > target_mprotect(error, guard, PROT_NONE); > > > > info->stack_limit = error + guard; > > > > - stack_base = info->stack_limit + size - > > MAX_ARG_PAGES*TARGET_PAGE_SIZE; - p += stack_base; > > - > > - for (i = 0 ; i < MAX_ARG_PAGES ; i++) { > > - if (bprm->page[i]) { > > - info->rss++; > > I think your patch has lost the calculation of info->rss. > > (We don't actually use info->rss anywhere, though, so if you wanted > to add a patch in front of this one explicitly removing it as useless > that would be an OK way to handle this.) Will add an explicit removal patch. info->mmap is not used either, ok to remove both in one go? > > - /* FIXME - check return value of memcpy_to_target() for > > failure */ - memcpy_to_target(stack_base, bprm->page[i], > > TARGET_PAGE_SIZE); - g_free(bprm->page[i]); > > - } > > - stack_base += TARGET_PAGE_SIZE; > > - } > > - return p; > > + > > + return info->stack_limit + size - sizeof(void *); > > > > } > > > > /* Map and zero the bss. We need to explicitly zero any fractional pages > > > > diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c > > index 506e837..09df934 100644 > > --- a/linux-user/linuxload.c > > +++ b/linux-user/linuxload.c > > @@ -137,8 +137,7 @@ int loader_exec(int fdexec, const char *filename, char > > **argv, char **envp,> > > int retval; > > int i; > > > > - bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int); > > - memset(bprm->page, 0, sizeof(bprm->page)); > > + bprm->p = 0; > > Nothing actually uses this value -- both the elfload and the flatload code > paths now either ignore bprm->p or set it themselves. It would be > better to delete this and also the dead assignment "p = bprm->p" at > the top of load_flt_binary(). OK to do this in a followup patch? > > bprm->fd = fdexec; > > bprm->filename = (char *)filename; > > bprm->argc = count(argv); > > thanks > -- PMM Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424
On 1 September 2015 at 18:27, Brüns, Stefan <Stefan.Bruens@rwth-aachen.de> wrote: > On Tuesday, September 01, 2015 15:29:26 you wrote: >> On 27 August 2015 at 20:35, Stefan Brüns <stefan.bruens@rwth-aachen.de> > wrote: >> > Instead of creating a temporary copy for the whole environment and >> > the arguments, directly copy everything to the target stack. >> > >> > For this to work, we have to change the order of stack creation and >> > copying the arguments. >> > >> > v2: fixed scratch pointer type, fixed checkpatch issues. >> >> This sort of 'v1 to v2 diffs' comment should go below the '---' >> line, so it doesn't end up in the final git commit message. >> >> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> >> > --- >> > >> > linux-user/elfload.c | 110 >> > ++++++++++++++++++++++++------------------------- linux-user/linuxload.c >> > | 7 +--- >> > linux-user/qemu.h | 7 ---- >> > linux-user/syscall.c | 6 --- >> > 4 files changed, 56 insertions(+), 74 deletions(-) >> >> Still doesn't compile: >> >> /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c: In >> function ‘loader_exec’: >> /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c:138:9: >> error: unused variable ‘i’ [-Werror=unused-variable] >> int i; >> ^ >> >> Mostly this looks good; I have a few review comments below. >> >> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c >> > index 9c999ac..991dd35 100644 >> > --- a/linux-user/elfload.c >> > +++ b/linux-user/elfload.c >> > >> > + int bytes_to_copy = (len > offset) ? offset : len; >> > + tmp -= bytes_to_copy; >> > + p -= bytes_to_copy; >> > + offset -= bytes_to_copy; >> > + len -= bytes_to_copy; >> > + >> > + if (bytes_to_copy == 1) { >> > + *(scratch + offset) = *tmp; >> > + } else { >> > + memcpy_fromfs(scratch + offset, tmp, bytes_to_copy); >> >> Why bother to special case the 1 byte case? > > Taken from the old code. Probably not worth the extra code, will remove it. Oops, I thought I'd checked the diff to check it wasn't already present, but I must have misread it. >> > + /* Linux kernel limits argument/environment space to 1/4th of stack >> > size, + but also has a floor of 32 pages. Mimic this behaviour. */ >> > + #define MAX_ARG_PAGES 32 >> >> This sounds like a minimum, not a maximum, so the name is very >> misleading. >> >> The comment says "limit to 1/4 of stack size" but the code doesn't >> seem to do anything like that. >> >> Please move the #define to top-level in the file, not hidden >> inside a function definition. > > MAX_ARG_PAGES is the name used in old kernels (and current, when there is > no MMU), so it is useful to keep this name (maybe in a comment). > > What about: > --- > /* Older linux kernels provide up to MAX_ARG_PAGES (default: 32) of > * argument/environment space. Newer kernels (>2.6.33) provide up to > * 1/4th of stack size, but guarantee at least 32 pages for backwards > * compatibility. */ > #define STACK_LOWER_LIMIT (32 * TARGET_PAGE_SIZE) > --- OK. (Incidentally I prefer the trailing "*/" of a multiline comment on its own line.) > The 1/4th is not enforced, as our stack has to be big enough to hold args/env > coming from the kernel, but is no problem if we provide more space. That's OK, but then we should either (a) just not mention the 1/4 limit in the comment or (b) mention it and say we don't enforce the limit in QEMU. >> I think your patch has lost the calculation of info->rss. >> >> (We don't actually use info->rss anywhere, though, so if you wanted >> to add a patch in front of this one explicitly removing it as useless >> that would be an OK way to handle this.) > > Will add an explicit removal patch. info->mmap is not used either, ok to > remove both in one go? Yes. >> > --- a/linux-user/linuxload.c >> > +++ b/linux-user/linuxload.c >> > @@ -137,8 +137,7 @@ int loader_exec(int fdexec, const char *filename, char >> > **argv, char **envp,> >> > int retval; >> > int i; >> > >> > - bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int); >> > - memset(bprm->page, 0, sizeof(bprm->page)); >> > + bprm->p = 0; >> >> Nothing actually uses this value -- both the elfload and the flatload code >> paths now either ignore bprm->p or set it themselves. It would be >> better to delete this and also the dead assignment "p = bprm->p" at >> the top of load_flt_binary(). > > OK to do this in a followup patch? If you want to remove the dead assignment in its own patch I would do that before this patch, rather than after. thanks -- PMM
On Tuesday 01 September 2015 19:42:26 you wrote: > >> > --- a/linux-user/linuxload.c > >> > +++ b/linux-user/linuxload.c > >> > @@ -137,8 +137,7 @@ int loader_exec(int fdexec, const char *filename, > >> > char > >> > **argv, char **envp,> > >> > > >> > int retval; > >> > int i; > >> > > >> > - bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int); > >> > - memset(bprm->page, 0, sizeof(bprm->page)); > >> > + bprm->p = 0; > >> > >> Nothing actually uses this value -- both the elfload and the flatload > >> code > >> paths now either ignore bprm->p or set it themselves. It would be > >> better to delete this and also the dead assignment "p = bprm->p" at > >> the top of load_flt_binary(). > > > > OK to do this in a followup patch? > > If you want to remove the dead assignment in its own patch > I would do that before this patch, rather than after. Before is not really possible, as it depends on the reordering. Regards, Stefan
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index 9c999ac..991dd35 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -1365,66 +1365,70 @@ static bool elf_check_ehdr(struct elfhdr *ehdr) * to be put directly into the top of new user memory. * */ -static abi_ulong copy_elf_strings(int argc,char ** argv, void **page, - abi_ulong p) +static abi_ulong copy_elf_strings(int argc, char **argv, char *scratch, + abi_ulong p, abi_ulong stack_limit) { - char *tmp, *tmp1, *pag = NULL; - int len, offset = 0; + char *tmp; + int len, offset; + abi_ulong top = p; if (!p) { return 0; /* bullet-proofing */ } + + offset = ((p - 1) % TARGET_PAGE_SIZE) + 1; + while (argc-- > 0) { tmp = argv[argc]; if (!tmp) { fprintf(stderr, "VFS: argc is wrong"); exit(-1); } - tmp1 = tmp; - while (*tmp++); - len = tmp - tmp1; - if (p < len) { /* this shouldn't happen - 128kB */ + len = strlen(tmp) + 1; + tmp += len; + + if (len > (p - stack_limit)) { return 0; } while (len) { - --p; --tmp; --len; - if (--offset < 0) { - offset = p % TARGET_PAGE_SIZE; - pag = (char *)page[p/TARGET_PAGE_SIZE]; - if (!pag) { - pag = g_try_malloc0(TARGET_PAGE_SIZE); - page[p/TARGET_PAGE_SIZE] = pag; - if (!pag) - return 0; - } - } - if (len == 0 || offset == 0) { - *(pag + offset) = *tmp; + int bytes_to_copy = (len > offset) ? offset : len; + tmp -= bytes_to_copy; + p -= bytes_to_copy; + offset -= bytes_to_copy; + len -= bytes_to_copy; + + if (bytes_to_copy == 1) { + *(scratch + offset) = *tmp; + } else { + memcpy_fromfs(scratch + offset, tmp, bytes_to_copy); } - else { - int bytes_to_copy = (len > offset) ? offset : len; - tmp -= bytes_to_copy; - p -= bytes_to_copy; - offset -= bytes_to_copy; - len -= bytes_to_copy; - memcpy_fromfs(pag + offset, tmp, bytes_to_copy + 1); + + if (offset == 0) { + memcpy_to_target(p, scratch, top - p); + top = p; + offset = TARGET_PAGE_SIZE; } } } + if (offset) { + memcpy_to_target(p, scratch + offset, top - p); + } + return p; } -static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, +static abi_ulong setup_arg_pages(struct linux_binprm *bprm, struct image_info *info) { - abi_ulong stack_base, size, error, guard; - int i; + abi_ulong size, error, guard; + + /* Linux kernel limits argument/environment space to 1/4th of stack size, + but also has a floor of 32 pages. Mimic this behaviour. */ + #define MAX_ARG_PAGES 32 - /* Create enough stack to hold everything. If we don't use - it for args, we'll use it for something else. */ size = guest_stack_size; - if (size < MAX_ARG_PAGES*TARGET_PAGE_SIZE) { - size = MAX_ARG_PAGES*TARGET_PAGE_SIZE; + if (size < MAX_ARG_PAGES * TARGET_PAGE_SIZE) { + size = MAX_ARG_PAGES * TARGET_PAGE_SIZE; } guard = TARGET_PAGE_SIZE; if (guard < qemu_real_host_page_size) { @@ -1442,19 +1446,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm, target_mprotect(error, guard, PROT_NONE); info->stack_limit = error + guard; - stack_base = info->stack_limit + size - MAX_ARG_PAGES*TARGET_PAGE_SIZE; - p += stack_base; - - for (i = 0 ; i < MAX_ARG_PAGES ; i++) { - if (bprm->page[i]) { - info->rss++; - /* FIXME - check return value of memcpy_to_target() for failure */ - memcpy_to_target(stack_base, bprm->page[i], TARGET_PAGE_SIZE); - g_free(bprm->page[i]); - } - stack_base += TARGET_PAGE_SIZE; - } - return p; + + return info->stack_limit + size - sizeof(void *); } /* Map and zero the bss. We need to explicitly zero any fractional pages @@ -2196,6 +2189,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info) struct image_info interp_info; struct elfhdr elf_ex; char *elf_interpreter = NULL; + char *scratch; info->start_mmap = (abi_ulong)ELF_START_MMAP; info->mmap = 0; @@ -2209,18 +2203,24 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info) when we load the interpreter. */ elf_ex = *(struct elfhdr *)bprm->buf; - bprm->p = copy_elf_strings(1, &bprm->filename, bprm->page, bprm->p); - bprm->p = copy_elf_strings(bprm->envc,bprm->envp,bprm->page,bprm->p); - bprm->p = copy_elf_strings(bprm->argc,bprm->argv,bprm->page,bprm->p); + /* Do this so that we can load the interpreter, if need be. We will + change some of these later */ + bprm->p = setup_arg_pages(bprm, info); + + scratch = g_new0(char, TARGET_PAGE_SIZE); + bprm->p = copy_elf_strings(1, &bprm->filename, scratch, + bprm->p, info->stack_limit); + bprm->p = copy_elf_strings(bprm->envc, bprm->envp, scratch, + bprm->p, info->stack_limit); + bprm->p = copy_elf_strings(bprm->argc, bprm->argv, scratch, + bprm->p, info->stack_limit); + g_free(scratch); + if (!bprm->p) { fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG)); exit(-1); } - /* Do this so that we can load the interpreter, if need be. We will - change some of these later */ - bprm->p = setup_arg_pages(bprm->p, bprm, info); - if (elf_interpreter) { load_elf_interp(elf_interpreter, &interp_info, bprm->buf); diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c index 506e837..09df934 100644 --- a/linux-user/linuxload.c +++ b/linux-user/linuxload.c @@ -137,8 +137,7 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp, int retval; int i; - bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int); - memset(bprm->page, 0, sizeof(bprm->page)); + bprm->p = 0; bprm->fd = fdexec; bprm->filename = (char *)filename; bprm->argc = count(argv); @@ -172,9 +171,5 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp, return retval; } - /* Something went wrong, return the inode and free the argument pages*/ - for (i=0 ; i<MAX_ARG_PAGES ; i++) { - g_free(bprm->page[i]); - } return(retval); } diff --git a/linux-user/qemu.h b/linux-user/qemu.h index 8012cc2..04c315b 100644 --- a/linux-user/qemu.h +++ b/linux-user/qemu.h @@ -145,12 +145,6 @@ extern const char *qemu_uname_release; extern unsigned long mmap_min_addr; /* ??? See if we can avoid exposing so much of the loader internals. */ -/* - * MAX_ARG_PAGES defines the number of pages allocated for arguments - * and envelope for the new program. 32 should suffice, this gives - * a maximum env+arg of 128kB w/4KB pages! - */ -#define MAX_ARG_PAGES 33 /* Read a good amount of data initially, to hopefully get all the program headers loaded. */ @@ -162,7 +156,6 @@ extern unsigned long mmap_min_addr; */ struct linux_binprm { char buf[BPRM_BUF_SIZE] __attribute__((aligned)); - void *page[MAX_ARG_PAGES]; abi_ulong p; int fd; int e_uid, e_gid; diff --git a/linux-user/syscall.c b/linux-user/syscall.c index f62c698..7d4e60e 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -5799,12 +5799,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, } *q = NULL; - /* This case will not be caught by the host's execve() if its - page size is bigger than the target's. */ - if (total_size > MAX_ARG_PAGES * TARGET_PAGE_SIZE) { - ret = -TARGET_E2BIG; - goto execve_end; - } if (!(p = lock_user_string(arg1))) goto execve_efault; ret = get_errno(execve(p, argp, envp));
Instead of creating a temporary copy for the whole environment and the arguments, directly copy everything to the target stack. For this to work, we have to change the order of stack creation and copying the arguments. v2: fixed scratch pointer type, fixed checkpatch issues. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- linux-user/elfload.c | 110 ++++++++++++++++++++++++------------------------- linux-user/linuxload.c | 7 +--- linux-user/qemu.h | 7 ---- linux-user/syscall.c | 6 --- 4 files changed, 56 insertions(+), 74 deletions(-)