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

autoPatchelfHook skips position-independent executables #48330

Closed
tadfisher opened this issue Oct 13, 2018 · 1 comment
Closed

autoPatchelfHook skips position-independent executables #48330

tadfisher opened this issue Oct 13, 2018 · 1 comment
Assignees

Comments

@tadfisher
Copy link
Contributor

Issue description

autoPatchelfHook determines which executables it should patch via:

isExecutable() {
    readelf -h "$1" 2> /dev/null | grep -q '^ *Type: *EXEC\>'
}

However, PIEs have a type of DYN, as in:

$ readelf -h result/build-tools/28.0.3/zipalign
ELF Header:
  Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 
  Class:                             ELF64
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              DYN (Shared object file)
  Machine:                           Advanced Micro Devices X86-64
  Version:                           0x1
  Entry point address:               0x5350
  Start of program headers:          241664 (bytes into file)
  Start of section headers:          198080 (bytes into file)
  Flags:                             0x0
  Size of this header:               64 (bytes)
  Size of program headers:           56 (bytes)
  Number of program headers:         10
  Size of section headers:           64 (bytes)
  Number of section headers:         32
  Section header string table index: 27

Questions:

  • Should autoPatchelfHook also patch PIEs? An alternative is to just call autoPatchelfFile manually, but I'm having trouble finding out how to run it in the correct phase.
  • Is there even a reliable way to distinguish between shared libraries and PIEs? I know in general, shared libs don't have an .interp section, but is this a reliable enough assumption?

cc @aszlig

@aszlig
Copy link
Member

aszlig commented Oct 13, 2018

@tadfisher: Actually, executables can also be shared libraries at the same time (eg. libc.so.6 and I'm doing exactly that in my recent project as well), so the only thing that distinguishes them is an .interp section (except if statically linked of course). So I guess it's a better idea to just look for .interp in isExecutable instead of what we currently do. At least if I don't miss something, this should be the most reliable way, after all what we want to do with executables is to patch the .interp section.

If nobody else sends a PR in the meantime, I'm going to fix this next week.

As for autoPatchelfFile it's a bit more difficult, because it needs to be run after the normal patchelf hook, so it might be necessary to add postPhases.

@aszlig aszlig self-assigned this Nov 3, 2018
@aszlig aszlig closed this as completed in c64624b Nov 3, 2018
worldofpeace pushed a commit to worldofpeace/nixpkgs that referenced this issue Feb 10, 2019
I originally thought it would just be enough to just check for an INTERP
section in isExecutable, however this would mean that we don't detect
statically linked ELF files, which would break our recent improvement to
gracefully handle those.

In theory, we are only interested in ELF files that have an INTERP
section, so checking for INTERP would be enough. Unfortunately the
isExecutable function is already used outside of autoPatchelfHook, so we
can't easily get rid of it now, so let's actually strive for more
correctness and make isExecutable actually match ELF files that are
executable.

So what we're doing instead now is to check whether either the ELF type
is EXEC *or* we have an INTERP section and if one of them is true we
should have an ELF executable, even if it's statically linked.

Along the way I also set LANG=C for the invocations of readelf, just to
be sure we don't get locale-dependent output.

Tested this with the following command (which contains almost[1] all the
packages using autoPatchelfHook), checking whether we run into any
library-related errors:

  nix-build -E 'with import ./. { config.allowUnfree = true; };
    runCommand "test-executables" {
      drvs = [
        anydesk cups-kyodialog3 elasticsearch franz gurobi
        masterpdfeditor oracle-instantclient powershell reaper
        sourcetrail teamviewer unixODBCDrivers.msodbcsql17 virtlyst
        vk-messenger wavebox zoom-us
      ];
    } ("for i in $drvs; do for b in $i/bin/*; do " +
       "[ -x \"$b\" ] && timeout 10 \"$b\" || :; done; done")
  '

Apart from testing against library-related errors I also compared the
resulting store paths against the ones prior to this commit. Only
anydesk and virtlyst had the same as they didn't have self-references,
everything else differed only because of self-references, except
elasticsearch, which had the following PIE binaries:

  * modules/x-pack/x-pack-ml/platform/linux-x86_64/bin/autoconfig
  * modules/x-pack/x-pack-ml/platform/linux-x86_64/bin/autodetect
  * modules/x-pack/x-pack-ml/platform/linux-x86_64/bin/categorize
  * modules/x-pack/x-pack-ml/platform/linux-x86_64/bin/controller
  * modules/x-pack/x-pack-ml/platform/linux-x86_64/bin/normalize

These binaries were now patched, which is what this commit is all about.

[1]: I didn't include the "maxx" package (MaXX Interactive Desktop)
     because the upstream URLs are no longer existing and I couldn't
     find them elsewhere on the web.

Signed-off-by: aszlig <aszlig@nix.build>
Fixes: NixOS#48330
Cc: @gnidorah (for MaXX Interactive Desktop)
(cherry picked from commit c64624b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants