Repository navigation
fix(native): correct Mach-O command completeness - #8937
Draft
ducanhnguyen223 wants to merge 2 commits into
Draft
ducanhnguyen223 wants to merge 2 commits into
ducanhnguyen223 wants to merge 2 commits into
Conversation
Contributor
[Medium impact] The PR appears safe to merge; both previous evidence-loss issues are fixed. SummaryCorrects when Mach-O commands count as fully decoded.
Both previous findings are fixed. No new actionable issues or security issues were found. Tests were inspected but not run during this review. Supersession could not be checked because GitHub access was unavailable. Reviews (2) · Last reviewed commit: "fix(native): preserve exact Mach-O comma..." · Reviewed by Greptile |
This branch has not been deployed
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
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.
Summary
Fixes #8935.
Problem and expected behavior
Load commands with every payload byte decoded were still reported as partially decoded, which added an incorrect limitation and redundant whole-command digest.
Change and scope
Treat
LC_LINKER_OPTIONas complete only when the count and NUL-terminated strings exactly consumecmdsize; treatLC_NOTEas complete only at its 40-byte modeled extent and preserve its full owner storage asdata_owner_storage_hex; and treatLC_FILESET_ENTRYas complete only when the entry-id starts at byte 32 and its terminator reachescmdsize. Gaps and extra bytes retain the existing limitation and digest fallback.This is a focused correction in the existing Mach-O reader. No new capability or target behavior is added; a maintainer scope decision is not applicable.
Contract and boundary impact
MachoCommandNamesrecords byte-level completeness;MachoBinaryProjectionconsumes that fact.data_owner_storage_hexfor all 16LC_NOTEowner bytes; existing fields retain their meanings.Expected effect:
Evidence and regression coverage
LC_FILESET_ENTRYwith an unexamined gap, and anLC_NOTEowner with an embedded NUL followed by nonzero bytes. The gap retains its digest/limitation; the complete owner storage is projected as hex.LC_LINKER_OPTION -lz; it verifies the fallback digest and limitation are absent.check:prsuite was not run; note/fileset CLI behavior is covered by synthetic binary fixtures, not separate compiler-generated samples.Validation performed
fnm exec --using=24.22.1 npm run test:local -- src/native/MachoCommandNames.test.ts src/native/MachoBinaryProjection.test.ts— 2 files, 20 tests passed.fnm exec --using=24.22.1 npm run test:focused -- tests/boundary/cli/nativeMachoLinkerOptionsCli.test.ts— 1 file, 2 tests passed; included real Clang-generated local input.fnm exec --using=24.22.1 npm run check:fast— typecheck and lint passed.oxfmt --checkandgit diff --check— passed.Compatibility, safety, and release
data_owner_storage_hexis additive and all existing evidence fields remain.Author readiness
Human owner: @ducanhnguyen223
Before merge