Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the complete diff and its overflow boundary cases. PT_LOAD ends, the extra block end, alignment of the synthetic header block, and the final header-block end are checked before the layout is written. The regression fixtures cover each overflow separately plus valid BSS and a representable high address. This head is based directly on current main. On Linux/GCC 12.2, the relinker-only target built, linux_load_alignment passed (1/1), and Conventions plus diff whitespace checks passed. GitHub reports no upstream checks for this head, so this approval records the local evidence only.
Dextroid17
left a comment
There was a problem hiding this comment.
Verified on Linux, on this PR's own parent rather than current main: the PR is CONFLICTING, so its parent (f1451396) is the honest base and the only one its diff is guaranteed to apply to. Head 2da4361a, cherry-picked clean.
Arm A (PR head) — linux_load_alignment Passed (0.02 s). I also ran the six cases through my own probe (their fixture() primitives, my orchestration, not their harness):
valid-bss rc=0 output=yes all 4 PT_LOAD ranges fit uint64
valid-high-address rc=0 output=yes all 4 PT_LOAD ranges fit uint64
load-range-wrap rc=2 output=NO FAIL: PT_LOAD virtual address range overflows
extra-range-wrap rc=2 output=NO FAIL: Extra block virtual address range overflows
header-alignment-wrap rc=2 output=NO FAIL: Header block virtual address alignment overflows
header-range-wrap rc=2 output=NO FAIL: Header block virtual address range overflows
Four distinct named errors, each with no executable written, and both controls still passing with the invariant holding on every load.
Arm B (only ProgramHeaderLayoutBuilder.cpp reverted to the parent, all tests kept) — the test fails, and the probe shows why, quantitatively. The baseline returns 0 and writes an ELF for all four defect cases, with these loadable ranges:
load-range-wrap vaddr 0x2000 memsz 0xffffffffffffffff -> 0x10000000000001fff wraps
extra-range-wrap vaddr 0xfffffffffffffff5 memsz 0xd8 -> 0x100000000000000cd wraps
header-range-wrap vaddr 0xffffffffffffc000 memsz 0x4150 -> 0x10000000000000150 wraps
The third line is your fixture, and the numbers match the description byte for byte (PT_LOAD at 0xffffffffffffc000 with size 0x4150) — good sign that the claim was measured rather than argued.
One nuance worth stating precisely. For header-alignment-wrap the baseline also returns 0 and writes an executable, but its output shows no wrapping PT_LOAD range — every load satisfies p_vaddr + p_memsz <= 2^64-1. So that guard is stricter than the artefact-visible harm: it rejects an input the current code happens to survive, whereas the other three are demonstrable in the produced file. That is not a defect — being stricter than the visible damage is fine — but a reader comparing the four cases will notice, so it may be worth a sentence in the body distinguishing "wraps the written range" from "would overflow the alignment computation".
Sequencing, for whoever merges: this is the third open PR touching ProgramHeaderLayoutBuilder.cpp (#2274 counts synthetic headers, #1559 orders the table, this one guards the arithmetic). Each is correct alone; only one can land without the others rebasing, and this one is already CONFLICTING.
Not verified: a Windows host, and whether any shipping title reaches these inputs — the fixture is synthetic by construction, and I did not check the overflow cases against a real module.
Written with AI assistance.
Merge upstream main f8c2f07 into the existing contribution branch. Preserve the four reviewed overflow checks and EH-frame behavior. Tested exact tree 8dec423: Linux relinker-only Release, single build job, 43/43 CTest; six-case overflow and EH-frame fixtures passed. AI-assisted; the submitting author remains responsible.
|
Closing for the same reason as #1942: a |
|
Dextroid17: the distinction is correct. Three baseline fixtures wrap written PT_LOAD ranges; header-alignment-wrap overflows the alignment computation, but its written load ranges still fit uint64. The conflict was resolved at ba02db6; that does not supply affected-title evidence. The fixtures are synthetic, and I have no real title demonstrating a conversion failure. I accept keeping this PR closed. No further CI approval is requested for it. AI-assisted review and reply. |
What
A PT_LOAD with a large zero-filled memory range can wrap the end address used to place Linux's synthetic load block. The relinker then returns success and writes an ELF with an unrepresentable or overlapping load range. Header alignment and the final header-block extent can overflow independently.
Reject an overflowing PT_LOAD or synthetic block range, use the existing checked AlignUp64 helper for header alignment, and require the computed header-block range to fit uint64_t. Extend linux_load_alignment with ordinary BSS and representable high-address controls plus separate load-end, synthetic-block, header-alignment and final-header-range failures. Keep all existing assertions.
Tested
The independent reviewer's final-header-range fixture is byte-identical to the regression input: baseline returns success and emits a PT_LOAD at 0xffffffffffffc000 with size 0x4150; the patched binary exits 2 with “Header block virtual address range overflows” and creates no executable. All four overflow cases were checked individually; ordinary BSS and representable high-address controls pass before and after. Their declared BSS is not allocated or executed. Zorin OS 18.1 x86-64, GCC 13.3.0, Python 3.12 and CMake 3.28.3, relinker-only Release/static, one build job. Author validation of the composed four patches passed all 39 configured CTest registrations in 6.06 s, including progress_report and import_audit; exact-base baseline passed 38/38 in 5.93 s. A separate independent review rebuilt the composed revision and passed 39/39 in 6.03 s. Focused baseline and patched checks use small synthetic inputs. Existing assertions are retained; no real game, firmware, private asset or GPU input was used.
This checks representable address arithmetic, not canonical-address or general ELF layout validity. The existing four-versus-five synthetic-header count gap remains outside this fix. Native Windows/macOS runtime, real titles, sanitizers, cross-PR builds and cold shared-CI timing remain unverified.
Checklist
Open #1559 and #1586 also edit ProgramHeaderLayoutBuilder.cpp for ordering and EH-frame retention. They do not implement these arithmetic checks. No cross-PR merge or runtime validation is claimed. A fresh main/open-work check found no equivalent implementation.