[Qemu-devel] [PATCH 1/2] linux-user: remove useless variable

[Qemu-devel] [PATCH 1/2] linux-user: remove useless variable

[Laurent Vivier]

filename is only used to open the file if AT_EXECFD is not provided.
But exec_path already contains the path of the file to open.
Remove filename as it is only used in main.c whereas exec_path is
also used in syscall.c.

Fixes: d088d664f201 ("linux-user: identify running binary in /proc/self/exe")
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/main.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index a59ae9439de1..ef8e8cb10eba 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -48,7 +48,6 @@
 char *exec_path;
 
 int singlestep;
-static const char *filename;
 static const char *argv0;
 static int gdbstub_port;
 static envlist_t *envlist;
@@ -580,7 +579,6 @@ static int parse_args(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
-    filename = argv[optind];
     exec_path = argv[optind];
 
     return optind;
@@ -651,9 +649,9 @@ int main(int argc, char **argv, char **envp)
 
     execfd = qemu_getauxval(AT_EXECFD);
     if (execfd == 0) {
-        execfd = open(filename, O_RDONLY);
+        execfd = open(exec_path, O_RDONLY);
         if (execfd < 0) {
-            printf("Error while loading %s: %s\n", filename, strerror(errno));
+            printf("Error while loading %s: %s\n", exec_path, strerror(errno));
             _exit(EXIT_FAILURE);
         }
     }
@@ -778,10 +776,10 @@ int main(int argc, char **argv, char **envp)
     cpu->opaque = ts;
     task_settid(ts);
 
-    ret = loader_exec(execfd, filename, target_argv, target_environ, regs,
+    ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
         info, &bprm);
     if (ret != 0) {
-        printf("Error while loading %s: %s\n", filename, strerror(-ret));
+        printf("Error while loading %s: %s\n", exec_path, strerror(-ret));
         _exit(EXIT_FAILURE);
     }
 
-- 
2.21.0

[Qemu-devel] [PATCH 2/2] linux-user: manage binfmt-misc preserve-arg[0] flag

[Laurent Vivier]

Add --preserve-arg0 in qemu-binfmt-conf.sh to configure the preserve-arg0
flag.

Now, if QEMU is started with -0 or QEMU_ARGV0 and an empty parameter
argv[0] (the full pathname provided by binfmt-misc) is removed and
replaced by argv[1] (the original argv[0] provided by binfmt-misc when
'P'/preserve-arg[0] is set)

For instance:

  $ sudo QEMU_ARGV0= chroot m68k-chroot sh -c 'echo $0'
  sh

without this patch:

  $ sudo chroot m68k-chroot sh -c 'echo $0'
  /usr/bin/sh

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/main.c           | 18 ++++++++++++++-
 scripts/qemu-binfmt-conf.sh | 44 +++++++++++++++++++++++--------------
 2 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index ef8e8cb10eba..eeaba4a7b914 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -647,6 +647,9 @@ int main(int argc, char **argv, char **envp)
 
     init_qemu_uname_release();
 
+    /*
+     * Manage binfmt-misc open-binary flag
+     */
     execfd = qemu_getauxval(AT_EXECFD);
     if (execfd == 0) {
         execfd = open(exec_path, O_RDONLY);
@@ -656,6 +659,19 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    /*
+     * Manage binfmt-misc preserve-arg[0] flag
+     *    argv[optind]     full path to the binary
+     *    argv[optind + 1] original argv[0]
+     */
+    if (optind + 1 < argc && argv0 != NULL && argv0[0] == 0) {
+        /*
+         * argv0 with an empty string will set argv[optind + 1]
+         * as target_argv[0]
+         */
+        optind++;
+    }
+
     if (cpu_model == NULL) {
         cpu_model = cpu_get_model(get_elf_eflags(execfd));
     }
@@ -760,7 +776,7 @@ int main(int argc, char **argv, char **envp)
      * argv[0] pointer with the given one.
      */
     i = 0;
-    if (argv0 != NULL) {
+    if (argv0 != NULL && argv0[0] != 0) {
         target_argv[i++] = strdup(argv0);
     }
     for (; i < target_argc; i++) {
diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index b5a16742a149..7c9a4609c232 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -170,25 +170,27 @@ usage() {
 Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
                            [--help][--credential yes|no][--exportdir PATH]
                            [--persistent yes|no][--qemu-suffix SUFFIX]
+                           [--preserve-arg0 yes|no]
 
        Configure binfmt_misc to use qemu interpreter
 
-       --help:        display this usage
-       --qemu-path:   set path to qemu interpreter ($QEMU_PATH)
-       --qemu-suffix: add a suffix to the default interpreter name
-       --debian:      don't write into /proc,
-                      instead generate update-binfmts templates
-       --systemd:     don't write into /proc,
-                      instead generate file for systemd-binfmt.service
-                      for the given CPU. If CPU is "ALL", generate a
-                      file for all known cpus
-       --exportdir:   define where to write configuration files
-                      (default: $SYSTEMDDIR or $DEBIANDIR)
-       --credential:  if yes, credential and security tokens are
-                      calculated according to the binary to interpret
-       --persistent:  if yes, the interpreter is loaded when binfmt is
-                      configured and remains in memory. All future uses
-                      are cloned from the open file.
+       --help:          display this usage
+       --qemu-path:     set path to qemu interpreter ($QEMU_PATH)
+       --qemu-suffix:   add a suffix to the default interpreter name
+       --debian:        don't write into /proc,
+                        instead generate update-binfmts templates
+       --systemd:       don't write into /proc,
+                        instead generate file for systemd-binfmt.service
+                        for the given CPU. If CPU is "ALL", generate a
+                        file for all known cpus
+       --exportdir:     define where to write configuration files
+                        (default: $SYSTEMDDIR or $DEBIANDIR)
+       --credential:    if yes, credential and security tokens are
+                        calculated according to the binary to interpret
+       --persistent:    if yes, the interpreter is loaded when binfmt is
+                        configured and remains in memory. All future uses
+                        are cloned from the open file.
+       --preserve-arg0  preserve arg[0]
 
     To import templates with update-binfmts, use :
 
@@ -261,6 +263,9 @@ qemu_generate_register() {
     if [ "$PERSISTENT" = "yes" ] ; then
         flags="${flags}F"
     fi
+    if [ "$PRESERVE_ARG0" = "yes" ] ; then
+        flags="${flags}P"
+    fi
 
     echo ":qemu-$cpu:M::$magic:$mask:$qemu:$flags"
 }
@@ -322,9 +327,10 @@ DEBIANDIR="/usr/share/binfmts"
 QEMU_PATH=/usr/local/bin
 CREDENTIAL=no
 PERSISTENT=no
+PRESERVE_ARG0=no
 QEMU_SUFFIX=""
 
-options=$(getopt -o ds:Q:S:e:hc:p: -l debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential:,persistent: -- "$@")
+options=$(getopt -o ds:Q:S:e:hc:p:0: -l debian,systemd:,qemu-path:,qemu-suffix:,exportdir:,help,credential:,persistent:,preserve-arg0: -- "$@")
 eval set -- "$options"
 
 while true ; do
@@ -380,6 +386,10 @@ while true ; do
         shift
         PERSISTENT="$1"
         ;;
+    -0|--preserve-arg0)
+        shift
+        PRESERVE_ARG0="$1"
+        ;;
     *)
         break
         ;;
-- 
2.21.0

Re: [Qemu-devel] [PATCH 2/2] linux-user: manage binfmt-misc preserve-arg[0] flag

[John Paul Adrian Glaubitz]

Hi!

> On Jul 14, 2019, at 3:40 PM, Laurent Vivier <laurent@vivier.eu> wrote:
> 
> Add --preserve-arg0 in qemu-binfmt-conf.sh to configure the preserve-arg0
> flag.
> 
> Now, if QEMU is started with -0 or QEMU_ARGV0 and an empty parameter
> argv[0] (the full pathname provided by binfmt-misc) is removed and
> replaced by argv[1] (the original argv[0] provided by binfmt-misc when
> 'P'/preserve-arg[0] is set)
> 
> For instance:
> 
>  $ sudo QEMU_ARGV0= chroot m68k-chroot sh -c 'echo $0'
>  sh
> 
> without this patch:
> 
>  $ sudo chroot m68k-chroot sh -c 'echo $0'
>  /usr/bin/sh

As a regular user of qemu-user (we’re using qemu-user to run Debian’s buildds for m68k and sh4), I would like to add that the idea of having to pass additional environment variables to make qemu behave as expected, i.e. as the real hardware, is sub-optimal.

I would prefer that enabling the preserve flag with the qemu-binfmt.sh script would make qemu-user behave correctly.

If I understand correctly, the current design with the environment variable was chosen because my preferred approach would break compatibility in certain cases. However, I think that correct emulation is more important than compatibility to an old broken behavior and I would therefore be in favor to make the correct behavior default.

This will also be necessary when using qemu-user with Debian’s sbuild to “cross”-build packages with qemu-user. This particular bug was actually discovered while building Debian packages for m68k and sh4 using qemu-user.

Thanks,
Adrian

PS: I have disabled receiving messages for this list, so please keep me CC’ed.

Re: [Qemu-devel] [PATCH 2/2] linux-user: manage binfmt-misc preserve-arg[0] flag

[Laurent Vivier]

Le 14/07/2019 à 18:19, John Paul Adrian Glaubitz a écrit :
> Hi!
> 
>> On Jul 14, 2019, at 3:40 PM, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Add --preserve-arg0 in qemu-binfmt-conf.sh to configure the preserve-arg0
>> flag.
>>
>> Now, if QEMU is started with -0 or QEMU_ARGV0 and an empty parameter
>> argv[0] (the full pathname provided by binfmt-misc) is removed and
>> replaced by argv[1] (the original argv[0] provided by binfmt-misc when
>> 'P'/preserve-arg[0] is set)
>>
>> For instance:
>>
>>  $ sudo QEMU_ARGV0= chroot m68k-chroot sh -c 'echo $0'
>>  sh
>>
>> without this patch:
>>
>>  $ sudo chroot m68k-chroot sh -c 'echo $0'
>>  /usr/bin/sh
> 
> As a regular user of qemu-user (we’re using qemu-user to run Debian’s buildds for m68k and sh4), I would like to add that the idea of having to pass additional environment variables to make qemu behave as expected, i.e. as the real hardware, is sub-optimal.
> 
> I would prefer that enabling the preserve flag with the qemu-binfmt.sh script would make qemu-user behave correctly.

QEMU is not able to detect if it has been started by binfmt_misc with
the preserve-arg[0] enabled or not, so it can't adapt the args analysis
to get the correct list.

> 
> If I understand correctly, the current design with the environment variable was chosen because my preferred approach would break compatibility in certain cases. However, I think that correct emulation is more important than compatibility to an old broken behavior and I would therefore be in favor to make the correct behavior default.
> 
> This will also be necessary when using qemu-user with Debian’s sbuild to “cross”-build packages with qemu-user. This particular bug was actually discovered while building Debian packages for m68k and sh4 using qemu-user.

The problem we have here is we don't know how qemu-user is used in the
wild. In my knowledge you are the most involved user, but you're not the
only one reporting problem via launchpad. Moreover, distros provide
qemu-user statically linked and binfmt configuration files, so we can
guess we have other users.

And I don't like to break existing things...

What I can propose:

1- modify this patch to add a configure option:

   by default qemu will need the QEMU_ARGV0 but we will be able to
define at configure time it always runs with preserve-arg[0] flag
enabled (something like "--enable-preserve-arg0")

[So debian will be able to provide qemu-user-static with this enabled by
default if you're not afraid to break debian users environment]

2- try (again) to push in the kernel the binfmt_misc namespace that
allows to have per chroot basis binfmt configuration

3- once 3 done, enable preserve-arg[0] by default

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 2/2] linux-user: manage binfmt-misc preserve-arg[0] flag

[John Paul Adrian Glaubitz]

Hi!

Sorry for the late reply!

On 7/17/19 12:07 PM, Laurent Vivier wrote:
> And I don't like to break existing things...
> 
> What I can propose:
> 
> 1- modify this patch to add a configure option:
> 
>    by default qemu will need the QEMU_ARGV0 but we will be able to
> define at configure time it always runs with preserve-arg[0] flag
> enabled (something like "--enable-preserve-arg0")
> 
> [So debian will be able to provide qemu-user-static with this enabled by
> default if you're not afraid to break debian users environment]

This sounds like a reasonable approach for the time being, I agree with
that. I could just pass that option to configure whenever I build new
static qemu binaries for the buildds and I can drop your customized
patch locally.

> 2- try (again) to push in the kernel the binfmt_misc namespace that
> allows to have per chroot basis binfmt configuration

That would be cool, too. Has there been some discussion on this already?

> 3- once 3 done, enable preserve-arg[0] by default

You mean once "2 done"?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 2/2] linux-user: manage binfmt-misc preserve-arg[0] flag

[Peter Maydell]

On Wed, 17 Jul 2019 at 11:07, Laurent Vivier <laurent@vivier.eu> wrote:
> QEMU is not able to detect if it has been started by binfmt_misc with
> the preserve-arg[0] enabled or not, so it can't adapt the args analysis
> to get the correct list.

If the kernel provided a more useful interface (for instance
telling us what it was doing via the ELF auxv so we knew how
to interpret the command line arguments) this would help.
Of course you still have to wait until that kernel change actually
gets into the hands of users...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] linux-user: remove useless variable

[Philippe Mathieu-Daudé]

On 7/14/19 3:40 PM, Laurent Vivier wrote:
> filename is only used to open the file if AT_EXECFD is not provided.
> But exec_path already contains the path of the file to open.
> Remove filename as it is only used in main.c whereas exec_path is
> also used in syscall.c.
> 
> Fixes: d088d664f201 ("linux-user: identify running binary in /proc/self/exe")
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/main.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index a59ae9439de1..ef8e8cb10eba 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -48,7 +48,6 @@
>  char *exec_path;
>  
>  int singlestep;
> -static const char *filename;
>  static const char *argv0;
>  static int gdbstub_port;
>  static envlist_t *envlist;
> @@ -580,7 +579,6 @@ static int parse_args(int argc, char **argv)
>          exit(EXIT_FAILURE);
>      }
>  
> -    filename = argv[optind];
>      exec_path = argv[optind];
>  
>      return optind;
> @@ -651,9 +649,9 @@ int main(int argc, char **argv, char **envp)
>  
>      execfd = qemu_getauxval(AT_EXECFD);
>      if (execfd == 0) {
> -        execfd = open(filename, O_RDONLY);
> +        execfd = open(exec_path, O_RDONLY);
>          if (execfd < 0) {
> -            printf("Error while loading %s: %s\n", filename, strerror(errno));
> +            printf("Error while loading %s: %s\n", exec_path, strerror(errno));
>              _exit(EXIT_FAILURE);
>          }
>      }
> @@ -778,10 +776,10 @@ int main(int argc, char **argv, char **envp)
>      cpu->opaque = ts;
>      task_settid(ts);
>  
> -    ret = loader_exec(execfd, filename, target_argv, target_environ, regs,
> +    ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
>          info, &bprm);
>      if (ret != 0) {
> -        printf("Error while loading %s: %s\n", filename, strerror(-ret));
> +        printf("Error while loading %s: %s\n", exec_path, strerror(-ret));
>          _exit(EXIT_FAILURE);
>      }
>  
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] linux-user: remove useless variable

[Laurent Vivier]

Le 14/07/2019 à 15:40, Laurent Vivier a écrit :
> filename is only used to open the file if AT_EXECFD is not provided.
> But exec_path already contains the path of the file to open.
> Remove filename as it is only used in main.c whereas exec_path is
> also used in syscall.c.
> 
> Fixes: d088d664f201 ("linux-user: identify running binary in /proc/self/exe")
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/main.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index a59ae9439de1..ef8e8cb10eba 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -48,7 +48,6 @@
>  char *exec_path;
>  
>  int singlestep;
> -static const char *filename;
>  static const char *argv0;
>  static int gdbstub_port;
>  static envlist_t *envlist;
> @@ -580,7 +579,6 @@ static int parse_args(int argc, char **argv)
>          exit(EXIT_FAILURE);
>      }
>  
> -    filename = argv[optind];
>      exec_path = argv[optind];
>  
>      return optind;
> @@ -651,9 +649,9 @@ int main(int argc, char **argv, char **envp)
>  
>      execfd = qemu_getauxval(AT_EXECFD);
>      if (execfd == 0) {
> -        execfd = open(filename, O_RDONLY);
> +        execfd = open(exec_path, O_RDONLY);
>          if (execfd < 0) {
> -            printf("Error while loading %s: %s\n", filename, strerror(errno));
> +            printf("Error while loading %s: %s\n", exec_path, strerror(errno));
>              _exit(EXIT_FAILURE);
>          }
>      }
> @@ -778,10 +776,10 @@ int main(int argc, char **argv, char **envp)
>      cpu->opaque = ts;
>      task_settid(ts);
>  
> -    ret = loader_exec(execfd, filename, target_argv, target_environ, regs,
> +    ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
>          info, &bprm);
>      if (ret != 0) {
> -        printf("Error while loading %s: %s\n", filename, strerror(-ret));
> +        printf("Error while loading %s: %s\n", exec_path, strerror(-ret));
>          _exit(EXIT_FAILURE);
>      }
>  
> 

Applied to my linux-user branch.

Thanks,
Laurent