Address non-scanner review findings#4
Merged
Merged
Conversation
pointer_size_bytes() defaulted to 8 and was never overridden, so the absolute-pointer (*) and pointer-width skip/push atoms read 8 bytes on 32-bit ELF/Mach-O images (both of which the wrappers accept). Override it from the parsed image class (elf.is_64 / MachO.is_64). PE is unaffected (it already rejects non-64). Adds a fixture test asserting the width.
- Enforce per-arm {/} balance in parse_group: an unbalanced brace inside an
alternation arm (e.g. "(e8 ${ | 90)") was silently swallowed by the per-arm
depth reset and produced a dangling Push/Jump with no Pop. Now rejected.
- Remove the unreachable empty-alternatives branch and the unused Parser
source field (drops the struct lifetime).
- Rewrite compile_alternatives from recursion to an iterative right-fold
(identical atom output, no per-arm recursion).
- Doc: note trailing-skip stripping and Atom::Aligned's valid range (<64).
- Add a private loadmap module (LoadMap/LoadRange) and use it from both the ELF and Mach wrappers, removing the duplicated LoadRange + lookup logic. - Replace the Cell<Option<usize>> resolution caches in all three wrappers with a relaxed AtomicUsize, so PeFile/ElfFile/MachFile are Sync and a parsed image can be shared across threads for parallel scanning. Add a static Sync assertion. - Document the fat-Mach first-slice behavior and the PE code-span extent convention.
mapped_c_str scanned for the terminating NUL in file space without checking that the bytes stay contiguous in mapped space, so a string near a region's end could read into unrelated, non-adjacent file bytes. Keep the fast file-space NUL scan and verify contiguity with a single extra lookup of the terminator (within a region the mapping is linear), mirroring mapped_slice_strict. Adds a regression test.
Invalid pattern syntax, a non-string-literal argument, and bad escapes now expand to a compile_error! at the call site instead of panicking the compiler, giving a clear diagnostic. Valid patterns are unchanged.
cargo test --all-targets excludes doctests, leaving the crate's extensive executable docs unvalidated. Add a dedicated --doc test step.
- pelite_compare: assert goblin/pelite first-match parity per case before timing, so a matching regression can't masquerade as a speedup (totals legitimately differ due to different code-scan scope). - scan_perf: gate the per-pattern shape dump behind SIGSCAN_BENCH_SHAPES; add a Mach-O (aarch64) scan group. - Add a parse_perf bench for the runtime pattern parser.
- CLI: --quiet/-q suppresses per-match output so the scan loop dominates a flamegraph instead of stdout formatting. - workspace Cargo.toml: [profile.release] debug = "line-tables-only" for readable, inline-aware flamegraph frames. - flamegraph-scan.sh / scripts README: use --quiet and note the Linux perf_event_paranoid requirement.
killerra
force-pushed
the
review/non-scanner-findings
branch
from
June 25, 2026 19:41
b58bf1c to
96f3282
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the non-scanner findings from the full code review. The scan-engine
items (
scan.rs:VM-*,SCAN-*) are intentionally left for a separate,focused PR.
Highlights
pointer_size_bytes()was hardcoded to 8 and neveroverridden, so
*(Ptr),Skip(0), andPush(0)read 8 bytes on 32-bitELF/Mach images — both of which the wrappers accept (32-bit fixtures ship in the
repo). Now derived from the image class (
elf.is_64/MachO.is_64); PE isunaffected (it already rejects non-64). Covered by a new fixture test.
What's included
{/}inside a group arm (was silentlyswallowed); drop a dead branch and the unused
Parsersource field; rewritecompile_alternativesiteratively (identical output); doc notes for trailing-skipstripping and
Atom::Aligned's valid range.loadmapmodule; replace the
Cellresolution caches withAtomicUsizeso all threewrappers are
Sync(parallel scanning) — asserted statically; doc the fat-Machfirst-slice behavior and PE code-span extent convention.
mapped_c_strnow requires the string to stay contiguous inmapped space (one extra terminator lookup, not one per byte) instead of scanning
file bytes past a region boundary.
pattern!misuse expands tocompile_error!instead ofpanicking the compiler.
--all-targetsskips them).pelite_compare; gate theshape dump behind
SIGSCAN_BENCH_SHAPES; add a Mach scan group and aparse_perfbench.
--quietflag;[profile.release] debug = "line-tables-only"for readable flamegraphs; script/README perf notes.Behavior changes (intentional, all more correct)
a malformed atom stream.
mapped_c_strreturnsNone(instead of a possibly-wrong string) when a stringwould cross a mapping hole.
pattern!misuse is a compile error rather than a panic.Deferred
All
scan.rsengine items (VM-*,SCAN-*),FMT-6(mapped-extent clamp — changesscan coverage), and
MACRO-1(string →TokenStreamrewrite).Verification
cargo test --workspace --all-targets --all-features,cargo test --workspace --doc,cargo clippy --workspace --all-targets --all-features -- -D warnings -D clippy::all,and
cargo fmt --all -- --checkall pass.