Patchwork Fix commandline handling for ARM semihosted executables, on Linux and BSD hosts

login
register
mail settings
Submitter Schildbach, Wolfgang
Date Nov. 23, 2010, 3:26 p.m.
Message ID <F49BF8E684BC6C4188D1D63513C4CA070D2CBAB5@sparrow.dolby.net>
Download mbox | patch
Permalink /patch/72690/
State New
Headers show

Comments

Schildbach, Wolfgang - Nov. 23, 2010, 3:26 p.m.
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(-)
Peter Maydell - Nov. 24, 2010, 12:52 p.m.
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
Schildbach, Wolfgang - Nov. 24, 2010, 10:15 p.m.
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

Patch

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