dm2-saves: robustness batch — silent header rewrites, silent map-table skips, recursion depth, tile-index panics, unlink-before-validate #175

New issue

Closed

opened 2026-07-04 15:15:18 +02:00 by jqueuniet · 1 comment

jqueuniet commented

2026-07-04 15:15:18 +02:00

(Migrated from codeberg.org)

Batch of low-severity robustness items from a full-crate review. None affect the current fixtures; all are silent-misbehavior or panic hazards on unusual input.

SaveFile::to_bytes zeroes n_records[11..=13] (sksave.rs ~466-469): the recompute uses arr.len().checked_div(size).unwrap_or(0), and sizes for types 11-13 are 0 — a save whose header carried nonzero counts for the unused types would be silently rewritten, breaking byte-exactness. Preserve the parsed counts for zero-size types.
total_columns silently skips unparseable map entries (sksave.rs ~186-192, filter_map): a corrupt map table yields a wrong column sum and mis-sliced sections instead of a parse error.
Unbounded recursion in read_chain/write_chain (checkcode.rs): creature→possessions→creature / chest→contents→chest nesting is bounded only by input size (~60 bits per level); a multi-hundred-KB adversarial bitstream can overflow the stack (abort, not catchable). Typical fuzz input sizes won't reach it — consider a depth cap.
Direct map.tiles[...] indexing after width/height-only checks (records.rs ~631 is_teleporter_detail_tile, ~707 / ~1002 tile-chain decode/encode): panics if a hand-built DungeonMapInfo has tiles.len() < width*height. The walker paths (~1265) are defensive; these aren't.
Unlink-before-validate in removal paths (object_place.rs ~899 remove_object, ~940 despawn_creature, chest/creature removals): unlink_dynamic_record performs irreversible chain surgery before decode_object is checked; an Undecodable error leaves the record orphaned. Validate first (or re-link on error).
Dead phase-7 accumulation on the encoder (checkcode.rs ~969/987): cloud_records is collected but encode_cloud_postfix replays the decoded entries without cross-checking, so an edit that changes container-flag/missile membership silently desyncs the postfix count vs what the engine accumulates on load. Also CloudPostfixEntry.rec_index is always 0 (placeholder), which is misleading to consumers.

Batch of low-severity robustness items from a full-crate review. None affect the current fixtures; all are silent-misbehavior or panic hazards on unusual input. - [ ] **`SaveFile::to_bytes` zeroes `n_records[11..=13]`** (`sksave.rs` ~466-469): the recompute uses `arr.len().checked_div(size).unwrap_or(0)`, and sizes for types 11-13 are 0 — a save whose header carried nonzero counts for the unused types would be silently rewritten, breaking byte-exactness. Preserve the parsed counts for zero-size types. - [ ] **`total_columns` silently skips unparseable map entries** (`sksave.rs` ~186-192, `filter_map`): a corrupt map table yields a wrong column sum and mis-sliced sections instead of a parse error. - [ ] **Unbounded recursion in `read_chain`/`write_chain`** (`checkcode.rs`): creature→possessions→creature / chest→contents→chest nesting is bounded only by input size (~60 bits per level); a multi-hundred-KB adversarial bitstream can overflow the stack (abort, not catchable). Typical fuzz input sizes won't reach it — consider a depth cap. - [ ] **Direct `map.tiles[...]` indexing after width/height-only checks** (`records.rs` ~631 `is_teleporter_detail_tile`, ~707 / ~1002 tile-chain decode/encode): panics if a hand-built `DungeonMapInfo` has `tiles.len() < width*height`. The walker paths (~1265) are defensive; these aren't. - [ ] **Unlink-before-validate in removal paths** (`object_place.rs` ~899 `remove_object`, ~940 `despawn_creature`, chest/creature removals): `unlink_dynamic_record` performs irreversible chain surgery before `decode_object` is checked; an `Undecodable` error leaves the record orphaned. Validate first (or re-link on error). - [ ] **Dead phase-7 accumulation on the encoder** (`checkcode.rs` ~969/987): `cloud_records` is collected but `encode_cloud_postfix` replays the decoded entries without cross-checking, so an edit that changes container-flag/missile membership silently desyncs the postfix count vs what the engine accumulates on load. Also `CloudPostfixEntry.rec_index` is always 0 (placeholder), which is misleading to consumers.

jqueuniet commented

2026-07-06 08:47:22 +02:00

(Migrated from codeberg.org)

Fixed by #209 (merged into main).

Fixed by #209 (merged into `main`).

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

jqueuniet/dm2-tools#175

No description provided.

Rows
Columns