On 15 August 2015 at 19:26, Stefan Brüns <stefan.bru...@rwth-aachen.de> wrote:
> qemu currently limits the space for the evironment and arguments to
> 32 * PAGE_SIZE. Linux limits the argument space to 1/4 of the stack size.
> A program trying to detect this with a getrlimit(RLIMIT_STACK) syscall
> will typically get a much larger limit than qemus current 128kB.
>
> The current limit causes "Argument list too long" errors.
>
> Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
Thanks for this bug fix; it definitely seems like a good idea.
I have a few review comments below.
> ---
> linux-user/elfload.c | 29 ++++++++++++++++++-----------
> linux-user/linuxload.c | 7 ++++---
> linux-user/qemu.h | 11 ++---------
> linux-user/syscall.c | 4 ++++
> 4 files changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 1788368..be8f4d6 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1365,11 +1365,13 @@ 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,
This should have a space after the 'argc,'.
(If you run scripts/checkpatch.pl you'll find it catches this
and other minor style errors.)
> + struct linux_binprm *bprm)
> {
> char *tmp, *tmp1, *pag = NULL;
> int len, offset = 0;
> + void **page = bprm->page;
> + abi_ulong p = bprm->p;
>
> if (!p) {
> return 0; /* bullet-proofing */
> @@ -1383,8 +1385,13 @@ static abi_ulong copy_elf_strings(int argc,char **
> argv, void **page,
> tmp1 = tmp;
> while (*tmp++);
> len = tmp - tmp1;
> - if (p < len) { /* this shouldn't happen - 128kB */
> - return 0;
> + if (p < len) {
Since this looks almost but not quite like a standard reallocate-larger,
a comment here would be helpful I think:
/* Reallocate the page array to add extra zero entries at the start */
> + bprm->page = (void**)calloc(bprm->n_arg_pages + 32,
> sizeof(void*));
Prefer
bprm->page = g_new0(void *, bprm->n_arg_pages + 32);
> + memcpy(&bprm->page[32], page, sizeof(void*) * bprm->n_arg_pages);
> + free(page);
g_free(page);
> + page = bprm->page;
> + bprm->n_arg_pages += 32;
> + p += 32 * TARGET_PAGE_SIZE;
I think we have enough repetitions of '32' here to merit a #define.
But having said all that, I wonder if it would be better to
precalculate how big a page array we need and just do the
allocation once, rather than having this complicated code to
handle a reallocate-and-fix-up-everything. In particular this
is basically just adding string lengths for filename, argv
and envp together. load_flt_binary() already wants that information,
so it might be better to have loader_exec() calculate this
and fill in new bprm->argv_strlen and bprm->envp_strlen values
for the callees to use.
> }
> while (len) {
> --p; --tmp; --len;
> @@ -1423,8 +1430,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct
> linux_binprm *bprm,
> /* 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 < bprm->n_arg_pages * TARGET_PAGE_SIZE) {
> + size = bprm->n_arg_pages * TARGET_PAGE_SIZE;
> }
> guard = TARGET_PAGE_SIZE;
> if (guard < qemu_real_host_page_size) {
> @@ -1442,10 +1449,10 @@ 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;
> + stack_base = info->stack_limit + size - bprm->n_arg_pages *
> TARGET_PAGE_SIZE;
> p += stack_base;
>
> - for (i = 0 ; i < MAX_ARG_PAGES ; i++) {
> + for (i = 0; i < bprm->n_arg_pages; i++) {
> if (bprm->page[i]) {
> info->rss++;
> /* FIXME - check return value of memcpy_to_target() for failure
> */
> @@ -2211,9 +2218,9 @@ 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);
> + bprm->p = copy_elf_strings(1, &bprm->filename, bprm);
> + bprm->p = copy_elf_strings(bprm->envc, bprm->envp, bprm);
> + bprm->p = copy_elf_strings(bprm->argc, bprm->argv, bprm);
> if (!bprm->p) {
> fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG));
> exit(-1);
> diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
> index 506e837..59943ea 100644
> --- a/linux-user/linuxload.c
> +++ b/linux-user/linuxload.c
> @@ -137,8 +137,9 @@ 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->n_arg_pages = 32;
> + bprm->p = bprm->n_arg_pages * TARGET_PAGE_SIZE - sizeof(unsigned int);
> + bprm->page = (void**)calloc(bprm->n_arg_pages, sizeof(void*));
Rather than calloc, prefer
bprm->page = g_new0(void *, bprm->n_arg_pages);
> bprm->fd = fdexec;
> bprm->filename = (char *)filename;
> bprm->argc = count(argv);
> @@ -173,7 +174,7 @@ int loader_exec(int fdexec, const char *filename, char
> **argv, char **envp,
> }
>
> /* Something went wrong, return the inode and free the argument pages*/
> - for (i=0 ; i<MAX_ARG_PAGES ; i++) {
> + for (i = 0; i < bprm->n_arg_pages; i++) {
> g_free(bprm->page[i]);
> }
You should g_free() bprm->page too here now, since we now allocate it.
> return(retval);
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 8012cc2..cf4aa73 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -144,14 +144,6 @@ void stop_all_tasks(void);
> 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. */
> #define BPRM_BUF_SIZE 1024
> @@ -162,7 +154,8 @@ extern unsigned long mmap_min_addr;
> */
> struct linux_binprm {
> char buf[BPRM_BUF_SIZE] __attribute__((aligned));
> - void *page[MAX_ARG_PAGES];
> + void **page;
> + int n_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..6fa5fb4 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5799,12 +5799,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
> arg1,
> }
> *q = NULL;
>
> +/* This check is bogus. MAX_ARG_PAGES is a thing of the past. Trust the host
> +execve to set errno appropriately */
> +#if 0
> /* 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;
> }
> +#endif
You should just delete this code now it's not required; we don't
need to leave #if-0'd out obsolete code in the tree.
> if (!(p = lock_user_string(arg1)))
> goto execve_efault;
> ret = get_errno(execve(p, argp, envp));
> --
> 2.1.4
thanks
-- PMM
