fix(serializers): bound untrusted msgpack decode depth and header allocation (LAB-2503)

[27Bslash6]

What & why (LAB-2503)

Every cache read decodes MessagePack bytes the backend controls. msgpack-python's C unpacker pre-allocates each container (PyList_New(n)) before decoding its children, and nested headers stack those allocations depth-first. The ticket assumed an "82 MB hard ceiling"; that was an artifact of the array16(10000) probe — with array32 headers claiming len(input) the library defaults allow ~8 × 1024 × len(input): 10 KB → 67 MB measured, linear in input, and N concurrent poisoned reads multiply it.

The fix

unpackb_bounded(data, **opts) in serializers/base.py, now the only msgpack.unpackb call site (auto ×11, standard, decode_interop_value):

1. Zero-copy structural walk first — check_msgpack_structure(data, MSGPACK_MAX_NESTING) in the Rust extension (rust/src/msgpack_bounds.rs, opcode table mirrors cachekit-rs check_structure). Header-only: str/bin/ext payloads are skipped by offset, the input is borrowed in place (bytes, or the read-only memoryview-of-bytes the read path carries), and the only allocation is one u64 per open collection. Rejects nesting past 1024 (MSGPACK_MAX_NESTING, cachekit's own ceiling, bounded above by the C unpacker's stack) and any point where the elements still owed by open headers exceed the remaining input — so a 15 KB array16(2000) spine is rejected at the 8th header, not after a 1024-level descent. Every element that survives is backed by ≥ 1 byte, so the real decode's total pre-allocation is bounded by len(data) rather than depth × declared length. Rejections raise ValueError naming the bound; all read paths already turn that into a controlled cache miss. Measured: 2–13 % of decode time on collection-heavy payloads (1M ints: 2.1 ms vs 29.3 ms), ~0 on a 50 MiB bin, 0 B Python-heap peak.
2. Explicit max_*_len=len(data) on unpackb — unreachable once the walk passes; defence in depth against a walk regression, documented as such.

Also fixed on the way (found by the new test): AutoSerializer fail-open — when a checksum-verified ByteStorage envelope's payload failed to decode, the except Exception fallback re-decoded the envelope bytes as plain MessagePack and returned its positional fields as the cached value (the LAB-1765 class of bug). Now raises SerializationError. The plain path's final error now carries the envelope/msgpack/numpy reasons instead of surfacing only "expected NUMPY_RAW header".

Exception contract: the broad except Exception clauses in AutoSerializer.deserialize now catch PAYLOAD_DECODE_ERRORS (ValueError, TypeError, KeyError, AttributeError, OverflowError, BufferError — defined once in base.py, shared with StandardSerializer), so a missing optional dependency (RuntimeError) bubbles instead of reading as a corrupt entry.

History: the first version of the walk used msgpack.Unpacker(...).skip(), whose feed() copies the input — that +1× transient tripped the File-backend 3.5× allocation bound in CI (4.00×). The Rust walk replaced it; the bound passes.

Tests

tests/unit/protocol/test_decode_bounds.py: the protocol's decode-bounds.json (vendored, sha-pinned) run through 7 decode paths — unpackb_bounded, interop, standard plain/envelope, auto plain/envelope, and CacheSerializationHandler.deserialize_data on a forged CK v3 frame — asserting rejection as ValueError/SerializationError with tracemalloc peak < 2 MiB + 4×input on every reject vector, decode on every accept vector, the 1024/1025 nesting boundary, and trailing-byte rejection. tests/unit/test_auto_serializer_new_types.py: a forged ndarray payload (itemsize past C long → OverflowError inside the object hook) is a SerializationError on both the plain and verified-envelope paths. Full unit + critical suite green (2286); tests/performance/test_large_object_memory.py 8/8 including the previously red File-backend bound.

Review

Round 1 (skip-based walk), expert panel at critical stakes: security NO FINDINGS; craftsman/bug-hunter findings applied (empty StackError message, hidden decode error behind the NumPy fallback, dishonest "two bounds" docstring, feed-copy cost recorded); catchphrase cuts applied.

Round 2 (Rust walk), same panel: security NO FINDINGS after 200k fuzz probes (no abort under panic=abort, no walker/decoder desync vs msgpack-python 1.2.1 across all 256 markers, depth boundary matches the C unpacker exactly); bug-hunter found the OverflowError/TypeError gaps the exception narrowing exposed (fixed + pinned); craftsman/catchphrase: pure walk moved out of the FFI file, PAYLOAD_DECODE_ERRORS centralised, stale StackError-era comments rewritten, BytesView folded to two variants, unreachable UnpackException dropped. Deferred with tickets: ByteStorage.retrieve error typing (checksum mismatch vs not-an-envelope both raise ValueError), duplicated DataFrame/Series integrity-on branches, core-shared zero-copy walk for py/rs/wasm (this PR ships the py-local one).

Docs

README "Production Hardened" bullet; unpackb_bounded docstring is the canonical rationale (doctest-executed), mechanism documented on check_msgpack_structure in rust/src/msgpack_bounds.rs. Protocol spec/vectors: cachekit-io/protocol#59. Sibling: cachekit-io/cachekit-rs#73.

[coderabbitai]

Important

Approval pending

CodeRabbit has no unresolved comments, but it has not reviewed the latest commit.

Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[kodus-27b]

Found critical issues please review the requested changes

• Violates 'Don’t Use assert for Data Validation'

[codecov]

Codecov Report

❌ Patch coverage is 71.11111% with 13 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/cachekit/serializers/auto_serializer.py	59.37%	11 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[27Bslash6]

@kody start-review

[kodus-27b]

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration

Interacting with Kody

• Request a Review: Ask Kody to review your PR manually by adding a comment with the @kody start-review command at the root of your PR.

• Validate Business Logic: Ask Kody to validate your code against business rules by adding a comment with the @kody -v business-logic command.

• Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Current Kody Configuration

Review Options

The following review options are enabled or disabled:

Options	Enabled
Bug	✅
Performance	✅
Security	✅
Business Logic	✅

Access your configuration settings here.

​

[kodus-27b]

WHAT: _deserialize_numpy catches only (ValueError, TypeError, IndexError), but np.frombuffer with the attacker-controlled dtype_str decoded at line 721 raises OverflowError on a forged/huge itemsize, which escapes this handler. WHY: the two NUMPY_RAW entry points at deserialize() lines 523 and 532 call _deserialize_numpy via a bare return with no surrounding try/except, so a forged cache entry leaks a raw OverflowError out of deserialize() instead of the contracted SerializationError — the PR added OverflowError to PAYLOAD_DECODE_ERRORS for exactly this reason but did not update this inner catch. HOW: add OverflowError to the except tuple.

except (ValueError, TypeError, IndexError, OverflowError) as e:
    # TypeError: np.frombuffer on a forged dtype string; OverflowError: forged itemsize past C long;
    # UnicodeDecodeError is a ValueError.
    raise SerializationError(f"Failed to deserialize NumPy array: {e}") from e

Prompt for LLM

File src/cachekit/serializers/auto_serializer.py:

Line 743 to 745:

WHAT: _deserialize_numpy catches only (ValueError, TypeError, IndexError), but np.frombuffer with the attacker-controlled dtype_str decoded at line 721 raises OverflowError on a forged/huge itemsize, which escapes this handler. WHY: the two NUMPY_RAW entry points at deserialize() lines 523 and 532 call _deserialize_numpy via a bare return with no surrounding try/except, so a forged cache entry leaks a raw OverflowError out of deserialize() instead of the contracted SerializationError — the PR added OverflowError to PAYLOAD_DECODE_ERRORS for exactly this reason but did not update this inner catch. HOW: add OverflowError to the except tuple.

Suggested Code:

        except (ValueError, TypeError, IndexError, OverflowError) as e:
            # TypeError: np.frombuffer on a forged dtype string; OverflowError: forged itemsize past C long;
            # UnicodeDecodeError is a ValueError.
            raise SerializationError(f"Failed to deserialize NumPy array: {e}") from e

_{Talk to Kody by mentioning @kody}

_{Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.}

​

​

[kodus-27b]

WHAT: _deserialize_numpy catches only (ValueError, TypeError, IndexError) but np.dtype/np.frombuffer/reshape on a forged NUMPY_RAW entry (e.g. a void dtype string with an oversized itemsize, or an overflowing element count) raises OverflowError, which this PR deliberately added to PAYLOAD_DECODE_ERRORS as a forged-payload signal. WHY: _deserialize_numpy is invoked directly at lines 523 and 532 with no surrounding handler that normalizes PAYLOAD_DECODE_ERRORS, so an uncaught OverflowError propagates out of deserialize() as a raw exception instead of SerializationError, breaking the serializer's exception contract for forged cache entries on the NUMPY_RAW path (only the msgpack-fallback path at 641-642 masks it). HOW: include OverflowError in the inner catch so every forged-numpy failure fails closed as SerializationError regardless of entry point.

except (ValueError, TypeError, IndexError, OverflowError) as e:
    # TypeError: np.frombuffer on a forged dtype string; OverflowError: forged itemsize/count;
    # UnicodeDecodeError is a ValueError.
    raise SerializationError(f"Failed to deserialize NumPy array: {e}") from e

Prompt for LLM

File src/cachekit/serializers/auto_serializer.py:

Line 743 to 745:

WHAT: `_deserialize_numpy` catches only `(ValueError, TypeError, IndexError)` but `np.dtype`/`np.frombuffer`/`reshape` on a forged NUMPY_RAW entry (e.g. a void dtype string with an oversized itemsize, or an overflowing element count) raises `OverflowError`, which this PR deliberately added to `PAYLOAD_DECODE_ERRORS` as a forged-payload signal. WHY: `_deserialize_numpy` is invoked directly at lines 523 and 532 with no surrounding handler that normalizes `PAYLOAD_DECODE_ERRORS`, so an uncaught `OverflowError` propagates out of `deserialize()` as a raw exception instead of `SerializationError`, breaking the serializer's exception contract for forged cache entries on the NUMPY_RAW path (only the msgpack-fallback path at 641-642 masks it). HOW: include `OverflowError` in the inner catch so every forged-numpy failure fails closed as `SerializationError` regardless of entry point.

Suggested Code:

        except (ValueError, TypeError, IndexError, OverflowError) as e:
            # TypeError: np.frombuffer on a forged dtype string; OverflowError: forged itemsize/count;
            # UnicodeDecodeError is a ValueError.
            raise SerializationError(f"Failed to deserialize NumPy array: {e}") from e

_{Talk to Kody by mentioning @kody}

_{Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.}

​

​