Fix bug in walking .gnu.version_r linked list #86
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When writing the code to teach --replace-needed to modify the .gnu.version_r section (NixOSgh-85), I misunderstood how the ->vn_next pointers in the Elf_Verneed structs are supposed to be interpreted: I thought they gave an offset from the beginning of the section, but in fact they give an offset relative to the current struct. The resulting bug was very odd: generally, patchelf would complete without signalling an error, but it would only successfully replace filenames that occurred as either the first or second entries in the .gnu.version_r section, while the third or later entries would be left untouched. This commit fixes the interpretation of the ->vn_next pointers, so that now --replace-needed should work correctly even on ELF files with more than two version needed structs. Thanks to @matthew-brett for finding the bug / providing a test case, and to @rmcgibbo for helping me diagnose it.
|
@njsmith Awesome work! You're work on improving the correctness of |
|
@cstrahan: thanks! And yes, it's proven very useful; it's an integral part of the auditwheel tool that we use to make wheels self contained, and we probably couldn't practically make Linux wheels work without it. In fact I like it so much that I wrote my own version for PE files :-) (https://github.com/njsmith/redll) |
|
Ping. We've been shipping this in the manylinux docker image for a few weeks now and it seems to be working well. |
|
@edolstra Can we merge this? |
|
Thanks, merged! |
|
Really glad Python community is slowly moving to Nix :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When writing the code to teach --replace-needed to modify the
.gnu.version_r section (gh-85), I misunderstood how the ->vn_next
pointers in the Elf_Verneed structs are supposed to be interpreted: I
thought they gave an offset from the beginning of the section, but in
fact they give an offset relative to the current struct. The resulting
bug was very odd: generally, patchelf would complete without signalling
an error, but it would only successfully replace filenames that occurred
as either the first or second entries in the .gnu.version_r section,
while the third or later entries would be left untouched.
This commit fixes the interpretation of the ->vn_next pointers, so that
now --replace-needed should work correctly even on ELF files with more
than two version needed structs.
Thanks to @matthew-brett for finding the bug / providing a test case,
and to @rmcgibbo for helping me diagnose it.