Skip to content

fix: translate RecursionError to StackError in fallback Unpacker.skip() - #727

Merged
methane merged 1 commit into
msgpack:mainfrom
hdimer:fallback-skip-stackerror
Aug 17, 2026
Merged

fix: translate RecursionError to StackError in fallback Unpacker.skip()#727
methane merged 1 commit into
msgpack:mainfrom
hdimer:fallback-skip-stackerror

Conversation

@hdimer

@hdimer hdimer commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Finding #6 of #683.

Unpacker.skip() is the one unpack entry point in fallback.py that lets a RecursionError escape. unpack() and __next__() both translate it, skip() does not:

def skip(self):
    self._unpack(EX_SKIP)        # no guard
    self._consume()

def unpack(self):
    try:
        ret = self._unpack(EX_CONSTRUCT)
    except RecursionError:
        raise StackError
    self._consume()
    return ret

so on deeply nested input the two disagree:

>>> data = b"\x91" * 3000  # nested fixarray(len=1)
>>> u = Unpacker(); u.feed(data); u.unpack()
msgpack.exceptions.StackError
>>> u = Unpacker(); u.feed(data); u.skip()
RecursionError: maximum recursion depth exceeded

The C extension has no such gap: its skip() goes through the same Unpacker._unpack as unpack(), which raises StackError on ret == -3 (_unpacker.pyx:503). So what a caller sees on this path depends on which implementation is loaded.

StackError subclasses ValueError, so code that guards against adversarial nesting with except StackError or except ValueError is unprotected on the skip path under the pure-Python implementation, which is the configuration where the recursion is reachable in the first place.

The fix

Wrap the body the way unpack() already does, four lines away:

def skip(self):
    try:
        self._unpack(EX_SKIP)
    except RecursionError:
        raise StackError
    self._consume()

self._consume() stays outside the try, matching unpack(), so the buffer state after a failure is the same on both paths. I checked that: after the failure tell() is 0 and a retry raises StackError again, identical to unpack() on the same input.

The except cannot mask a user error here. Under EX_SKIP, _unpack calls no user-supplied hook: the array and map branches recurse with EX_SKIP without touching list_hook / object_hook / object_pairs_hook, and the if execute == EX_SKIP: return short-circuits before ext_hook. The only recursion inside the try is msgpack's own, which makes this guard strictly narrower than the two it copies.

read_array_header() / read_map_header() do not need the same treatment: _unpack returns the header count before it reaches the recursion, so they are unaffected by nesting depth.

Tests

The assertion sits next to the existing one-shot StackError case in test_invalidvalue, since it closes the one-shot vs. streaming gap on the same payload. It fails on main with RecursionError: maximum recursion depth exceeded and passes with the change. It is green under the C extension too, where the same input trips the embed stack limit, so it also pins the parity rather than just the fallback.

Verified on CPython 3.14: full suite green in both the C-extension and MSGPACK_PUREPYTHON=1 configurations, ruff check and ruff format clean.

Disclosure: I used an AI assistant while investigating and writing this patch. I reproduced the divergence myself, confirmed the fix against both builds, and I stand behind the change.

skip() was the only unpack entry point in fallback.py that let a
RecursionError escape on deeply nested input; unpack() and __next__()
both re-raise it as StackError, and the C extension raises StackError
on this path too. Callers guarding against adversarial nesting with
except StackError/ValueError were unprotected when skipping.

Wrap the body the same way unpack() does, leaving _consume() outside
the try so the post-failure buffer state matches.
@hdimer
hdimer marked this pull request as ready for review August 16, 2026 23:20
@methane
methane merged commit 2e4be09 into msgpack:main Aug 17, 2026
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants