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

avoid string table duplicates and DT_NEEDED reference corruption when using --replace-needed multiple times #237

Closed
wants to merge 2 commits into from

Conversation

DerDakon
Copy link
Contributor

When it happens that the .gnu.version_r stores the strings in .dynstr it can come to corruption of the library names written into DT_NEEDED (#158):

  • the library names in DT_NEEDED are replaced, new entries are written to the end of .dynstr
  • the version library names are replaced, and written to the end of the string section.

If the section for the version strings is also ".dynstr", the previous modifications were not taken into account and things were written from the old end of .dynstr again. The order in which these strings were written is not the same as the previous replacement, so things would end up with the same size, but different offsets. The .gnu.version_r table is correct, the file contents are fine, but the offsets in the DT_NEEDED entries are wrong. Since they are printed as 0-terminated strings the first one replaced will always be shown correct, which also is the case if the argument is only used once as the string is replaced with itself afterwards.

Now things are correct, but the strings are added to .dynstr twice. Use an unordered map to cache them.

src/patchelf.cc Outdated Show resolved Hide resolved
pranavkapoor001 pushed a commit to ArrowOS-Devices/android_device_asus_X00T that referenced this pull request Jan 16, 2021
* Turns out patchelf (from version 0.9 - 0.10) has trouble working with
  '--replace-needed' option (see NixOS/patchelf#237),
  as it corrupted the goodix lib we just patched from extract-utils.
* Though I'm still not sure as to why it's seemingly working with few binaries
  and straight out corrupting others.
* Since the host patchelf "seems" to be working just fine, revert back
  to using it.

Signed-off-by: Saurabh Charde <saurabhchardereal@gmail.com>
Swamoy14 pushed a commit to Swamoy14/android_device_asus_X00T_4.4 that referenced this pull request Jan 18, 2021
* Turns out patchelf (from version 0.9 - 0.10) has trouble working with
  '--replace-needed' option (see NixOS/patchelf#237),
  as it corrupted the goodix lib we just patched from extract-utils.
* Though I'm still not sure as to why it's seemingly working with few binaries
  and straight out corrupting others.
* Since the host patchelf "seems" to be working just fine, revert back
  to using it.

Signed-off-by: Saurabh Charde <saurabhchardereal@gmail.com>
When it happens that the .gnu.version_r stores the strings in .dynstr it can
come to corruption of the library names written into DT_NEEDED:

-the library names in DT_NEEDED are replaced, new entries are written to the end
 of .dynstr
-the version library names are replaced, and written to the end of the string
 section.

If the section for the version strings is also ".dynstr", the previous
modifications were _not_ taken into account and things were written from the old
end of .dynstr again. The order in which these strings were written is not the
same as the previous replacement, so things would end up with the same size, but
different offsets. The .gnu.version_r table is correct, the file contents are
fine, but the offsets in the DT_NEEDED entries are wrong. Since they are printed
as 0-terminated strings the first one replaced will always be shown correct,
which also is the case if the argument is only used once as the string is
replaced with itself afterwards.
This can happen especially if .gnu.version_r stores the strings in .dynstr, so
replacing the library names would add them twice to the same section. Keep a map
of what was already added and where, and simply reuse the old entries if they
are needed again.
@Mic92
Copy link
Member

Mic92 commented Nov 27, 2021

Thanks! I rebased and merged this.

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.

None yet

2 participants