| Message ID | 20190714134028.315-2-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] linux-user: remove useless variable | expand |
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.
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
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
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
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 ;;
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(-)