Repository navigation
fix(tools): keep SELF PT_PHDR tables in the load image - #11407
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the exact head and the ELF header/segment handling. The change updates e_phoff to the relocated PT_PHDR, resolves a zero virtual address through its containing load, and rejects duplicate, late, undersized or non-file-backed PT_PHDR metadata before publishing output. The new fixtures cover zero/nonzero addresses, in-load table offsets, explicit/inferred SELF records, malformed metadata with existing and absent outputs, and preservation of surrounding payload bytes. The author reports 82 unwrapper/audit methods passing on Windows, all three new methods passing, readelf -h -l -W accepting all eight valid outputs, and the Linux/Windows/macOS x86_64 relinker plus macOS ARM64 and ASan jobs now passing. The product build jobs are skipped by path policy. No blocker found; approving this head.
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed 9aeb251 merged onto main 570089a (2026-10-26); the merge is clean. Windows 11, Python 3.13.9.
Results
test_unwrap_self.py: 59 tests OK, 4 skipped. Withtest_audit_dump.pythat is the 82 methods from the description.- Counter-check:
main'sunwrap_self.pyunder these tests gives 21 failures and no errors, as the description says: 12 subtests oftest_invalid_phdr_metadata_preserves_input_and_output, 8 oftest_phdr_names_the_rewritten_table_in_the_load_image, andtest_phdr_record_outside_its_load_image_is_refused. - 3,000 random containers without PT_PHDR come out byte-identical to
main's output. 3,000 random valid PT_PHDR containers (table at load + 0x40) get e_phoff at the mapped PT_PHDR offset with the table there, and every other byte as onmain. The overlapping-load inputs from my #10998 review give the same output asmain, alone and together with #10923. - With #10923 (2a45d72) the merge is clean and the combined test file passes, 65 tests and 4 skipped.
- Every new refusal fires before anything is written.
Should fix: see the inline comment. The table is written where PT_PHDR points without checking that it points at the table.
Nits
-
Two of the new checks can be removed without failing a test:
- the duplicate check (line 260), because the duplicate fixture's 112-byte PT_PHDR is already refused by the coverage check for the 168-byte table;
- the
p_fileszhalf of the coverage check (line 264), because the filesz fixtures changep_memsztoo.
A duplicate PT_PHDR of 3 × 56 bytes and a fixture that changes only
p_fileszwould pin both. -
No test accepts a PT_PHDR larger than the table, or one after a non-load header. One positive fixture would cover both, for example PT_NOTE, then a PT_PHDR of table size + 0x20, then PT_LOAD.
-
The module docstring says nothing about PT_PHDR yet. It doesn't mention that e_phoff now follows its mapped offset, or that a vaddr-0 PT_PHDR is placed through its load, which the rule for segments without a vaddr (lines 15-20) no longer describes.
Notes, nothing to change here
- A PT_PHDR after a PT_LOAD is now refused, where
mainconverted such a container with an intact table. That order breaks the gABI, so refusing it is defensible, but it is a change againstmainthe description could name. - A PT_PHDR at vaddr 0 that no load covers is refused with the generic "segment 0 vaddr 0x0 is in no PT_LOAD". The outcome is right, but unlike the other new refusals the message doesn't name PT_PHDR.
Not checked: real PS5 SELF files with a PT_PHDR (I don't know whether retail or homebrew SELFs carry one, which decides how likely the inline case is), GNU readelf, Linux and macOS.
Apart from the inline point, I found nothing to change. For disclosure: none of my open PRs touches tools/unwrap_self.py or its tests; I reviewed #10923 and #10998, which change the same file.
Written with AI assistance (Claude Code). I ran the tests, the counter-check and the probes above on my Windows 11 machine.
| if header["new"] + header["size"] > len(result): | ||
| raise Refused("segment %d escaped the output" % header["index"]) | ||
|
|
||
| if phdr is not None: |
There was a problem hiding this comment.
Should fix: this points e_phoff at phdr["new"] and writes the rewritten table there, but nothing checks that the PT_PHDR maps the program header table. Its source comes only from p_vaddr - host.vaddr (lines 199-214), or from its SELF record position (lines 189-197), and line 269 only requires a host load. If p_offset and p_vaddr disagree with the load, or e_phoff != p_offset, the tool exits 0 and overwrites len(headers) * 56 bytes of load data. Three hand-built containers:
- PT_PHDR offset 0x40, vaddr 0; PT_LOAD offset 0x4000, vaddr 0: load bytes change at +0x0.
- PT_PHDR offset 0x40, vaddr 0x400040; PT_LOAD offset 0x4000, vaddr 0x400000: load bytes change at +0x40.
- e_phoff 0x40, outside the loads; PT_PHDR offset and vaddr 0x1040 inside a load at offset 0, vaddr 0: load bytes change at +0x1040.
main leaves those bytes intact, and its stale PT_PHDR entry does no harm because the relinker drops input PT_PHDR (core/relinker/elfpatcher/src/general/SegmentFilter.cpp:19). So this is a regression, limited to PT_PHDR metadata that breaks the gABI, and it contradicts "Bytes outside the table and the input container are preserved". Refusing when the mapped bytes are not the table closes it, right after line 270:
if phdr is not None:
table = inner + struct.unpack_from("<Q", elf_header, 0x20)[0]
if data[phdr["source"]:phdr["source"] + len(headers) * 56] != data[table:table + len(headers) * 56]:
raise Refused("PT_PHDR does not map the program header table")With it, your three PT_PHDR tests pass, the three containers above are refused, and the 3,000 random valid PT_PHDR containers and 3,000 without PT_PHDR give the same output as before with no refusal. A test with a PT_PHDR whose load bytes are not the table would pin it.
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 9aeb251ba360119ccfe10c11cb15dadbf186e009. Approve; no actionable finding in this focused review.
PT_PHDR is assigned to its file-backed load region even at virtual address zero, and the output ELF header points at the relocated program-header table. Multiple, undersized, misordered and unbacked PT_PHDR layouts are refused before output publication.
Validation: Windows/Python 3.11. Candidate: test_unwrap_self: 59 run, 4 skip reports; no failures. Comparison base: test_unwrap_self: 56 run, 4 skip reports. Old production fails 21 assertions in the new synthetic layout and preservation cases.
Scope: Four existing platform cases skip; no commercial SELF or title execution. Current head and effective hosted checks are verified again before submission.
|
@veosett Thanks, the PT_PHDR handling looks right for well-formed inputs. One change: please take roboterspinne's inline suggestion at unwrap_self.py:331. Refuse when the bytes that PT_PHDR maps are not the input's program header table (compare data[phdr source : +len(headers)*56] with the table at inner+e_phoff), and add a test with a PT_PHDR whose mapped load bytes are not the table. I reproduced the overwrite on this head: a PT_PHDR at offset 0x40, vaddr 0x400040, inside a load at offset 0x4000, vaddr 0x400000 (e_phoff 0x40) exits 0, moves e_phoff to 0x1040 and changes 103 load bytes, where main changes none. |
9aeb251 to
ee78599
Compare
What
unwrap_self.pyrelocated load data andPT_PHDRbut kepte_phoffat its old file offset, leaving a rewritten table outside the load image and a stale table at the mapped address.Write the table at the mapped
PT_PHDRoffset and point the ELF header there, including when its virtual address is zero. Before publishing output, refuse duplicate or latePT_PHDRentries, insufficient file/memory coverage, records outside their file-backed load, and mapped bytes that differ from the input program-header table. This last check prevents malformed metadata from directing the rewrite into unrelated load payload. Preserve the input, existing output on refusal, and load bytes outside the rewritten table.The ELF gABI defines
PT_PHDRas the program-header table in both the file and memory image, occurring at most once before load entries.Tested
Base:
750cf6cf487f948017295b1f3717af6f886f0f85; head:ee785992dde2f8522328ab3b05d845f61f6d3518.Native Windows, CPython 3.12.14, GNU readelf 2.46.0.20260210; synthetic SELF/ELF fixtures.
1f21b45361fa8cae0a69f2ce549c26058b20c172, those 12 cases fail their refusal assertions with zero setup errors; they pass with the guard. Separate CLI probes measure 109-110 altered load bytes in each of six layouts before the guard; afterward every layout is refused and both input and existing output remain unchanged.readelf -h -l -Wwith no diagnostics, and its reported table and PHDR offsets agree.python tools/check_conventions.py --base origin/main, source test-registration audit and whitespace checks pass.unwrap_self/audit_dumptargets pass in 17.15 / 0.30 seconds on Linux and 19.80 / 0.65 seconds on Windows.linux,windows,register-contextandwrite-watchjobs were skipped by workflow policy. No local WSL, product build, GPU or title execution.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)