[packaging] Reserve RPATH pad to avoid patchelf breaking ELF layout on RHEL 8.10 (#4271)

build_tools/_therock_utils/py_packaging.py

@@ -36,6 +36,35 @@
 
 ENABLED_VLOG_LEVEL: int = 5
 
+# Marker substring baked into RPATH pad entries at link time by
+# cmake/therock_subproject_utils.cmake. ELFs TheRock ships are linked with a
+# ~1KB filler RPATH entry containing this marker so that when we later rewrite
+# RPATHs below with `patchelf --set-rpath`, the replacement always fits inside
+# the .dynstr allocation that was reserved at link time. Patchelf therefore
+# overwrites in place and does not need to prepend a new PT_LOAD segment,
+# which is the mutation that crashes execve() on RHEL 8.10 / EL 4.18 kernels.
+# See https://github.com/ROCm/TheRock/issues/4271 for the full diagnosis.
+#
+# The same constant is duplicated in
+# build_tools/packaging/python/templates/rocm/src/rocm_sdk/tests/core_test.py
+# because that file ships inside the wheel and cannot import from build-only
+# modules. Keep the two string literals in sync.
+PATCHELF_PAD_MARKER = "__therock_patchelf_pad__"
+
+
+def _format_rpath_for_log(rpath: str) -> str:
+    """Collapse the pad filler to '<pad>' for readable REWRITE_RPATH logs.
+
+    The pad entry is ~1KB of X's and dumping it unmodified into every log line
+    makes `py_packaging.log` unreadable and spooks users watching the build
+    (issue #4271). The on-disk value is untouched; this only affects logging.
+    """
+    if not rpath:
+        return rpath
+    return ":".join(
+        "<pad>" if PATCHELF_PAD_MARKER in entry else entry for entry in rpath.split(":")
+    )
+
 
 def log(*args, vlog: int = 0, **kwargs):
     if vlog > ENABLED_VLOG_LEVEL:
@@ -428,51 +457,71 @@ def _populate_file(
             # Update RPATHs on Linux.
             file_type = get_file_type(dest_path)
             if file_type == "exe" or file_type == "so":
-                self._extend_rpath(dest_path)
-                self._normalize_rpath(dest_path)
+                self._rewrite_rpath(dest_path, self._dep_rpath_entries(dest_path))
 
-    def _extend_rpath(self, file_path: Path):
+    def _dep_rpath_entries(self, file_path: Path) -> list[str]:
+        """$ORIGIN-relative RPATH entries contributed by rpath_deps packages."""
+        entries: list[str] = []
         for dep_project, rpath in self.rpath_deps:
             parent_relpath = self._platform_dir.parent.relative_to(
                 file_path.parent, walk_up=True
             )
             dep_py_package_name = dep_project.entry.get_py_package_name(
                 self.target_family
             )
-            addl_rpath = f"$ORIGIN/{parent_relpath}/{dep_py_package_name}/{rpath}"
-            log(f"  ADD_RPATH: {file_path}: {addl_rpath}")
-            patchelf_cl = [
-                "patchelf",
-                "--add-rpath",
-                addl_rpath,
-                str(file_path),
-            ]
-            subprocess.check_call(patchelf_cl)
-
-    def _normalize_rpath(self, file_path: Path):
+            entries.append(f"$ORIGIN/{parent_relpath}/{dep_py_package_name}/{rpath}")
+        return entries
+
+    def _rewrite_rpath(self, file_path: Path, extra_rpath_entries: Sequence[str] = ()):
+        """Rewrite DT_RPATH in place.
+
+        Reads the existing RPATH, strips any filler entries the build planted
+        at link time (identified by PATCHELF_PAD_MARKER), appends the requested
+        extra_rpath_entries deduplicated against what's already there, and
+        writes the result back with a single `patchelf --set-rpath`.
+
+        Using `--set-rpath` means patchelf overwrites the existing .dynstr
+        entry in place as long as the new string fits within the space that
+        was reserved at link time by the pad. That's the whole point: it
+        avoids the .dynstr-growth path that prepends a new PT_LOAD segment
+        and trips the RHEL 8.10 execve() bug (see PATCHELF_PAD_MARKER docs
+        and issue #4271). If the replacement ever doesn't fit, patchelf will
+        fall back to growing .dynstr and the CI pad-marker grep will flag it.
+        """
         existing_rpath = (
-            subprocess.check_output(
-                [
-                    "patchelf",
-                    "--print-rpath",
-                    str(file_path),
-                ]
-            )
+            subprocess.check_output(["patchelf", "--print-rpath", str(file_path)])
             .decode()
             .strip()
         )
-        if not existing_rpath:
-            return
 
-        # Possibly in the future, do manual normalization of the RPATH.
-        norm_rpath = existing_rpath
+        entries: list[str] = []
+        seen: set[str] = set()
+        if existing_rpath:
+            for entry in existing_rpath.split(":"):
+                if not entry or PATCHELF_PAD_MARKER in entry:
+                    continue
+                if entry in seen:
+                    continue
+                entries.append(entry)
+                seen.add(entry)
+        for entry in extra_rpath_entries:
+            if entry and entry not in seen:
+                entries.append(entry)
+                seen.add(entry)
+
+        new_rpath = ":".join(entries)
+        if new_rpath == existing_rpath:
+            return
 
-        log(f"  NORMALIZE_RPATH: {file_path}: {norm_rpath}")
+        log(
+            f"  REWRITE_RPATH: {file_path}: "
+            f"{_format_rpath_for_log(existing_rpath)!r} -> {new_rpath!r}"
+        )
         subprocess.check_call(
             [
                 "patchelf",
                 "--set-rpath",
-                norm_rpath,
+                new_rpath,
                 # Forces the use of RPATH vs RUNPATH, which is more appropriate
                 # for hermetic libraries like these since it does not allow
                 # LD_LIBRARY_PATH to interfere.
@@ -637,10 +686,12 @@ def _populate_devel_file(
         shutil.copy2(src_entry.path, dest_path)
 
         if not is_windows:
-            # Update RPATHs on Linux.
+            # Update RPATHs on Linux. Devel files don't pull in new
+            # dep-relative RPATHs, but we still run the rewrite so any link-
+            # time pad entries get stripped in place.
             file_type = get_file_type(dest_path)
             if file_type == "exe" or file_type == "so":
-                self._normalize_rpath(dest_path)
+                self._rewrite_rpath(dest_path)
 
 
 MAGIC_AR_MATCH = re.compile("ar archive")

build_tools/packaging/python/templates/rocm/src/rocm_sdk/tests/core_test.py

@@ -5,8 +5,10 @@
 
 import importlib
 import locale
+import mmap
 from pathlib import Path
 import platform
+import struct
 import subprocess
 import sys
 import unittest
@@ -16,6 +18,17 @@
 
 import rocm_sdk
 
+# Keep in sync with PATCHELF_PAD_MARKER in
+# build_tools/_therock_utils/py_packaging.py. See that file for why the pad
+# exists and why we want the absence of this marker in shipped binaries.
+PATCHELF_PAD_MARKER_BYTES = b"__therock_patchelf_pad__"
+ELF_MAGIC = b"\x7fELF"
+# ELF program header flag bits.
+PF_X = 0x1
+PF_W = 0x2
+PF_R = 0x4
+PT_LOAD = 1
+
 utils.assert_is_physical_package(rocm_sdk)
 
 core_mod_name = di.ALL_PACKAGES["core"].get_py_package_name()
@@ -60,6 +73,57 @@
 )
 
 
+def _file_contains_bytes(path: Path, needle: bytes) -> bool:
+    try:
+        size = path.stat().st_size
+    except OSError:
+        return False
+    if size < len(needle):
+        return False
+    with open(path, "rb") as f:
+        with mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ) as mm:
+            return mm.find(needle) != -1
+
+
+def _is_elf(path: Path) -> bool:
+    try:
+        with open(path, "rb") as f:
+            return f.read(4) == ELF_MAGIC
+    except OSError:
+        return False
+
+
+def _parse_elf64_pt_loads(path: Path):
+    """Return (e_entry, [(p_flags, p_vaddr, p_memsz), ...]) for PT_LOAD segments.
+
+    Only supports 64-bit little-endian ELFs. Returns None for anything else
+    (32-bit, big-endian, non-ELF). ROCm only ships 64-bit LE binaries.
+    """
+    with open(path, "rb") as f:
+        ehdr = f.read(64)
+        if len(ehdr) < 64 or not ehdr.startswith(ELF_MAGIC):
+            return None
+        if ehdr[4] != 2 or ehdr[5] != 1:
+            return None
+        e_entry, e_phoff = struct.unpack_from("<QQ", ehdr, 0x18)
+        e_phentsize, e_phnum = struct.unpack_from("<HH", ehdr, 0x36)
+        if e_phentsize < 56 or e_phnum == 0:
+            return None
+        f.seek(e_phoff)
+        phdrs = f.read(e_phentsize * e_phnum)
+    segments = []
+    for i in range(e_phnum):
+        base = i * e_phentsize
+        p_type = struct.unpack_from("<I", phdrs, base)[0]
+        if p_type != PT_LOAD:
+            continue
+        p_flags = struct.unpack_from("<I", phdrs, base + 0x04)[0]
+        p_vaddr = struct.unpack_from("<Q", phdrs, base + 0x10)[0]
+        p_memsz = struct.unpack_from("<Q", phdrs, base + 0x28)[0]
+        segments.append((p_flags, p_vaddr, p_memsz))
+    return e_entry, segments
+
+
 class ROCmCoreTest(unittest.TestCase):
     def testInstallationLayout(self):
         """The `rocm_sdk` and core module must be siblings on disk."""
@@ -159,3 +223,84 @@ def testPreloadLibraries(self):
                     shortname=lib_entry.shortname,
                 ):
                     rocm_sdk.preload_libraries(lib_entry.shortname)
+
+    @unittest.skipIf(is_windows, "Patchelf RPATH pad only applies on ELF")
+    def testPatchelfPadStripped(self):
+        """No shipped file should still contain the patchelf RPATH pad marker.
+
+        TheRock links binaries with a ~1KB filler RPATH entry whose job is to
+        reserve .dynstr space for patchelf to overwrite in place at packaging
+        time. Finding the marker in an installed file means py_packaging
+        didn't rewrite that binary's RPATH (likely a file-type detection
+        miss), so the ~1KB pad leaked into the shipped artifact. Harmless at
+        runtime but a packaging bug worth failing CI on.
+
+        Context: https://github.com/ROCm/TheRock/issues/4271
+        """
+        core_root = Path(core_mod.__file__).parent
+        offenders = []
+        for f in core_root.rglob("*"):
+            if not f.is_file() or f.is_symlink():
+                continue
+            if _file_contains_bytes(f, PATCHELF_PAD_MARKER_BYTES):
+                offenders.append(str(f.relative_to(core_root)))
+        self.assertFalse(
+            offenders,
+            msg=(
+                "Files still contain the patchelf RPATH pad marker "
+                f"{PATCHELF_PAD_MARKER_BYTES.decode()!r}; py_packaging did not "
+                "rewrite their RPATHs:\n  " + "\n  ".join(sorted(offenders))
+            ),
+        )
+
+    @unittest.skipIf(is_windows, "ELF layout check only applies on Linux")
+    def testExecutableElfLayoutIntact(self):
+        """Every shipped ELF executable's first PT_LOAD must be non-writable.
+
+        The RHEL 8.10 / EL 4.18 execve() crash reproduced in issue #4271 is
+        triggered by ELFs whose first PT_LOAD segment is read-write at a
+        non-canonical base address (e.g. 0x3ff000 instead of 0x400000). That
+        layout is the telltale signature of patchelf having grown .dynstr and
+        prepended a new PT_LOAD. We assert the invariant directly instead of
+        trying to enumerate every kernel that cares: the first PT_LOAD of a
+        freshly linked executable is always R or RX, never RW.
+
+        Shared objects (`.so`) are excluded because the kernel bug only
+        affects executables loaded via execve(); ld.so handles the layout
+        correctly for DT_NEEDED loads.
+        """
+        core_root = Path(core_mod.__file__).parent
+        bin_dir = core_root / "bin"
+        if not bin_dir.is_dir():
+            self.skipTest("Core package has no bin/ directory")
+
+        offenders = []
+        checked = 0
+        for f in bin_dir.rglob("*"):
+            if not f.is_file() or f.is_symlink():
+                continue
+            if not _is_elf(f):
+                continue
+            parsed = _parse_elf64_pt_loads(f)
+            if parsed is None:
+                continue
+            _, segments = parsed
+            if not segments:
+                continue
+            checked += 1
+            first_flags, first_vaddr, _ = segments[0]
+            if first_flags & PF_W:
+                offenders.append(
+                    f"{f.relative_to(core_root)}: first PT_LOAD is writable "
+                    f"(flags=0x{first_flags:x}, vaddr=0x{first_vaddr:x})"
+                )
+
+        self.assertGreater(checked, 0, msg=f"Found no ELF executables under {bin_dir}")
+        self.assertFalse(
+            offenders,
+            msg=(
+                "ELF layout looks patchelf-mutated (issue #4271); expected "
+                "first PT_LOAD to be read-only on executables:\n  "
+                + "\n  ".join(sorted(offenders))
+            ),
+        )

cmake/therock_subproject.cmake

@@ -10,6 +10,11 @@
 
 include(ExternalProject)
 
+# therock_subproject_utils defines THEROCK_INSTALL_RPATH_PAD and friends,
+# which are baked into each subproject's init file below. Include it eagerly
+# so the pad is computed before any _init_contents generation runs.
+include(therock_subproject_utils)
+
 # Global properties.
 # THEROCK_DEFAULT_CMAKE_VARS:
 # List of CMake variables that will be injected by default into the
@@ -768,7 +773,15 @@ function(therock_cmake_subproject_activate target_name)
   string(APPEND _init_contents "set(THEROCK_PRIVATE_BUILD_RPATH_DIRS)\n")
   string(APPEND _init_contents "set(THEROCK_BUILD_STAMP_FILE \"@_build_stamp_file@\")\n")
   string(APPEND _init_contents "set(THEROCK_STAGE_STAMP_FILE \"@_stage_stamp_file@\")\n")
-  string(APPEND _init_contents "set(THEROCK_SUBPROJECT_TARGET \"@target_name@\")\n")
+  string(APPEND _init_contents "set(THEROCK_SUBPROJECT_TARGET \"@target_name@\")\n"
+    # Propagate the patchelf pad so pre-hooks and auto-managed targets in the
+    # child CMake invocation see the same reserved RPATH entry that the
+    # super-project uses. See therock_subproject_utils.cmake for the full
+    # rationale (issue #4271).
+    "set(THEROCK_INSTALL_RPATH_PAD_SIZE \"${THEROCK_INSTALL_RPATH_PAD_SIZE}\" CACHE STRING \"\" FORCE)\n"
+    "set(THEROCK_INSTALL_RPATH_PAD_MARKER \"${THEROCK_INSTALL_RPATH_PAD_MARKER}\" CACHE STRING \"\" FORCE)\n"
+    "set(THEROCK_INSTALL_RPATH_PAD \"${THEROCK_INSTALL_RPATH_PAD}\" CACHE INTERNAL \"\")\n"
+    "set(THEROCK_INSTALL_RPATH_PAD_COLON \"${THEROCK_INSTALL_RPATH_PAD_COLON}\" CACHE INTERNAL \"\")\n")
 
   # Support generator expressions in install CODE
   # We rely on this for debug symbol separation and some of our very old projects