Automated code + security review: fixes (PRs #13-15) and additional findings

[grub-basket]

Hi @jorio! I ran an automated code + security review over rsrcdump and opened focused PRs for the clear-cut fixes. Lower-confidence items and hardening ideas are listed below with suggested fixes so you can decide what's worth acting on.

All prepared with Claude Fable 5 on Low effort mode. Happy to adjust, split, or drop anything.

Pull requests

• Fix broken --create (JSON repack) round-trip #13 — Fix broken --create (JSON repack) round-trip. rsrcdump -c currently raises TypeError on every invocation (a bad encoding= kwarg), so the whole repack path is dead. Also fixes high-byte FourCC type-key round-tripping, the len(type_name) > 4 metadata heuristic that dropped those encoded keys, and a broken JSONEncoderBase16Fallback fallback. Verified with an extract-key → repack round-trip.
• Fix PICT pixel decoding for 32-bit planar and cicn icons #14 — Fix PICT pixel decoding. 32-bit planar pixmaps (unpack4, 4-plane branch) used a 3-plane row stride → scrambled output; cicn read the B&W bitmap with the mask's height → corrupted palette.
• Guard against oversized PICT canvas allocation #15 — Guard against oversized PICT canvas allocation. A malformed PICT canvas rect forces a multi-GB allocation from a few bytes (memory-exhaustion DoS when extracting untrusted forks). Rejects implausible dimensions before allocating; the extractor already falls back to base16 on PICTError.

Additional findings (not PR'd — your call)

1. pack_pstr padding formula is wrong (latent). rsrcdump/packutils.py — pad_count = (1 + len(bintext)) % padding computes the wrong pad count. It's masked today because every caller uses padding=2 (where x%2 == (-x)%2), but any padding != 2 would mis-align chunks. Fix: pad_count = (-(1 + len(bintext))) % padding.

2. StructTemplate rejects the x pad byte. rsrcdump/structtemplate.py (split_struct_format_fields) only accepts CB?HILFQD + s, so a --struct spec using x to skip reserved bytes fails at construction, even though struct handles it and record_length would be correct. Consuming x without emitting a named field would support a common real-world record-layout need. (Note the .field{n} fallback naming would also over-count fields vs. the unpack value tuple, tripping the assert len(self.field_names) == len(values) in tag_values.)

Hardening notes (defensive, low priority)

• assert used to validate attacker-controlled data throughout resfork.py, adf.py, pict.py, sndtoaiff.py. Under python -O, asserts are stripped, so several bounds/dimension checks silently vanish. Converting the security-relevant ones to explicit raise InvalidResourceFork(...) would make them robust regardless of -O.
• Malformed/truncated forks surface as raw tracebacks. main() only catches InvalidResourceFork, so a truncated file or an out-of-range interior length raises struct.error / AssertionError instead of a clean "invalid resource fork" message. Broadening the top-level catch (or validating interior offsets) would fail more gracefully.
• QuickTime → Pillow path (pict.py, opcode 0x8200) hands embedded TIFF/JPEG bytes to PIL.Image.open(...) and swallows all exceptions. Pillow's MAX_IMAGE_PIXELS bounds the worst case, but validating decoded dimensions before the Python BGRA pixel loop (and not silently swallowing every error) would be safer.
• sanitize_type_name defensive note. Not attacker-reachable today (only fixed safe FourCCs reach the separate-file write path, and unknown types fall back to inline base16), but adding an explicit reject for ../path-separators in sanitize_type_name would harden the extract-to-disk path against a future converter that routes an arbitrary type name into a filename.

Security review

Threat model: running rsrcdump -x on an attacker-supplied .rsrc/.adf. I checked path traversal / zip-slip on extraction, unsafe deserialization (eval/exec/pickle/yaml), format-string / template code execution, and unbounded struct reads / allocation bombs. All clean except the PICT canvas allocation bomb (#15) — which is a local-CLI DoS with no code-execution or information-disclosure angle, so it's here in the open rather than a private disclosure. The struct-spec DSL only accepts struct format chars (no code eval), and specs come from operator --struct input, not the untrusted file.

---

Reviewed & prepared with Claude Fable 5 (Low effort mode). Automated findings — each was manually sanity-checked (and the --create fix was smoke-tested), but please review before merging.