Repository navigation
fix(relinker): give Linux guest modules a default non-executable stack - #1506
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
This is a well-documented and thoroughly tested fix for a real glibc 2.41+ compatibility issue. The approach of adding a default non-executable PT_GNU_STACK header when absent is the standard solution.
Strengths:
- Clear before/after comparison table showing the exact problem and fix
- Extensive testing across multiple platforms (macOS, Debian container, native x86-64 Linux)
- Proper handling of edge cases (explicit RWX headers preserved and still rejected by loader)
- Comprehensive test suite that actually loads converted libraries via dlopen
Minor suggestions:
- Consider adding a comment explaining why flag value 6 is used (PF_R | PF_W = read-write, non-executable) for future maintainers
- The magic number 0x6474e551 could potentially be replaced with an ELF constant if available in the codebase's headers
Overall this looks ready to merge once any rebase conflicts are resolved. Good work on the thorough testing and documentation.
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Good improvement for Linux compatibility and security. Adding a GNU_STACK program header to converted guest modules ensures proper stack permissions (non-executable by default) which is important for modern Linux systems.
Strengths:
- Clean implementation that preserves existing GNU_STACK headers if present, or adds one with non-executable flags (6 = PF_R | PF_W) if missing
- Comprehensive test coverage including native dlopen execution on Linux x86-64 to verify stack permissions at runtime
- Good documentation of the security implications in the PR description
Technical notes:
- The approach of checking for existing GNU_STACK headers before adding a new one is correct
- Proper use of PF_R | PF_W flags (non-executable) as the default
- Test properly verifies both header presence and actual runtime stack permissions via /proc/self/maps
This will improve compatibility with modern Linux systems that expect or require the GNU_STACK header. Ready to merge.
|
@klosixgaming-arch I will update both PRs at 22 CEST time. thanks for the comments and help! |
ddefc46 to
c78a8e2
Compare
|
@adriadam10 Could you review this when you have time? I’ve rebased it onto current main, addressed the review suggestions, and marked it ready for review. Validation passes on Linux, Windows and macOS; the upstream checks are still queued. Thanks! |
0a7ff62 to
d5241d2
Compare
|
Checked the diff against the description: |
…-header fix(relinker): give Linux guest modules a default non-executable stack
What
Converted Linux guest libraries without
PT_GNU_STACKcan fail to load on glibc 2.41+: its absence is interpreted as an executable-stack requirement, anddlopenrefuses it when the process stack is non-executable (glibc change).Emit a default RW, non-executable stack header only when absent. Preserve explicit input headers, including executable flags and stack-size hints. The regression converts headerless, RW and RWX fixtures, verifies the output headers, and on Linux x86-64 loads the first two outputs, calls their export and verifies that the process stack remains non-executable. The original headerless input reproduces
cannot enable executable stack as shared object requires; the fixed output loads and returns 42. The #1156 discussion encountered the same loader failure.Rebuilt directly on
maind70b8998: one commit, four files. No changes from #1265, #1913, #1986 or #1999 remain. Main's merged #1586 unwind-header constant is preserved. The stack change is independently mergeable.Tested
Exact source head
d5241d28: independent validation passes on native GitHub Linux and Windows runners, Release / GCC, including full default builds, all-library packaging and three repetitions of the relevant runtime tests under a 30-second limit. All three related tests execute on Linux and Windows; Linux x86-64 additionally performs native loading and checks process-stack permissions. Windows checks ELF conversion and headers.These are fork validation jobs, not collaborator-dispatched upstream checks. Unrelated tests from the full suite were not run in this validation.
Local macOS 27.0.1 arm64, Apple Clang, Release relinker-only build: all 39 registered tests pass, longest 1.56 s. Native Linux loading is skipped on macOS and checked by Linux validation. Conventions and
git diff --checkpass.Checklist
mainsnapshotd70b8998; no other open PR implements this change