Skip to content

fix(relinker): validate executable export names and attributes like guest modules - #11381

Merged
adriadam10 merged 5 commits into
boykopovar:mainfrom
roboterspinne:fix/relinker-executable-export-attributes
Oct 28, 2026
Merged

adriadam10 merged 5 commits into
boykopovar:mainfrom
roboterspinne:fix/relinker-executable-export-attributes

Conversation

@roboterspinne

@roboterspinne roboterspinne commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

What

The executable export loop in RelinkerPipeline.cpp (#10646) checked where an export lies but not its name or attributes: on untouched 4017310, relinker --skip-sce-module for Linux, Linux --lazy-binding and --windows exited 0 for an export whose name is empty after stripping # ("", #A#B), an STT_GNU_IFUNC, STT_FILE, STT_COMMON or global STT_SECTION export, binding 3 or 10 (STB_GNU_UNIQUE), st_other 0x10, and two exports with the same stripped name and different values or bindings. Linux copies these entries into .dynsym with only the name rewritten, so ld.so calls an IFUNC export as a resolver and binds conflicting duplicates by hash-chain order; --macos already refused all of them except st_other 0x10, with its own messages (Empty macOS executable export, Unsupported macOS executable export attributes, Duplicate Mach-O export), and GuestImageReader.cpp:189-192 refuses all of them in guest modules.

Tested

Windows 11, GCC 15.2.0 (WinLibs UCRT), relinker-only Release build (-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON); synthetic fixtures only, no title or homebrew.

  • Counter-check (run on 4017310; e05ba6f, onto which the branch was rebased, has no relinker changes since): with the new test and the test_macos_executable_exports.py edit on untouched 4017310, executable_export_attributes fails with AssertionError: ('empty_name-linux', 0, ...) and macos_executable_exports with AssertionError: ('attributes', 2, 'FAIL: Unsupported macOS executable export attributes ...'). A per-case probe of the same build: all 13 refusal inputs exited 0 on Linux, lazy and Windows; on macOS empty name and #A#B failed with Empty macOS executable export, the six attribute inputs with Unsupported macOS executable export attributes, the four conflicting pairs with Duplicate Mach-O export, and st_other 0x10 exited 0. The accepted inputs behaved as with the change. With the change every refusal input fails with the new line on all four targets.
  • Before the merge of main, on 4017310: ctest -R 'export|executable|macos|guest' -j 3: 43 tests passed in 19.5 s, among them executable_export_attributes (2.1 s), executable_export_bounds, macos_executable_exports, linux_executable_exports, executable_tls_relocations, executable_segments, relinker_executables, guest_export_lookup, windows_guest_export_aliases, macos_output, macos_empty_tls, macos_tls_overlap and the guest_* tests. 55 further tests whose fixtures carry DT_OS_SYMTABSZ and so reach the export loop passed in 42.1 s (-j 3): strict_nid_filter, gnu_hash_analysis, init_array_roots, jmprel_table_bounds, needed_libraries, none_relocations, null_symbol_relocations, optional_plt, sce_code_extent, sce_module_code_extent, sce_relro, string_table_bounds, unused_filter_indirect, windows_tls_protected_entries, linux_named_imports, linux_trace_imports, weak_imports, windows_import_modules, windows_named_imports, windows_trace_imports, the two windows_kernel_variant_* and the 33 tls_function_coverage_* tests.
  • Merged current main (7b41552, which includes feat(relinker): trace function imports on macOS #9408 and fix(relinker): reject out-of-range executable TLS symbols #11058) into the branch without conflicts (b93be68; the earlier merges are 4b1a9a3 in a9d053f, 5b5d24c in 12ad27b and 204c674 in de709f9); tools/check_main_sync.py --base upstream/main reported 0 behind and a clean merge at 7b41552, and the later merges on main up to 035ac96 touch no relinker file. On b93be68, ctest -R 'executable_export|macos_executable_exports|linux_executable_exports|guest_symbol|weak_imports|macos_trace_imports|trace_imports|executable_tls_relocations' -j 6: 12 tests passed: executable_export_attributes (2.8 s), executable_export_bounds, macos_executable_exports, linux_executable_exports, guest_symbol_names, weak_imports, windows_guest_weak_imports, macos_guest_weak_imports, macos_trace_imports (added by feat(relinker): trace function imports on macOS #9408), linux_trace_imports, windows_trace_imports, executable_tls_relocations (extended by fix(relinker): reject out-of-range executable TLS symbols #11058).
  • Full suite on b93be68: ctest --test-dir build-relinker -j 6: 310 tests, 307 passed, long_paths skipped; failures known on this machine on main as well: select_tests (Windows symlink fixture) and patch_advisor_unit (path with spaces, which open test(tools): compare quoted and resolved paths in the patch advisor test #10667 fixes); both failed again alone.
  • python tools/check_conventions.py --base upstream/main with this title and description passed on b93be68.
  • Not tested on Linux or macOS; CI runs the tests on all three hosts.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes (Claude Code)

…uest modules

The executable export loop accepted names that are empty after
stripping '#', IFUNC, FILE, COMMON and SECTION types, bindings above
STB_WEAK, st_other bits above the visibility, and same-name exports
that differ, and Linux copied them into .dynsym. It now rejects them
before the location checks with the rule GuestImageReader applies to
non-local guest symbols, while identical aliases stay accepted. Every
target reports the same line at the symbol entry offset, and the macOS
attribute and empty-name checks, now unreachable, are removed.
@MarcZX10

Copy link
Copy Markdown
Contributor

Review follow-up: the new executable export validation is well specified and the hosted Linux, macOS and macOS x86_64 relinker checks pass. The Windows relinker job failed after 253.68 s with four check_compile_modes timeouts (check_compile_modes_test_include_directory_values_can_look_like_modes, ...forwarded_macro_options_are_preserved, ...forwarded_include_operands_can_look_like_modes, ...library_directory_values_can_look_like_modes); the log shows the new executable export tests completed successfully. This looks unrelated to the changed export code, but please rerun or confirm those timeouts are environmental before merge. One review-scope note: the PR diff also contains multiple PT_DYNAMIC/duplicate-tag, PT_TLS, GNU EH-frame and import symbol-type changes in RelinkerPipeline.cpp; please ensure the full current-head test report and review explicitly cover these collateral edits (the existing test matrix is broad, but the description attributes the feature primarily to export validation).

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 5021 core/relinker/CMakeLists.txt
PR 6713 core/relinker/CMakeLists.txt
PR 7173 core/relinker/CMakeLists.txt
PR 7531 core/relinker/CMakeLists.txt
PR 9793 core/relinker/CMakeLists.txt

@roboterspinne

Copy link
Copy Markdown
Contributor Author

@MarcZX10 Thanks. The Windows failure was on the earlier head de709f9 (Build run 40304520175). On the current head 12ad27b, which merges main 5b5d24c, Build run 40306429208 passed on every host, including the Windows relinker job, so the four check_compile_modes timeouts did not recur. The PT_DYNAMIC and duplicate-tag, PT_TLS, GNU EH-frame and import symbol-type changes in RelinkerPipeline.cpp are not part of this PR: they are main's own changes from #10403, #10752, #10730, #8076 and #10017, which reached the branch through the merges of main, so they show up only in a diff against the branch's original base e05ba6f. Against main (the Files tab, or git diff upstream/main...12ad27b10e) this PR changes six files, and in RelinkerPipeline.cpp only the <map> include and the seven lines of the export check (8 additions, no deletions). The focused and full-suite runs in the Tested section are on 12ad27b and so cover the code with those main changes.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying the scope: I verified the current diff against main; it is the six-file change you described, and the Windows relinker rerun passed alongside the other hosted relinker jobs and ASan. The implementation and focused tests look sound.

One current-main gate remains before merge. The branch contains the merge of main at 5b5d24cb22, while current main is cbf76e7a6e and includes the later #9408 macOS import-tracing changes in RelinkerPipeline.cpp. The preview merge is clean, but the reported local matrix and current hosted checks are on the older merged base. Please sync the latest main and rerun the focused export tests (and hosted relinker matrix) on that exact head. CONTRIBUTING.md asks branches to stay up to date with main; I’m holding approval until that integration is checked.

@roboterspinne

Copy link
Copy Markdown
Contributor Author

@MarcZX10 Merged current main 4b1a9a3 into the branch (a9d053f, no conflicts). A note on the premise: #9408 (cbf76e7) does not change RelinkerPipeline.cpp, and no commit on main since 5b5d24c does. It does touch two of this PR's six files: core/relinker/CMakeLists.txt (the new macos_trace_imports test; main also added an ASan option for input_magic there) and MacOsMachOPatcher.cpp, where #9408 adds its needed and --trace-imports lines before the export loop (lines 68-70) and the Traced binds after it (lines 104-110). Neither hunk overlaps the four lines this PR removes (lines 84-86 and 88 of main), and the export loop reads none of needed, traceImports, Binds or Traced; Traced is built from relocations.Imports, undefined symbols that the loop skips at its SHN_UNDEF check. On a9d053f the focused export tests plus macos_trace_imports, linux_trace_imports and windows_trace_imports passed (11/11), and the full suite passed 307 of 310 tests, with long_paths skipped and the same two known local failures as on main. The line references and the Tested section of the description now refer to the merged main, and the hosted relinker matrix has been triggered on a9d053f.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the refreshed head a9d053f4be0c13b1a8679e04aa07f345213435e9. The export-name, binding, visibility, type and duplicate checks match the documented guest-module restrictions; the author reports the focused export/tracing tests and current-head relinker matrix passing. The remaining request is integration validation: this head is based on 4b1a9a3b2f, while current main is 2db37cf1de46fd7b133fc5d0be1e395340f0fd2c and adds TLS-symbol bounds handling in the same RelinkerPipeline.cpp before the export loop. The hunk is separate, but the branch/report do not include it. Please sync current main and rerun the focused export tests before merge. AI-assisted review; the author-reported Windows/full-suite results are accurately distinguished from the hosted matrix.

@MarcZX10

Copy link
Copy Markdown
Contributor

Correction to my earlier base note: 2db37cf1de did not yet contain the TLS-symbol bounds change I referenced. Current main is now 2cce8bc51d64bfcff29b967e703d60f246357730; it includes commit 5f4c8cb927, which adds TLS-symbol range validation in RelinkerPipeline.cpp. I compared that patch: it changes definedTlsSymbolValue and is separate from this PR's executable-export validation, but it is in the same pipeline file and is absent from head a9d053f4be. The request to sync to current main and rerun the focused export tests remains; I apologize for misattributing the timing in my prior review.

@roboterspinne

Copy link
Copy Markdown
Contributor Author

@MarcZX10 Synced: b93be68 merges current main 7b41552, which includes #11058, into the branch without conflicts; the export check is unchanged. The export loop reads the symbol table directly and calls neither symbolEntryPos nor definedTlsSymbolValue, so the two hunks do not interact, and the merges on main after 7b41552 up to 035ac96 touch no relinker file. On b93be68 I rebuilt relinker-only and reran the focused export selection plus executable_tls_relocations: 12/12 passed; the full suite passed 307 of 310 tests, with long_paths skipped and the same two known local failures as on main. The Tested section and the line references now refer to b93be68 and main 7b41552, and the hosted relinker matrix runs on it. Please re-review on this head once it is green, so the changes-requested state is cleared.

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed b93be6845b4d against 7b415520b607. No actionable findings.

The relinker validates defined, nonlocal, visible executable exports before target conversion: stripped names must be nonempty; binding, visibility and type must be supported; same-name entries must match in every field after st_name. Identical aliases remain accepted for Linux/Windows, while macOS retains its duplicate-trie rejection. This closes malformed IFUNC and conflicting dynamic-symbol paths and matches guest-module validation.

Validation: Hosted relinker CTests passed on the pinned head: Windows 310, Ubuntu 314, and both macOS jobs 304 each; the ASan job’s 34 TLS-coverage tests passed. Full-product Windows/Linux jobs were skipped. Locally, the current-main-plus-patch relinker built with GCC 15.3 and 12 focused CTests passed. A 28-case review-only sweep covered duplicate-field differences and ordering, hidden/internal IFUNC skips, input preservation, and absent outputs on rejection. Unpatched main accepted the empty-name negative-control fixture that the candidate rejects.

The local cache lacked the pinned PR objects, so local tests used the patch on 035ac96; hosted checks ran exact head b93be6845b4d.

@adriadam10 adriadam10 self-assigned this Oct 27, 2026
@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 28, 2026 00:33

Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.

@adriadam10
adriadam10 merged commit 4603105 into boykopovar:main Oct 28, 2026
14 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the export name, attribute and duplicate rules against GuestImageReader.cpp, the removal of the now-unreachable macOS checks, and that the new test's 13 refusal inputs only fail with the change. Relinker jobs on Linux, macOS, macOS x86_64, Windows and ASan are green on this head; merged.

@roboterspinne
roboterspinne deleted the fix/relinker-executable-export-attributes branch October 28, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants