Repository navigation
fix(tools): validate ELF import table file ranges - #10352
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the parser paths and added boundary cases. The new take calls validate the entire declared file-backed range before iterating dynamic entries or accepting an empty symbol table, while preserving valid EOF-sized ranges and post-DT_NULL behavior. The tests exercise oversized/truncated ranges, zero-sized out-of-file tables, previous-report preservation, and compatibility with an early terminator. Reported Windows/Linux Python tests and Windows relinker CTest passed; current hosted eligibility, conventions, intake, labels and relinker matrix also pass. Product build jobs are skipped because this is tooling-only. I found no correctness issue in the changed paths.
|
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.
|
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 5bf97cce6b2a against author merge base e78acf4bdac0. No actionable findings in this focused review.
The three added range checks validate the complete declared file extent before any early DT_NULL can stop iteration, and cover empty dynamic symbol tables whose offsets still lie beyond EOF. The existing take bounds check allows exact EOF endpoints and uses Python integer arithmetic for extreme 64-bit values. Import callers retain the same error propagation, and malformed inputs reach the existing diagnostic path before a previous JSON report is replaced. This matches the file-backed range definitions in the ELF program-header specification and section-header specification.
On Windows x64 / CPython 3.11, python -X utf8 tools/tests/test_import_audit.py -v ran 131 methods successfully with three skips (128 passed). The unchanged merge-base suite ran 126 with three skips (123 passed). Grafting only the new tests onto baseline production produced 14 expected assertion failures, zero errors, and three skips: invalid ranges that the old parser accepted are rejected at the head, while valid EOF/terminated-padding cases remain accepted. Conventions, whitespace and test registration passed.
GitHub reports successful relinker matrix/ASan, conventions and intake checks. Full product jobs were skipped; no full native build, Linux/macOS local execution or title run is claimed. Validation covers the Python parser and CLI behavior.
What
Reject malformed ELF import metadata before reporting a successful audit. Check the complete
PT_DYNAMICandSHT_DYNAMICfile ranges before walking their entries, even when an earlyDT_NULLwould hide a truncated declared end. Also check theSHT_DYNSYMimport range when it contains no entries, matching the existing export-reader validation.The ELF ABI defines file-backed ranges through
p_offset/p_fileszandsh_offset/sh_size. Valid tables ending at EOF, zero-sized symbol tables at EOF and ignored entries afterDT_NULLretain their behavior. Errors use the existing file-qualifiedAuditErrordiagnostics and leave a previous JSON report unchanged.This is independent of #10311, which rejects missing
DT_NULLterminators. That behavior is deliberately not included here; #10311 still accepts the invalid ranges covered by this PR.Tested
maine78acf4bda; tested PR head5bf97cce6b. Publication recheck against newermainad19afc31e: audit source/tests are unchanged upstream and the branch merges cleanly. No title, GPU, macOS or full-runtime run; this changes Python tooling only.66b5034fd3. All five new methods pass with this fix.ANYPS5_RELINKER_ONLY=ON,BUILD_TESTING=ON:cmake --build build-relinker --parallel 4andctest --test-dir build-relinker --output-on-failure --parallel 4; 235/235 CTest entries pass. Independently,python tools/tests/test_import_audit.py -vruns 131 tests successfully, with three existing platform/permission skips (two POSIX permission cases and Windows symlink creation without the required privilege).python3 tools/tests/test_import_audit.py -vruns 131 tests successfully, with the existing Windows-only skip.66b5034fd3: both merge orders succeed with the same resulting tree. The combined parser/test modules run all 133 tests on Windows (three skips) and Linux (one skip) successfully in a Git-blob loading harness. Its subprocess UTF-8 smoke test uses this branch's on-disk CLI; the parser/API and in-process command tests use the merged module.python tools/check_conventions.py --base origin/mainandgit diff --checkpass.tools/pr_overlap.py --branch fix/audit-dynamic-tablesreports no confirmed conflicts. It cannot compare fix(tools): recognize guest module identities in import audit #6171 and fix(tools): resolve $ORIGIN-prefixed converted module dependencies in import_audit #7454 because they already conflict with main. Their current shared-file patches were reviewed manually: fix(tools): recognize guest module identities in import audit #6171 adds module identities, including adjacent dynamic-loop additions; fix(tools): resolve $ORIGIN-prefixed converted module dependencies in import_audit #7454 changes dependency-name resolution. Neither validates these file ranges. This does not claim their existing base conflicts are resolved.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)