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

Repair ELF executables in the "scripts" directory #443

Merged
merged 4 commits into from
Feb 3, 2024

Conversation

chriskuehl
Copy link
Contributor

Fixes #340

This is an early draft and request for feedback. I'm happy to add tests but wanted to request an initial review first to see if this approach is workable.

Summary

Some packages contain ELF executables in their "scripts" directory which link to shared libraries embedded in the wheel. Currently auditwheel can't correctly repair the RPATH of these executables, and repaired wheels will have broken executables when installed.

The approach here is to:

  • Move executables into a new {package}.scripts directory in platlib, similar to the {package}.scripts directory.
  • Replace the executable in the old scripts directory with a shim that execs the relocated executable when called.

This works because the relocated executables can have a consistent RPATH ($ORIGIN/../{package}.libs). This is required since installed executables in scripts don't have a consistent relative path to platlib (it varies based on how the wheel is installed).

This is roughly following the approach suggested in #340 (comment).

Demonstration

I'm testing using a wheel I built for uwsgi==2.0.22:

$ auditwheel repair --plat manylinux_2_31_x86_64 uWSGI-2.0.22-cp39-cp39-linux_x86_64.whl  
INFO:auditwheel.main_repair:Repairing uWSGI-2.0.22-cp39-cp39-linux_x86_64.whl
INFO:auditwheel.wheeltools:Previous filename tags: linux_x86_64
INFO:auditwheel.wheeltools:New filename tags: manylinux_2_31_x86_64
INFO:auditwheel.wheeltools:Previous WHEEL info tags: cp39-cp39-linux_x86_64
INFO:auditwheel.wheeltools:New WHEEL info tags: cp39-cp39-manylinux_2_31_x86_64
INFO:auditwheel.main_repair:
Fixed-up wheel written to /home/ckuehl/proj/auditwheel/wheelhouse/uWSGI-2.0.22-cp39-cp39-manylinux_2_31_x86_64.whl

Installing the wheel produces a working uwsgi binary:

$ pip install ./uWSGI-2.0.22-cp39-cp39-manylinux_2_31_x86_64.whl 
Processing ./uWSGI-2.0.22-cp39-cp39-manylinux_2_31_x86_64.whl
Installing collected packages: uWSGI
Successfully installed uWSGI-2.0.22

$ which uwsgi
/tmp/tmp.VQ8OCc05xH/venv/bin/uwsgi

$ uwsgi --version
2.0.22

# Also works without the virtualenv activated.
$ deactivate
$ /tmp/tmp.VQ8OCc05xH/venv/bin/uwsgi --version
2.0.22

The uwsgi binary on my PATH was replaced with a wrapper script:

$ cat /tmp/tmp.VQ8OCc05xH/venv/bin/uwsgi
#!/tmp/tmp.VQ8OCc05xH/venv/bin/python
import os
import sys
import sysconfig


if __name__ == "__main__":
    os.execv(
        os.path.join(sysconfig.get_path("platlib"), 'uWSGI.scripts/uwsgi'),
        sys.argv,
    )

The real executable was relocated into site-packages:

$ ls -l /tmp/tmp.VQ8OCc05xH/venv/lib/python3.9/site-packages/uWSGI.scripts/uwsgi 
-rwxr-xr-x 1 ckuehl ckuehl 1.5M Sep  3 00:09 /tmp/tmp.VQ8OCc05xH/venv/lib/python3.9/site-packages/uWSGI.scripts/uwsgi

...and it has an RPATH to the libs directory:

$ readelf -d /tmp/tmp.VQ8OCc05xH/venv/lib/python3.9/site-packages/uWSGI.scripts/uwsgi | grep RPATH
 0x000000000000000f (RPATH)              Library rpath: [$ORIGIN/../uWSGI.libs]

Comparison to current auditwheel

Running the same repair on the main branch produces a repaired wheel but the uwsgi binary doesn't work after installation:

$ uwsgi --version
uwsgi: error while loading shared libraries: libpcre-2092419b.so.3.13.3: cannot open shared object file: No such file or directory

...and the rewritten RPATH points to an invalid location:

$ readelf -d venv/bin/uwsgi | grep RPATH
 0x000000000000000f (RPATH)              Library rpath: [$ORIGIN/../../uWSGI.libs]

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (45a8c00) 92.17% compared to head (c52a868) 92.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
+ Coverage   92.17%   92.27%   +0.09%     
==========================================
  Files          20       20              
  Lines        1252     1268      +16     
  Branches      304      306       +2     
==========================================
+ Hits         1154     1170      +16     
  Misses         56       56              
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mayeut
Copy link
Member

mayeut commented Sep 10, 2023

Thanks @chriskuehl, this looks good.

Could you add 2 tests for this:

  • an executable without dependencies shall be left untouched at the original location (there's a penalty using a shim so if not required, might as well not use one)
  • an executable with dependencies shall use the shim.

@matejsp
Copy link

matejsp commented Jan 4, 2024

I have the same issue. Any chance to complete this PR?

podman was failing with this error due to (apparently) lacking support
for abbreviated sha256 references:

    docker.errors.APIError: 500 Server Error for http+docker://localhost/v1.41/containers/create: Internal Server Error ("normalizing image: normalizing name for compat API: sha256:a455ef9d0843: invalid format: no 64-byte hexadecimal value")

Using the full SHA256 hash works under podman (and presumably works
under Docker too?).
@chriskuehl
Copy link
Contributor Author

Could you add 2 tests for this:

  • an executable without dependencies shall be left untouched at the original location (there's a penalty using a shim so if not required, might as well not use one)
  • an executable with dependencies shall use the shim.

I've added tests that cover this under the existing test_build_wheel_with_binary_executable test.

For reference, the updated test fails on main like this as expected (exhibiting the issue reported in #340):

INFO     test_manylinux:test_manylinux.py:151 docker exec 2d9783f8b742: ['/usr/local/bin/testprogram', '4']                                                                                                                                                                                                                  
/usr/local/bin/testprogram: error while loading shared libraries: libgsl-b64c76bc.so.23.1.0: cannot open shared object file: No such file or directory                                                                                                                                                                       
                                                                                                                                                                                                                                                                                                                             
FAILED

Let me know if you'd like me to move these assertions to an entirely new test case or to rearrange things in some other way.


if __name__ == "__main__":
os.execv(
os.path.join(sysconfig.get_path("platlib"), {binary_path!r}),
Copy link
Member

@mayeut mayeut Feb 3, 2024

Choose a reason for hiding this comment

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

I wonder if this works for any possible deployment scenarios.
Maybe using importlib.metadata on python >= 3.8 would be a more robust way to get the real path.

Maybe this could be an improvement left for a later PR if an issue is reported.

Copy link
Member

@mayeut mayeut left a comment

Choose a reason for hiding this comment

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

Thanks @chriskuehl !

This looks good. I left a comment but I don't think it's a blocker (unless @lkollar disagrees).

@mayeut mayeut requested a review from lkollar February 3, 2024 10:35
@mayeut mayeut mentioned this pull request Feb 3, 2024
@lkollar lkollar merged commit acb6883 into pypa:main Feb 3, 2024
11 checks passed
@hassec hassec mentioned this pull request Feb 5, 2024
5 tasks
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

Successfully merging this pull request may close these issues.

Scripts are not correctly repaired
4 participants