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

Allow multiple modifications in same call #361

Merged
merged 6 commits into from
Dec 22, 2021

Conversation

fzakaria
Copy link
Contributor

@fzakaria fzakaria commented Dec 21, 2021

patchelf previously would incorrectly patch the ELF header if it was called with multiple changes at once such as add & replace.

In order to support that, rewrite the sections in between each section modification.

Please see the issue for the bug and here is now the fix:

> ./src/patchelf --replace-needed libruby-2.7.so.2.7 /lib/x86_64-linux-gnu/libruby-2.7.so.2.7 \
--replace-needed libc.so.6 /lib/x86_64-linux-gnu/libc.so.6 \
--add-needed /lib/x86_64-linux-gnu/libcrypt.so.1 \
--add-needed /lib/x86_64-linux-gnu/libdl.so.2 \
--add-needed /lib/x86_64-linux-gnu/libgmp.so.10 \
--add-needed /lib/x86_64-linux-gnu/libm.so.6 \
--add-needed /lib/x86_64-linux-gnu/libpthread.so.0 \
--add-needed /lib/x86_64-linux-gnu/librt.so.1 /tmp/ruby

> ./src/patchelf --print-needed /tmp/ruby
/lib/x86_64-linux-gnu/libcrypt.so.1
/lib/x86_64-linux-gnu/libdl.so.2
/lib/x86_64-linux-gnu/libgmp.so.10
/lib/x86_64-linux-gnu/libm.so.6
/lib/x86_64-linux-gnu/libpthread.so.0
/lib/x86_64-linux-gnu/librt.so.1
/lib/x86_64-linux-gnu/libruby-2.7.so.2.7
/lib/x86_64-linux-gnu/libc.so.6

Fix #359

Thank you @trws for the idea of the fix.

CC @Mic92

`patchelf` previously would incorrectly patch the ELF header if it was
called with multiple changes at once such as _add_ & _replace_.

In order to support that, rewrite the sections in between each section
modification.

Fix NixOS#359
Comment on lines +17 to +18
# We have to set the soname on these libraries
${PATCHELF} --set-soname libbar.so ./libbar.so
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly discussed with @trws that if the shared object doesn't have a DT_SONAME, it won't make use of the previously discovered loaded shared object.

This was discovered while writing this test. You can see below that libbar.so was not found, even though its set with an absolute path.

ldd ~/code/github.com/NixOS/patchelf/tests/scratch/replace-add-needed/simple
	linux-vdso.so.1 (0x00007ffc90565000)
	/home/fmzakari/code/github.com/NixOS/patchelf/tests/scratch/replace-add-needed/libbar.so (0x00007f532a4c4000)
	/home/fmzakari/code/github.com/NixOS/patchelf/tests/scratch/replace-add-needed/libfoo.so (0x00007f532a4bf000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f532a2da000)
	/nix/store/jsp3h3wpzc842j0rz61m5ly71ak6qgdn-glibc-2.32-54/lib/ld-linux-x86-64.so.2 => /lib64/ld-linux-x86-64.so.2 (0x00007f532a4cb000)
	libbar.so => not found

@fzakaria fzakaria force-pushed the faridzakaria/fix-add-replace-for-real branch from d66caff to e268662 Compare December 21, 2021 01:34
src/patchelf.cc Show resolved Hide resolved
src/patchelf.cc Outdated Show resolved Hide resolved
@Mic92
Copy link
Member

Mic92 commented Dec 21, 2021

The test error for musl looks legit FAIL: replace-add-needed.sh

@fzakaria
Copy link
Contributor Author

@Mic92 ah -- my test tries to grep for glibc ! I didn't know we build musl libraries lol.
I'll figure out a new thing to do for the test + make the changes you recommend.

@fzakaria fzakaria force-pushed the faridzakaria/fix-add-replace-for-real branch from 2284a70 to 3e60d5e Compare December 21, 2021 14:02
Made changes according to feedback from @Mic92 such as moving the
`rewriteSections` inline into every method.

Improved `replace-add-needed.sh` to work with musl libc
@fzakaria fzakaria force-pushed the faridzakaria/fix-add-replace-for-real branch from 3e60d5e to d02a3ec Compare December 21, 2021 14:05
@fzakaria
Copy link
Contributor Author

Okay I started alpine locally and see that the issue is that the musl linker is working differently than glibc.

/app/tests # ./replace-add-needed.sh
Error loading shared library libbar.so: No such file or directory (needed by /app/tests/scratch/replace-add-needed/libfoo.so)
	/lib/ld-musl-x86_64.so.1 (0x7f3e0f527000)
	/app/tests/scratch/replace-add-needed/libfoo.so => /app/tests/scratch/replace-add-needed/libfoo.so (0x7f3e0e51a000)
	/app/tests/scratch/replace-add-needed/libbar.so => /app/tests/scratch/replace-add-needed/libbar.so (0x7f3e0e514000)
	libc.musl-x86_64.so.1 => /lib/ld-musl-x86_64.so.1 (0x7f3e0f527000)
Error loading shared library libbar.so: No such file or directory (needed by /app/tests/scratch/replace-add-needed/libfoo.so)

/app/tests # ../src/patchelf --print-soname scratch/replace-add-needed/libbar.so
libbar.so

It's failing to skip the libbar.so NEEDED since it already linked it.

This is quite interesting; tagging @trws

# Make the NEEDED in libfoo the same as simple
# This is a current "bug" in musl
# https://www.openwall.com/lists/musl/2021/12/21/1
${PATCHELF} --replace-needed libbar.so $(readlink -f ./libbar.so) ./libfoo.so
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mic92 please read the linked mailing issue I opened for why we have to do this for musl
https://www.openwall.com/lists/musl/2021/12/21/1

This should now succeed on musl and glibc proving the fix of the bug.

@fzakaria fzakaria requested a review from Mic92 December 21, 2021 22:32
@Mic92 Mic92 merged commit 3d0a58b into NixOS:master Dec 22, 2021
@Mic92
Copy link
Member

Mic92 commented Dec 22, 2021

Thanks!

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.

Cannot add-needed and replace-needed in same invocation
2 participants