* [Qemu-devel] [PATCH] Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts
@ 2010-11-23 15:26 Schildbach, Wolfgang
2010-11-24 12:52 ` Peter Maydell
0 siblings, 1 reply; 4+ messages in thread
From: Schildbach, Wolfgang @ 2010-11-23 15:26 UTC (permalink / raw)
To: qemu-devel
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(-)
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;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts
2010-11-23 15:26 [Qemu-devel] [PATCH] Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts Schildbach, Wolfgang
@ 2010-11-24 12:52 ` Peter Maydell
2010-11-24 22:15 ` Schildbach, Wolfgang
0 siblings, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2010-11-24 12:52 UTC (permalink / raw)
To: Schildbach, Wolfgang; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [Qemu-devel] [PATCH] Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts
2010-11-24 12:52 ` Peter Maydell
@ 2010-11-24 22:15 ` Schildbach, Wolfgang
0 siblings, 0 replies; 4+ messages in thread
From: Schildbach, Wolfgang @ 2010-11-24 22:15 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH] Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts
@ 2010-11-24 22:19 Schildbach, Wolfgang
0 siblings, 0 replies; 4+ messages in thread
From: Schildbach, Wolfgang @ 2010-11-24 22:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell
This reuses the commandline copy setup in user space by
loader_build_argptr()
and fixes https://bugs.launchpad.net/qemu/+bug/673613 .
Signed-off-by: Wolfgang Schildbach <wschi@dolby.com>
---
arm-semi.c | 79
+++++++++++++++++++++++++++++------------------
bsd-user/bsdload.c | 2 -
bsd-user/qemu.h | 1 -
linux-user/linuxload.c | 2 -
linux-user/qemu.h | 1 -
5 files changed, 49 insertions(+), 36 deletions(-)
diff --git a/arm-semi.c b/arm-semi.c
index 0687b03..533bb34 100644
--- a/arm-semi.c
+++ b/arm-semi.c
@@ -373,45 +373,64 @@ 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 (!arm_cmdline_len || host_cmdline_len > arm_cmdline_len)
{
+ return -1; /* not enough space to store command line */
+ }
- s = cmdline_buffer;
- while (*arg && len > 2) {
- int n = strlen(*arg);
+ if (!host_cmdline_len) {
+ /* We special-case the "empty command line" case
(arcg==0).
+ Just provide the terminating 0. */
+ arm_cmdline_buffer = lock_user(VERIFY_WRITE, ARG(0), 1,
0);
+ arm_cmdline_buffer[0] = 0;
+ unlock_user(arm_cmdline_buffer, ARG(0), 1);
- if (s != cmdline_buffer) {
- *(s++) = ' ';
- len--;
- }
- if (n >= len)
- n = len - 1;
- memcpy(s, *arg, n);
- s += n;
- len -= n;
- arg++;
+ /* Adjust the commandline length argument. */
+ SET_ARG(1, 0);
+ return 0;
}
- /* Null terminate the string. */
- *s = 0;
- len = s - cmdline_buffer;
- /* Unlock the buffer on the ARM side. */
- unlock_user(cmdline_buffer, ARG(0), len);
+ /* 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);
- /* Adjust the commandline length argument. */
- SET_ARG(1, len);
+ 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);
- /* Return success if commandline fit into buffer. */
- return *arg ? -1 : 0;
+ /* separate arguments by white spaces */
+ for (i = 0; i < host_cmdline_len-1; i++) {
+ if (arm_cmdline_buffer[i] == 0) {
+ arm_cmdline_buffer[i] = ' ';
+ }
+ }
+
+ /* Adjust the commandline length argument. */
+ SET_ARG(1, host_cmdline_len-1);
+ }
+
+ /* 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;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-11-24 22:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-23 15:26 [Qemu-devel] [PATCH] Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts Schildbach, Wolfgang
2010-11-24 12:52 ` Peter Maydell
2010-11-24 22:15 ` Schildbach, Wolfgang
-- strict thread matches above, loose matches on Subject: below --
2010-11-24 22:19 Schildbach, Wolfgang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).