| Message ID | F49BF8E684BC6C4188D1D63513C4CA070D2CBAB5@sparrow.dolby.net |
|---|---|
| State | New |
| Headers | show |
On 23 November 2010 15:26, Schildbach, Wolfgang <WSCHI@dolby.com> wrote: > When running an ARM semihosted executable on a linux machine, the > command line is not delivered to the guest (see > https://bugs.launchpad.net/qemu/+bug/673613). I've tested this, and it does work; however I don't think the code you have to handle the "empty command line" case is right. > + if (!host_cmdline_len) { > + /* is arcg==0 even a possibility? */ > + arm_cmdline_buffer[0] = 0; > + host_cmdline_len=1; > + } To answer the question in the comment: You can certainly start a native binary with argc==0 (ie an argv with only the terminating NULL) using execv(). The qemu loader_exec() function similarly separates the binary to be started from the argv array in its API. As it happens the command line parsing in linux-user/main.c doesn't provide the user with a way to give an empty array to loader_exec(), but I think it's more robust to just handle the special case here rather than impose an undocumented restriction on use of loader_exec(). (After all ARM semihosting is definitely a minority sport and this isn't exactly a performance critical bit of code :-)) In any case, I think that your handling for this case is being done too late. By this point you have already done the "is the buffer big enough" check and locked the user buffers with a host_cmdline_len of zero even though you're now writing a byte here. My suggestion would be just to handle this case early, by putting something like this before the 'command line too long' test: if (host_cmdline_len == 0) { /* Simpler to treat the "empty command line" case specially */ if (arm_cmdline_len < 1) { return -1; } arm_cmdline_buffer = lock_user(VERIFY_WRITE, ARG(0), 1, 0); arm_cmdline_buffer[0] = 0; SET_ARG(1, 0); unlock_user(arm_cmdline_buffer, ARG(0), 1); return 0; } -- PMM
OK, I declare defeat :-) I'll followup with a patch that special-cases an empty command line. - Wolfgang -----Original Message----- From: Peter Maydell [mailto:peter.maydell@linaro.org] Sent: Wednesday, November 24, 2010 1:53 PM To: Schildbach, Wolfgang Cc: qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH] Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts On 23 November 2010 15:26, Schildbach, Wolfgang <WSCHI@dolby.com> wrote: > When running an ARM semihosted executable on a linux machine, the > command line is not delivered to the guest (see > https://bugs.launchpad.net/qemu/+bug/673613). I've tested this, and it does work; however I don't think the code you have to handle the "empty command line" case is right. > + if (!host_cmdline_len) { > + /* is arcg==0 even a possibility? */ > + arm_cmdline_buffer[0] = 0; > + host_cmdline_len=1; > + } To answer the question in the comment: You can certainly start a native binary with argc==0 (ie an argv with only the terminating NULL) using execv(). The qemu loader_exec() function similarly separates the binary to be started from the argv array in its API. As it happens the command line parsing in linux-user/main.c doesn't provide the user with a way to give an empty array to loader_exec(), but I think it's more robust to just handle the special case here rather than impose an undocumented restriction on use of loader_exec(). (After all ARM semihosting is definitely a minority sport and this isn't exactly a performance critical bit of code :-)) In any case, I think that your handling for this case is being done too late. By this point you have already done the "is the buffer big enough" check and locked the user buffers with a host_cmdline_len of zero even though you're now writing a byte here. My suggestion would be just to handle this case early, by putting something like this before the 'command line too long' test: if (host_cmdline_len == 0) { /* Simpler to treat the "empty command line" case specially */ if (arm_cmdline_len < 1) { return -1; } arm_cmdline_buffer = lock_user(VERIFY_WRITE, ARG(0), 1, 0); arm_cmdline_buffer[0] = 0; SET_ARG(1, 0); unlock_user(arm_cmdline_buffer, ARG(0), 1); return 0; } -- PMM
diff --git a/arm-semi.c b/arm-semi.c index 0687b03..65dc398 100644 --- a/arm-semi.c +++ b/arm-semi.c @@ -373,45 +373,58 @@ uint32_t do_arm_semihosting(CPUState *env) #ifdef CONFIG_USER_ONLY /* Build a commandline from the original argv. */ { - char **arg = ts->info->host_argv; - int len = ARG(1); - /* lock the buffer on the ARM side */ - char *cmdline_buffer = (char*)lock_user(VERIFY_WRITE, ARG(0), len, 0); + char *arm_cmdline_buffer; + const char *host_cmdline_buffer; - if (!cmdline_buffer) - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; + unsigned int i; + unsigned int arm_cmdline_len = ARG(1); + unsigned int host_cmdline_len = + ts->info->arg_end-ts->info->arg_start; + + if (host_cmdline_len > arm_cmdline_len) { + return -1; /* command line too long */ + } + + /* lock the buffers on the ARM side */ + arm_cmdline_buffer = + lock_user(VERIFY_WRITE, ARG(0), host_cmdline_len, 0); + host_cmdline_buffer = + lock_user(VERIFY_READ, ts->info->arg_start, + host_cmdline_len, 1); - s = cmdline_buffer; - while (*arg && len > 2) { - int n = strlen(*arg); + if (arm_cmdline_buffer && host_cmdline_buffer) + { + /* the last argument is zero-terminated; + no need for additional termination */ + memcpy(arm_cmdline_buffer, host_cmdline_buffer, + host_cmdline_len); - if (s != cmdline_buffer) { - *(s++) = ' '; - len--; + /* separate arguments by white spaces */ + for (i = 0; i < host_cmdline_len-1; i++) { + if (arm_cmdline_buffer[i] == 0) { + arm_cmdline_buffer[i] = ' '; + } } - if (n >= len) - n = len - 1; - memcpy(s, *arg, n); - s += n; - len -= n; - arg++; - } - /* Null terminate the string. */ - *s = 0; - len = s - cmdline_buffer; - /* Unlock the buffer on the ARM side. */ - unlock_user(cmdline_buffer, ARG(0), len); + if (!host_cmdline_len) { + /* is arcg==0 even a possibility? */ + arm_cmdline_buffer[0] = 0; + host_cmdline_len=1; + } - /* Adjust the commandline length argument. */ - SET_ARG(1, len); + /* Adjust the commandline length argument. */ + SET_ARG(1, host_cmdline_len-1); + } - /* Return success if commandline fit into buffer. */ - return *arg ? -1 : 0; + /* Unlock the buffers on the ARM side. */ + unlock_user(arm_cmdline_buffer, ARG(0), host_cmdline_len); + unlock_user((void*)host_cmdline_buffer, ts->info->arg_start, 0); + + /* Return success if we could return a commandline. */ + return (arm_cmdline_buffer && host_cmdline_buffer) ? 0 : -1; } #else - return -1; + return -1; #endif case SYS_HEAPINFO: { diff --git a/bsd-user/bsdload.c b/bsd-user/bsdload.c index 14a93bf..6d9bb6f 100644 --- a/bsd-user/bsdload.c +++ b/bsd-user/bsdload.c @@ -176,8 +176,6 @@ int loader_exec(const char * filename, char ** argv, char ** envp, retval = prepare_binprm(&bprm); - infop->host_argv = argv; - if(retval>=0) { if (bprm.buf[0] == 0x7f && bprm.buf[1] == 'E' diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h index 9763616..e343894 100644 --- a/bsd-user/qemu.h +++ b/bsd-user/qemu.h @@ -50,7 +50,6 @@ struct image_info { abi_ulong entry; abi_ulong code_offset; abi_ulong data_offset; - char **host_argv; int personality; }; diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c index 9ee27c3..ac8c486 100644 --- a/linux-user/linuxload.c +++ b/linux-user/linuxload.c @@ -174,8 +174,6 @@ int loader_exec(const char * filename, char ** argv, char ** envp, retval = prepare_binprm(bprm); - infop->host_argv = argv; - if(retval>=0) { if (bprm->buf[0] == 0x7f && bprm->buf[1] == 'E' diff --git a/linux-user/qemu.h b/linux-user/qemu.h index 708021e..8f0a81f 100644 --- a/linux-user/qemu.h +++ b/linux-user/qemu.h @@ -50,7 +50,6 @@ struct image_info { abi_ulong saved_auxv; abi_ulong arg_start; abi_ulong arg_end; - char **host_argv; int personality; };
When running an ARM semihosted executable on a linux machine, the command line is not delivered to the guest (see https://bugs.launchpad.net/qemu/+bug/673613). This patch fixes this, for Linux and BSD hosts. Thanks to Peter Maydell for suggesting this patch, and to Nathan Froyd for helping me with the list netiquette! - Wolfgang Schildbach Signed-off-by: Wolfgang Schildbach <wschi@dolby.com> --- arm-semi.c | 73 ++++++++++++++++++++++++++++------------------- bsd-user/bsdload.c | 2 - bsd-user/qemu.h | 1 - linux-user/linuxload.c | 2 - linux-user/qemu.h | 1 - 5 files changed, 43 insertions(+), 36 deletions(-)