fix(mft): default-pipeline MFT correctness gaps + panic hardening - #569
Merged
Conversation
…sers StandardInfo already declares usn/security_id/owner_id and StandardInfo:: from_extended already copies them through, but the two record-parsing pipelines that actually run in production never called it: - io/parser/unified.rs process_record (the default MftReadMode::Auto -> SlidingIocpInline bulk-load path) unconditionally read only the 36-byte NTFS 1.2 StandardInformation and built StandardInfo via from_raw_ntfs_flags, whose own doc says the caller must set these three fields separately -- it never did. - parse/direct_index.rs parse_record_to_index (the live USN-journal incremental-update path, called from usn/windows.rs) had the identical bug: read only the 36-byte struct and dropped usn/security_id/owner_id before reaching StandardInfo::from_extended. Only the legacy ParsedRecord pipeline (parse/attribute_helpers.rs parse_standard_info_full, reachable via non-default read modes / the DataFrame export path) was already correct -- it branches on value_length to read the 72-byte NTFS 3.0+ StandardInformationExtended form when present. Fix: bump parse_standard_info_full to pub(crate) and call it from both production parsers instead of duplicating (buggy) inline logic. Net deletion of duplicated code in favor of the one already-tested implementation. These fields have no bearing on ordinary search/filter/display -- only forensic/security-auditing consumers (timestomping detection via STD_INFO vs FILE_NAME divergence, ACL/USN correlation) read them -- so this was a silent, zero-functional-impact gap until now, not a live bug. Fixing it is free: the fields already exist in StandardInfo's on-disk layout, the bytes are already read off disk in the same pass, and the extra cost is one well-predicted branch. Out of scope for this commit (flagged, not touched): - io/parser/index.rs's parse_record_to_index has zero production call sites (only its own test module calls it) -- looks like dead code, left alone to avoid fixing unreachable paths. - $FILE_NAME's own per-name timestamps: the modern MftIndex per-name storage (IndexNameRef/first_name/hard-link chain) has no timestamp fields at all, unlike the legacy FileRecord/NameInfo struct. Adding them means real, permanent per-hard-link storage growth -- a different cost/benefit decision than this free fix. Added a regression test exercising both production parsers end-to-end against a synthetic 72-byte StandardInformationExtended record.
…lete superseded dead parser Investigating whether io/parser/index.rs's parse_record_to_index (zero production call sites, only its own test module) was legacy/dead code surfaced a more serious finding: it isn't just dead -- it's the WI-5.2 panic-hardened (checked arithmetic, .get()-only access) sibling of the parser that's actually live, and the live one never got that hardening. parse/direct_index.rs's parse_record_to_index -- the parser actually wired to the daemon's live USN-journal incremental-update path via usn/windows.rs:466 -- reads a resident attribute's 4-byte value_length (offset+16..20) and 2-byte value_offset (offset+20..22) via raw &data[a..b] slicing in 7 places (StandardInformation, FileName, ReparsePoint x2, IndexRoot x2, ObjectId/EA/etc, and the unknown-type catch-all), with no bounds check beyond the outer attribute-length gate. A resident attribute whose *declared* length is short enough to pass that gate but too short to cover those fixed fields -- e.g. sitting at the tail of a truncated or corrupted record -- panics: range start index 76 out of range for slice of length 74 reproduced live against a crafted record. The daemon builds with panic = "abort", so this is a real whole-process DoS on a malformed MFT record during journal replay, not a theoretical one. The existing malformed_records_do_not_panic corpus never caught it because (a) it never exercised this specific parser at all, and (b) RecordBuilder leaves bytes_in_use at 0, which makes every parser's attribute loop short-circuit before touching a single attribute byte -- so the corpus wasn't reaching the code it claimed to stress-test for any parser. Fix: - Add rd_u16/rd_u32 (checked, .get()-based, mirroring unified.rs's existing helpers) and route all 7 unguarded reads through them. - assert_all_parsers_survive now patches bytes_in_use to the record's real length before running each parser, so the existing corpus actually reaches attribute-body code instead of trivially short- circuiting; added crate::parse::parse_record_to_index (the real, live parser) to the parsers it exercises. - Added a dedicated regression case reproducing the exact bug shape (short declared length at the tail of the buffer) across all 6 affected attribute types. Verified red before the fix (panics with the exact message above), green after. With direct_index.rs now at parity, io/parser/index.rs (+ its index_extension.rs helper, 1671 lines total) is pure duplicated dead code, not a fallback worth keeping -- deleted both, along with the index_helpers.rs functions (InternalStreamChain, ExtensionSnapshot, merge_extension_streams, merge_extension_names) that existed only to serve them. Updated io.rs / io/parser/mod.rs re-exports accordingly (parse_record_to_index is no longer reachable via uffs_mft::io::*; confirmed zero consumers anywhere in this repo or uffs-products). Fixed docs/architecture/engine/03-ntfs-parsing.md's two source-path references, which had pointed at the dead file all along.
…ction parsers IndexStreamInfo and InternalStreamInfo both already declare bit0=is_sparse, bit1=is_resident in their packed flags byte -- but every write site in both production parsers (unified.rs's process_record and direct_index.rs's parse_record_to_index) hardcoded those two bits to 0/false regardless of the real attribute, for every stream type: the default $DATA stream, every named $DATA (ADS), $REPARSE_POINT, non-$I30 $INDEX_ROOT/$INDEX_ALLOCATION/$BITMAP, $OBJECT_ID and friends, and the unknown-type catch-all. Every ADS reported as non-sparse/non-resident no matter what it actually was. Both bits are free to populate: is_resident is already known everywhere (attr_header.is_non_resident == 0, already read to pick the size-calc branch), and is_sparse lives in the attribute record header's own ATTRIBUTE_FLAG_SPARSE bit (0x8000), already-parsed data with no new I/O. No change to any on-disk struct layout or size. While auditing this, found and fixed the same panic-on-malformed-input gap in parse/direct_index_extension.rs (the extension-record sibling of direct_index.rs) that was fixed for direct_index.rs itself in the previous commit: unguarded 16-byte value_length / 2-byte value_offset reads via raw &data[a..b] slicing, now routed through checked rd_u16/rd_u32 helpers. This file processes extension records on the same live USN-journal path and had never received the WI-5.2 hardening pass either. - index_helpers.rs: add_stream_to_index now takes is_sparse/is_resident and bakes them into the flags byte alongside the existing type_name_id bits. Added a StreamEntry type alias ((name, size, allocated, is_sparse, is_resident)) to keep the SmallVec tuple under clippy::type_complexity. - direct_index.rs / direct_index_extension.rs: compute is_sparse/is_resident at every one of the ~10 stream-producing sites (Data/ADS, ReparsePoint, IndexRoot family, ObjectId family, catch-all) and thread them through; set the default stream's first_stream.flags, which neither file did before (it was left at its zero default, losing type_name_id too). - unified.rs: computed once per attribute in the shared catch-all dispatch arm and applied to all 4 write sites ($I30, default $DATA, ADS, internal-stream). Added a regression test building a synthetic non-resident, sparse-flagged ADS and asserting IndexStreamInfo::is_sparse()/is_resident() come out correct (not hardcoded false) on both process_record and parse_record_to_index.
…roduction parsers FileRecord already declares lsn (Log File Sequence Number, from the header's own log_file_sequence_number), namespace (the primary name's $FILE_NAME namespace), and fn_created/fn_modified/fn_accessed/fn_mft_changed ($FILE_NAME's own timestamps, which often differ from $STANDARD_INFORMATION -- e.g. timestomping alters STD_INFO but leaves FILE_NAME original). All five fields are exactly the same bug class as usn/security_id/owner_id and is_sparse/ is_resident fixed in the previous two commits: already declared on the struct, already fully decoded in memory (the header and $FILE_NAME attribute are both read in full regardless), but never copied onto the record by either unified.rs's process_record or direct_index.rs's parse_record_to_index -- so every record silently read back lsn=0, namespace=0, and all four fn_* timestamps=0, no matter what the disk actually held. - unified.rs: lsn set alongside the existing base-record-only sequence_number (extension records carry their own, differently-scoped LSN, matching the sequence_number precedent already established there). namespace/fn_* set at the same point $FILE_NAME's name/parent_frs are already written, matching the file's existing "push-to-front, most recent $FILE_NAME wins" model. - direct_index.rs: same fields threaded through via new locals (primary_fn_created/modified/accessed/mft_changed), captured whenever a name is chosen as primary (the existing Win32 > POSIX > DOS priority logic), written to the record alongside sequence_number. While auditing this, found and fixed a related pre-existing gap in direct_index.rs: the "no $FILE_NAME in base record" early-return path (the name arrives later via an extension record) never set sequence_number or lsn at all -- and nothing else in the pipeline would either, since an extension record's header carries its own, different-meaning sequence/LSN. A record whose name lands in an extension record was silently missing its own identity fields. Added regression tests: one asserting lsn/namespace/fn_* reach both production parsers from a real $FILE_NAME attribute, one asserting sequence_number/lsn are still set via the no-name early-return path.
… policy for real The previous three commits' fixes pushed both files over the file-size policy threshold. Fixed it properly instead of adding policy exceptions: - direct_index.rs: extracted four small helpers (is_primary_attribute/extract_attr_name/read_size_allocated/ resident_and_sparse) that were each duplicated verbatim across 4-5 match arms (Data, ReparsePoint, IndexRoot-family, ObjectId-family, catch-all) -- genuine deduplication, not just fewer lines. 856 -> 724 lines, and every one of those match arms is now shorter and less repetitive to read, not just smaller on a line-count report. - unified.rs: split the NTFS UTF-16/WTF-8 name-decoding cluster (decode_utf16le_into, decode_name_u16, wtf8_from_utf16le, store_name_lossless, the LOSSY_NAME_COUNT tally) into a new sibling module, unified/name_codec.rs. That cluster has zero dependency on process_record's attribute-loop state -- it only needs MftIndex -- so it was always a separable concern, just never separated. Re-exported decode_name_u16/lossy_name_count from unified.rs so the ~9 other modules that call them via crate::io::parser::unified::* keep working unchanged. 803 -> 559 lines in unified.rs; name_codec.rs is 270 lines, comfortably under the threshold on its own. Also extracted a small resident_value_offset(data, attr_offset) helper in unified.rs for the value-offset-field read duplicated between the $FILE_NAME and $REPARSE_POINT arms. All prior documentation is preserved verbatim -- this is a structural split, not a comment trim. cargo test -p uffs-mft: 260/260 passing, identical to before the split (the extraction is behavior-preserving by construction: same logic, moved, not rewritten). Neither file needs a scripts/ci/file_size_exceptions.txt entry anymore.
… via an extension record FileRecord.base_frs audit: confirmed correct as-is. Traced parse/merger.rs (the legacy pipeline) directly -- extension ParsedRecords are fully merged into their base record and never survive into the final Vec<ParsedRecord>, so base_frs is genuinely Frs::ZERO for every record that reaches MftIndex, in both the legacy and modern pipelines. Not a bug; no change needed. Continuing the same audit surfaced a real one: direct_index_extension.rs's "base record has no $FILE_NAME, promote the first extension name to primary" merge path (the case where a file has enough attributes to overflow its base MFT record and $FILE_NAME lands in an extension record) copied only the name text and parent FRS into first_name, silently dropping namespace and all four $FILE_NAME timestamps -- even though `$FILE_NAME`'s own attribute bytes were already fully decoded and even already read once by the same function. Every such record read back namespace=0 and fn_created/fn_modified/fn_accessed/fn_mft_changed=0 forever, with nothing else in the pipeline able to fix it later. Root cause: the `names` collection only ever carried (name, parent_frs) -- namespace/timestamps were decoded from `fn_attr` and then discarded before even reaching the promotion site. Extended it to a 7-field ExtNameEntry tuple (name, parent_frs, namespace, fn_created, fn_modified, fn_accessed, fn_mft_changed) and copy those fields onto the record alongside the name whenever it's promoted to primary. `LinkInfo` (storage for non-primary hard links) still has no room for these fields, so this only applies to whichever name ends up primary -- the same structural constraint as FileRecord's own fn_* fields. While touching this file, applied the same helper-extraction refactor already done for direct_index.rs (is_primary_attribute/extract_attr_name/ read_size_allocated/resident_and_sparse) -- same duplicated logic, same fix. 821 -> 762 lines, genuinely under the file-size policy threshold now instead of relying on its pre-existing PERMANENT exception, which is removed along with the stale "same reasoning as direct_index.rs" claim (direct_index.rs no longer needs one either). Added a regression test: base record with zero $FILE_NAME attributes, followed by an extension record carrying the file's only name, asserting namespace/fn_* land on the merged record. Verified red before the fix, green after.
Found during this same MFT-parser audit: CLAUDE.md described a fast/full mode toggle where the default path "skips extension MFT records" and --full "merges" them. Neither exists anymore -- confirmed via direct grep that no CLI flag, no fast_mode/full_mode concept, and no skip-extension-records behavior exists anywhere in uffs-cli or the current reader code. Both production parsers (unified.rs, direct_index.rs + direct_index_extension.rs) always merge extension records in the same pass; this was pre-unification legacy behavior that the "one function processes ALL records" consolidation (unified.rs's own module doc) already superseded.
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
1 participant
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
A completeness/correctness audit of
uffs-mft's two production record parsers (io::parser::unified::process_record, the default bulk-load pipeline, andparse::direct_index::parse_record_to_index+direct_index_extension.rs, the live USN-journal incremental-update pipeline) found a recurring bug class: struct fields that were already declared, with the underlying bytes already fully parsed into memory, but never actually copied onto the record on the path that runs in production today. Every one of these was populated correctly by the legacyParsedRecordpipeline (reachable only via non-default read modes / the DataFrame export path) — the default path silently diverged.Fixed, in order:
StandardInfo.usn/security_id/owner_id— both parsers only ever read the 36-byte NTFS 1.2$STANDARD_INFORMATIONform regardless of the record's realvalue_length, so every NTFS 3.0+ record silently lost these three fields. Routed both parsers through the already-correct, already-testedparse_standard_info_full(previously legacy-only) instead of duplicating buggy inline logic.Panic-on-malformed-input hardening — while investigating whether a third parser (
io/parser/index.rs) was dead code, found it was actually the WI-5.2 panic-hardened sibling of the parser that's actually live, and the live one (direct_index.rs) never received that hardening. Reproduced and fixed a real panic (range start index ... out of range) on a crafted malformed record on the daemon's live incremental-update path (the daemon builds withpanic = "abort", so this was a real whole-process DoS, not theoretical).io/parser/index.rs+ its extension helper (1868 lines) were then deleted as pure duplicated dead code, confirmed zero production call sites in this repo oruffs-products.IndexStreamInfo/InternalStreamInfo.is_sparse/is_resident — both bits already existed in the packed flags byte; every write site across both parsers (~10 call sites: default$DATA, every ADS,$REPARSE_POINT, non-$I30index attributes,$OBJECT_ID-family, the unknown-type catch-all) hardcoded them to false regardless of the real attribute. Both bits are free to populate (is_residentfrom already-branched-onis_non_resident,is_sparsefrom the attribute header's own already-parsedATTRIBUTE_FLAG_SPARSEbit).FileRecord.lsn/namespace/fn_created/fn_modified/fn_accessed/fn_mft_changed—lsn(Log File Sequence Number, forensic value) sat unused in the already-parsed record header;namespaceand$FILE_NAME's own four timestamps (which diverge from$STANDARD_INFORMATIONunder timestomping) were decoded fromfn_attrand then discarded before ever reaching the record. Also fixed an adjacent pre-existing gap: a record whose$FILE_NAMEarrives only via a later extension record never gotsequence_number/lsnset at all on the early-return path.Real 800-LOC-policy fix, not an exception — the fixes above pushed
direct_index.rsandunified.rsover the file-size policy threshold. Fixed it properly: extracted four small helpers that were each duplicated verbatim across 4-5 match arms indirect_index.rs(genuine dedup, not line-count padding), and splitunified.rs's self-contained NTFS name-decoding cluster into a new sibling module (unified/name_codec.rs) with zero behavior change. Both files are now genuinely under threshold with real margin; nofile_size_exceptions.txtentries added.Extension-record merge path — applying the same audit to
direct_index_extension.rs's "base record has no$FILE_NAME, promote the first extension name to primary" path found the identical bug: name text and parent FRS were copied, but namespace/timestamps were decoded and then discarded. Fixed, and applied the same helper-extraction refactor there too (its own pre-existing file-size exception is no longer needed either).Explicitly out of scope (by design, not oversight)
Confirmed via direct tracing of
parse/merger.rsthatFileRecord.base_frs/is_extensionare correctly always-zero in both pipelines (extensionParsedRecords are fully consumed during merge and never survive as standalone entries) — not a bug.reparse_tagis deliberately not surfaced to the publicCompactRecord(already correct inMftIndex, kept out of the free/public search index by design). Per-additional-hardlink$FILE_NAMEtimestamps,$DATALCN capture,$REPARSE_POINTtarget path,$OBJECT_ID,$EAcontent, and ADSis_compressedall need either new storage or new parsing — deferred to the private rich-index work, not public bugs.Test plan
cargo test -p uffs-mft --lib— 261/261 passingjust lint-prod/just lint-tests/cargo fmt --check/ file-size policy /cargo doc --document-private-itemsall green on every commitcargo check --workspace(index-reading logic builds off-Windows too)