Fix load_elf to support non-file istream paths and add regression tests#1048
Fix load_elf to support non-file istream paths and add regression tests#1048
Conversation
load_elf(istream, path) calls filesystem::file_size(path) for section bounds validation, but this fails when path does not refer to an actual file (e.g. path="memory" for in-memory ELF data). Fall back to computing the data size from the input stream via seekg/tellg. Also add regression tests for the assertion crash fix (aa31b0e) in Assume with operands of different inferred types. Signed-off-by: Michael Agun <danielagun@microsoft.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplace direct use of std::filesystem::file_size for istream-based ELF input with a new get_data_size helper that falls back to measuring the stream; use data_size for ELF section bounds validation. Add a regression test for reading an ELF from a non-file path stream and add assume-type-mismatch test cases in YAML. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bump external/ebpf-verifier from e68a347b to b9010eee (main), which includes the fix for the verifier_fuzzer CI timeout (issue microsoft#5055). Handle breaking API changes: - Include io/elf_reader.hpp (header split in microsoft#1026) - Re-implement _instype()/collect_stats() locally (removed in microsoft#1042 but needed for ebpf_stat_t public API) - Add resolve_ksym_btf_id nullptr to platform structs - Patch load_elf stream-size fallback for in-memory ELF loading (vbpf/prevail#1048)
Bump external/ebpf-verifier from e68a347b to b9010eee (main), which includes the fix for the verifier_fuzzer CI timeout (issue microsoft#5055). Handle breaking API changes: - Include io/elf_reader.hpp (header split in microsoft#1026) - Re-implement _instype()/collect_stats() locally (removed in microsoft#1042 but needed for ebpf_stat_t public API) - Add resolve_ksym_btf_id nullptr to platform structs - Patch load_elf stream-size fallback for in-memory ELF loading (vbpf/prevail#1048)
|
@elazarg Hi Elazar, would you have time to review this soon? |
Hi, thanks for the PR! I plan to review it next week. |
elazarg
left a comment
There was a problem hiding this comment.
Thanks for the commit! A small suggestion, and a note - this looks like two unrelated PRs. Please separate PRs in the future (no need to split this one now).
|
@mikeagun note the DSO check failure - please amend-sign-off and force-push |
Signed-off-by: Michael Agun <danielagun@microsoft.com>
|
Thanks! |
Bump external/ebpf-verifier from e68a347b to b9010eee (main), which includes the fix for the verifier_fuzzer CI timeout (issue microsoft#5055). Handle breaking API changes: - Include io/elf_reader.hpp (header split in microsoft#1026) - Re-implement _instype()/collect_stats() locally (removed in microsoft#1042 but needed for ebpf_stat_t public API) - Add resolve_ksym_btf_id nullptr to platform structs - Patch load_elf stream-size fallback for in-memory ELF loading (vbpf/prevail#1048)
…changes (#5078) * Bump PREVAIL verifier to b9010eee and handle breaking changes Bump external/ebpf-verifier from e68a347b to b9010eee (main), which includes the fix for the verifier_fuzzer CI timeout (issue #5055). Handle breaking API changes: - Include io/elf_reader.hpp (header split in #1026) - Re-implement _instype()/collect_stats() locally (removed in #1042 but needed for ebpf_stat_t public API) - Add resolve_ksym_btf_id nullptr to platform structs - Patch load_elf stream-size fallback for in-memory ELF loading (vbpf/prevail#1048) * bump submodule * Pass Instruction by const ref in _instype * Update DLL dependency baselines for PREVAIL verifier bump --------- Co-authored-by: Michael Agun <danielagun@microsoft.com>
…changes (microsoft#5078) * Bump PREVAIL verifier to b9010eee and handle breaking changes Bump external/ebpf-verifier from e68a347b to b9010eee (main), which includes the fix for the verifier_fuzzer CI timeout (issue microsoft#5055). Handle breaking API changes: - Include io/elf_reader.hpp (header split in microsoft#1026) - Re-implement _instype()/collect_stats() locally (removed in microsoft#1042 but needed for ebpf_stat_t public API) - Add resolve_ksym_btf_id nullptr to platform structs - Patch load_elf stream-size fallback for in-memory ELF loading (vbpf/prevail#1048) * bump submodule * Pass Instruction by const ref in _instype * Update DLL dependency baselines for PREVAIL verifier bump --------- Co-authored-by: Michael Agun <danielagun@microsoft.com>
…changes (#5078) * Bump PREVAIL verifier to b9010eee and handle breaking changes Bump external/ebpf-verifier from e68a347b to b9010eee (main), which includes the fix for the verifier_fuzzer CI timeout (issue #5055). Handle breaking API changes: - Include io/elf_reader.hpp (header split in #1026) - Re-implement _instype()/collect_stats() locally (removed in #1042 but needed for ebpf_stat_t public API) - Add resolve_ksym_btf_id nullptr to platform structs - Patch load_elf stream-size fallback for in-memory ELF loading (vbpf/prevail#1048) * bump submodule * Pass Instruction by const ref in _instype * Update DLL dependency baselines for PREVAIL verifier bump --------- Co-authored-by: Michael Agun <danielagun@microsoft.com>
Fix load_elf istream fallback and add Assume regression tests
load_elf fix: The ELF loader overhaul (#1026) added section-bounds validation using file_size(path) in
load_elf(istream, path). This breaks callers that pass in-memory data via std::stringstream with a non-file path
like "memory" — file_size("memory") fails with ERROR_FILE_NOT_FOUND. Fix: when file_size(path) fails, fall back to
seekg(0, end) + tellg() on the input stream. The section-bounds check still runs; only the size source changes.
Assume regression tests: The assertion crash fix (#1012, aa31b0e) shipped without test coverage. Added 3 YAML test
cases for reg-vs-reg Assume with differing inferred types (ctx==number, packet!=number, stack<number) and 1 Catch2
test for the istream non-file-path path.
Closes #1049
Summary by CodeRabbit
Bug Fixes
Tests