pclnpost: physically compact the deduplicated funcinfo carrier - #2267
pclnpost: physically compact the deduplicated funcinfo carrier#2267zhouguangyuan0718 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Review: pclnpost physical carrier compaction
This is a well-engineered change: the transactional stage → sync → (sign) → verify → atomic rename → dir-fsync flow in replaceBinary, the fail-closed shape validation, and the post-edit re-parse with elf.NewFile/macho.NewFile are all solid, and the added tests assert real physical shrink and that a failed verify leaves the original intact. Doc comments for Rewrite, writeBack, and the package were correctly updated away from the old "performs no writes" language.
The findings below are hardening gaps against malformed/crafted binary layouts and one ELF-alignment correctness concern. raw here is the freshly linked output, so these are robustness rather than remote-exploit issues, but the Mach-O path already guards several of them and the ELF path does not — worth closing the asymmetry.
Key items (inline):
patchMachOFileOffsetssecond load-command walk lacks the bounds/cmdsz-minimum checks the parser applies (compact.go:247).- ELF
oldEndis never bounds-checked againstlen(raw)beforecopy(out[lastUsed:], out[oldEnd:]), unlike the Mach-O path (compact.go:385/447). - ELF cut point
lastUsedis only 8-byte aligned; shifting later non-PT_LOAD segments by an unalignedremovedcan breakp_offset ≡ p_vaddr (mod p_align)— Mach-O page-aligns but ELF does not (compact.go:404). - Design doc /
funcInfoSiteLayoutArgscomment say carriers link "after.data" but the script insertsINSERT AFTER .got.plt.
Additional lower-priority notes (not inline):
parseELFLayoutcomputesshoff+shnum*shentsz/strOff+strSizefrom 64-bit header fields; a near-2^64shoff/strOffcan wrap and pass the> len(raw)guard, then panic on indexing (compact.go:466/469/475). Prefer overflow-safe comparison (shoff > len(raw)first, then compare the remainder).machoPageSizeinfers page size from ARM64 cputype only; other targets fall back to 4K. Fine for Apple Silicon today, but a comment noting the assumption would help.writeBackrecomputesphysicalRemovedfromos.Statand clamps negatives, discarding the authoritativeremovedreturned bycompactCarrier(write.go:255). SinceBytesRemovedis diagnostic-only, returningremoveddirectly (or asserting equality) is more robust.- Memory/CPU (link-time, single-shot, so Low): the full binary is duplicated ~3x (
info.raw,rawat write.go:160,outviaappendincompactCarrier) and fully parsed ~3x (compactCarrierre-parse +load(staged)in verify). Acceptable as defense-in-depth; one of the redundant full-buffer copies could be dropped if link-time footprint ever matters.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
875db43 to
d3f8e86
Compare
d3f8e86 to
80f64c0
Compare
ac39ffc to
e767561
Compare
Why this is still needed
Current main still rewrites the compact prebuilt functab into a buffer sized to the original
llgo_funcinfo_entrysection and only zeroes its unused tail. LTO inline copies are logically deduplicated, but the original section and file space remain allocated.Main has removed the old stub metadata model, so this revision deliberately keeps only one disposable entry carrier. If the compact table does not fit that carrier, rewriting fails closed and the existing runtime fallback remains authoritative.
Summary
.bssand the Mach-O carrier in an isolated__LLGOsegmentValidation
go test ./internal/pclnpost ./internal/buildgo test -race ./internal/pclnpostinternal/pclnpostcoverage: 86.1%texttemplate: 2,576,608 -> 2,366,320 bytes;__LLGO.filesize: 245,760 -> 32,768 bytescodesign --verify --strict, and caller/logging acceptance tests passprintln: 62,688 -> 61,616 bytes; final writablePT_LOAD.p_filesz:0x9a0->0x570No Go-version compatibility workaround is included in this PR.