Skip to content
Snippets Groups Projects

Fix MIPS segfaults relating to DT_MIPS_RLD_MAP_REL and RPath removal

Merged James Cowgill requested to merge jcowgill/cmake:mips-rld-map-rel into master

This is my attempt at fixing the issue I described in this Debian bug report: https://bugs.debian.org/820334

There is a comment in the code which describes why we need to do this on MIPS:

           // Background: debuggers need to know the "linker map" which contains
           // the addresses each dynamic object is loaded at. Most arches use
           // the DT_DEBUG tag which the dynamic linker writes to (directly) and
           // contain the location of the linker map, however on MIPS the
           // .dynamic section is always read-only so this is not possible. MIPS
           // objects instead contain a DT_MIPS_RLD_MAP tag which contains the
           // address where the dyanmic linker will write to (an indirect
           // version of DT_DEBUG). Since this doesn't work when using PIE, a
           // relative equivalent was created - DT_MIPS_RLD_MAP_REL. Since this
           // version contains a relative offset, moving it changes the
           // calculated address. This may cause the dyanmic linker to write
           // into memory it should not be changing.
           //
           // To fix this, we adjust the value of DT_MIPS_RLD_MAP_REL here. If
           // we move it up by n bytes, we add n bytes to the value of this tag.

The other 4 commits do some refactoring around the cmELF API to make it possible to implement this. I decided to add a DynamicEntryList type to allow cmELF callers to read the entire .dynamic section from the ELF, manipulate it, and then re-encode it. This seemed like the best abstraction to me since it keeps all the endian/bitness issues inside cmELF.

Instead of adding DT_MIPS_RLD_MAP_REL to the ByteSwap(ELF_Dyn& dyn) method, I simply removed the switch from it. In both ELF32 and ELF64, sizeof(dyn.d_un.d_val) == sizeof(dyn.d_un.d_ptr) so there is no need to distinguish between them when fixing the endianness.

Merge request reports

Pipeline #29731 passed

Pipeline passed for 15762b72 on jcowgill:mips-rld-map-rel

Approval is optional

Merged by avatar (Feb 22, 2025 1:11pm UTC)

Merge details

  • Changes merged into master with 15762b72.
  • Did not delete the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Basic content checks passed!

    Branch-at: 475eefa9
    Acked-by: @kwrobot

  • Hrm. CMake doesn't have a commit static_assert (that I can see right now). It'd be nice to static_assert the sizeof(dyn.d_un.d_val) == sizeof(dyn.d_un.d_ptr) so that if it ever does change, we'll know ahead of time.

    Also, it looks like this is a brand new tag, so it should be backwards compatible?

  • Author Contributor

    I can add a static_assert or use the old negative sized array hack. I wasn't sure if CMake requires a C++11 compiler yet or not.

    It is a new tag so I could have added DT_MIPS_RLD_MAP_REL to the switch case, but I don't think we need to distinguish between tags at all.

  • @jcowgill please use the negative sized array hack. We still support C++98 builds in general (the only use of C++11 is in the new optional cmake server mode feature).

    Otherwise, LGTM. The commit sequence is well organized. Please squash the static assertion (hack) back into its proper commit and force-push.

  • James Cowgill Added 5 commits:

    Added 5 commits:

    • 66c4d082 - elf: remove tag switch from ELF_Dyn ByteSwap function
    • 72eb6a37 - elf: add DynamicEntryList methods and rpath tag constants
    • df16bb48 - cmSystemTools: rewrite RemoveRPath using DyanmicEntryList methods
    • 1d156cd3 - cmSystemTools, elf: handle DT_MIPS_RLD_REL_MAP in RemoveRPath
    • 52fa6aab - elf: Remove GetDynamicEntryCount and ReadBytes methods
  • Author Contributor

    Should be fixed now.

  • Basic content checks passed!

    Changes since last check: compare

    Branch-at: 52fa6aab809c470d2bbc248da1367c1fdba53454
    Acked-by: @kwrobot

  • Reassigned to @brad.king

  • Brad King Milestone changed to %3.8.0

    Milestone changed to %3.8.0

  • Thanks! LGTM. Merged to next for testing. Once clean there I'll merge this to master.

  • Kitware Robot Added 3 commits:

    Added 3 commits:

    • b8b1d151 - cmSystemTools: rewrite RemoveRPath using DyanmicEntryList methods
    • cd4f573a - cmSystemTools, elf: handle DT_MIPS_RLD_REL_MAP in RemoveRPath
    • 15762b72 - elf: Remove GetDynamicEntryCount and ReadBytes methods
  • Basic content checks passed!

    Changes since last check: compare

    Branch-at: 15762b72
    Acked-by: @kwrobot

  • I updated the branch here to fix some conversion warnings reported from testing in next. Now it is ready to merge to master.

  • Kitware Robot Status changed to merged

    Status changed to merged

Please register or sign in to reply
Loading