Skip to content

Commit c14af4b

Browse files
authored
Reject negative lengths in Unpacker.read_bytes (#760)
`Unpacker.read_bytes()` does not reject negative lengths. The C extension raises an internal `SystemError` from `PyBytes_FromStringAndSize`; the Python implementation accepts the count and can return an unintended slice of buffered data. Raise `ValueError` for a negative byte count before reading or updating the stream position in either implementation. Four regression cases cover feed-based and file-based unpackers with `-1` and a larger negative count. They also check that the failed call leaves the position unchanged and that zero-length reads and subsequent reads/unpacking still work. On Windows/Python 3.13, all four cases fail with each unchanged implementation. With the fix, the complete C-extension suite has 164 passed and 1 skipped; the Python suite has 161 passed and 4 skipped. Both retain three existing unknown-marker warnings for `thread_unsafe` when the free-threaded test plugin is absent. Repository-wide Ruff lint and format checks pass. Other OS/Python and free-threaded matrices were not run locally. Related: #742 validates negative sizes in the packer header APIs; this change covers the unpacker's raw-byte reader. #752 concerns short feed-based reads, which is a separate case. AI assistance: OpenAI Codex investigated, generated and self-reviewed the patch, and ran the local checks. No separate human review is claimed.
1 parent a284599 commit c14af4b

3 files changed

Lines changed: 22 additions & 0 deletions

File tree

‎msgpack/_unpacker.pyx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,8 @@ cdef class Unpacker:
535535
raise RuntimeError("Unpacker.read_bytes() cannot be called while unpacking is in progress")
536536
if self.ctx.execute != NULL:
537537
raise ValueError("Cannot switch unpacking methods while an object is incomplete")
538+
if nbytes < 0:
539+
raise ValueError("nbytes must be non-negative")
538540
nread = min(self.buf_tail - self.buf_head, nbytes)
539541
ret = PyBytes_FromStringAndSize(self.buf + self.buf_head, nread)
540542
self.buf_head += nread

‎msgpack/fallback.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,8 @@ def _get_extradata(self):
354354
return self._buffer[self._buff_i :]
355355

356356
def read_bytes(self, n):
357+
if n < 0:
358+
raise ValueError("nbytes must be non-negative")
357359
ret = self._read(n, raise_outofdata=False)
358360
self._consume()
359361
# The C extension returns bytes. A bytearray slice cannot be a dict key.

‎test/test_unpack.py‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,24 @@ def test_unpacker_tell_read_bytes():
9999
assert unpacker.read_bytes(n) == raw
100100

101101

102+
@mark.parametrize("from_file", [False, True])
103+
@mark.parametrize("nbytes", [-1, -(2**31)])
104+
def test_unpacker_read_bytes_negative(from_file, nbytes):
105+
packed = packb([1, 2])
106+
unpacker = Unpacker(BytesIO(packed)) if from_file else Unpacker()
107+
if not from_file:
108+
unpacker.feed(packed)
109+
assert unpacker.read_array_header() == 2
110+
111+
with raises(ValueError, match="non-negative"):
112+
unpacker.read_bytes(nbytes)
113+
114+
assert unpacker.tell() == 1
115+
assert unpacker.read_bytes(0) == b""
116+
assert unpacker.read_bytes(1) == b"\x01"
117+
assert unpacker.unpack() == 2
118+
119+
102120
@mark.skipif(
103121
Unpacker.__module__ == "msgpack.fallback",
104122
reason="specific to C extension reinit leak",

0 commit comments

Comments
 (0)