Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/binfmt: Add QEMU wrapper to preserve argv[0] #143060

Merged
merged 3 commits into from
Jan 6, 2022

Conversation

zhaofengli
Copy link
Member

@zhaofengli zhaofengli commented Oct 26, 2021

Motivation for this change

This PR enables argv[0] preservation for the QEMU binfmt-misc rules generated by boot.binfmt.emulatedSystems. It allows certain packages that rely on argv[0] in their build process (e.g., coreutils for its tests) to correctly build under binfmt.

Traditionally, the emulator invoked by binfmt-misc doesn't know what the original argv[0] is when running an executable, because only the absolute path to the foreign executable is passed. Currently, the emulator will simply use the full path as argv[0]. This leads to problems with multi-call executables as well as diff-based test methodologies used in various packages. For example, coreutils relies on output comparison in many test cases:

uniq: test 145.p: stderr mismatch, comparing 145.p.3 (expected) and 145.p.E (actual)
*** 145.p.3     Mon Oct 18 07:11:43 2021
--- 145.p.E     Mon Oct 18 07:11:44 2021
***************
*** 1,7 ****
! uniq: invalid argument 'badoption' for '--group'
  Valid arguments are:
    - 'prepend'
    - 'append'
    - 'separate'
    - 'both'
! Try 'uniq --help' for more information.
--- 1,7 ----
! /build/coreutils-8.32/src/uniq: invalid argument 'badoption' for '--group'
  Valid arguments are:
    - 'prepend'
    - 'append'
    - 'separate'
    - 'both'
! Try '/build/coreutils-8.32/src/uniq --help' for more information.

binfmt-misc now has the "P" flag that, when enabled, will add an extra argument after the absolute path to the foreign binary containing the original argv[0]. However, this also means that CLI compatibility with existing emulator executables is broken. SUSE has been carrying a patch since 2011 to add -binfmt wrappers that translate the new arguments into ones that regular QEMU accepts (-0 for overriding argv[0]). There have been attempts to resolve this issue upstream, but so far there seems to be no consensus.

This PR adds a statically-linked wrapper for QEMU that translates the arguments. It's possible to rewrite the arguments in the wrapper script that we use (#118926), but that still leaves some remaining issues (e.g., extraneous output when setting LD_PRELOAD).

Tested by building coreutils for riscv64-linux on x86_64-linux under binfmt (before, after).

Things done
  • Tested on platform(s)
    • riscv64-linux on x86_64-linux
    • aarch64-linux on x86_64-linux
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 26, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Oct 27, 2021
@oxalica
Copy link
Contributor

oxalica commented Oct 27, 2021

Dup of #118926

@zhaofengli
Copy link
Member Author

Whoops, wasn't aware of that PR and GitHub wasn't returning good results for my searches. However, I still think the implementation here is more robust and only affects QEMU (not wine, for example).

@zhaofengli
Copy link
Member Author

I implemented the suggestion from @oxalica in #115406 (comment) and coreutils can now be built under binfmt. I did a --check build and the results match what I got from native riscv64. The old version is here for comparison.

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Oct 28, 2021
pkgs/applications/virtualization/qemu/binfmt-p-wrapper.c Outdated Show resolved Hide resolved
pkgs/applications/virtualization/qemu/binfmt-p-wrapper.c Outdated Show resolved Hide resolved
pkgs/applications/virtualization/qemu/binfmt-p-wrapper.c Outdated Show resolved Hide resolved
pkgs/applications/virtualization/qemu/binfmt-p-wrapper.c Outdated Show resolved Hide resolved
pkgs/applications/virtualization/qemu/binfmt-p-wrapper.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/qemu/binfmt-p-wrapper.c Outdated Show resolved Hide resolved
nixos/modules/system/boot/binfmt.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/binfmt.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/binfmt.nix Outdated Show resolved Hide resolved
@oxalica
Copy link
Contributor

oxalica commented Oct 28, 2021

Also we should update the PR title.

@zhaofengli zhaofengli force-pushed the binfmt-argv0 branch 2 times, most recently from 5c65b40 to 724d53c Compare October 28, 2021 19:28
@zhaofengli zhaofengli changed the title nixos/binfmt: Preserve argv[0] when using QEMU nixos/binfmt: Add QEMU wrapper to preserve argv[0] Oct 29, 2021
@zhaofengli
Copy link
Member Author

zhaofengli commented Oct 31, 2021

Looks like extra-sandbox-path cannot contain the path to the executable itself, otherwise there will be some interesting conflicts when you try to build nix.conf.

To reproduce:

  • Activate emulatedSystems using the previous commit
  • After activation, run nix-build --check $(nix-store -qd /etc/nix/nix.conf) on the same machine

@zhaofengli zhaofengli force-pushed the binfmt-argv0 branch 2 times, most recently from c45d694 to 5d0aa2a Compare November 3, 2021 22:09
@oxalica oxalica self-requested a review November 9, 2021 23:09
Copy link
Contributor

@oxalica oxalica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need more docs. It works pretty fine now.
I've started a build of riscv64 coreutils with binfmt from this PR. Hope it could pass tests.

@oxalica
Copy link
Contributor

oxalica commented Nov 26, 2021

riscv64-linux coreutils built and passed all tests with QEMU wrapper from this PR now.

@oxalica oxalica mentioned this pull request Nov 29, 2021
12 tasks
@zhaofengli zhaofengli mentioned this pull request Nov 29, 2021
@risicle
Copy link
Contributor

risicle commented Dec 30, 2021

True to your word this does appear to fix the LD_PRELOAD error message with binfmt - would you mind adding a test such as the following for this?

  ldPreload = makeTest {
    name = "systemd-binfmt-ld-preload";
    machine = {
      boot.binfmt.emulatedSystems = [
        "aarch64-linux"
      ];
    };
    testScript = let
      helloAarch64 = pkgs.pkgsCross.aarch64-multiplatform.hello;
      libredirectAarch64 = pkgs.pkgsCross.aarch64-multiplatform.libredirect;
    in ''
      machine.start()

      assert "error" not in machine.succeed(
          "LD_PRELOAD='${libredirectAarch64}/lib/libredirect.so' ${helloAarch64}/bin/hello 2>&1"
      ).lower()
    '';
  };

@risicle
Copy link
Contributor

risicle commented Dec 30, 2021

@ofborg test systemd-binfmt

(I want to see what happens when this runs on an aarch64 machine)

Copy link
Contributor

@risicle risicle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.

Copy link
Member

@0x4A6F 0x4A6F left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests successfully run against rebased master.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants