Skip to content

Commit c9201a1

Browse files
fix: gate ReadBuffer public API on poison flag
Issue 026 introduced the poison flag so that once a parse error desynchronizes the stream, subsequent operations fail fast with ProtocolError. The check was wired into MessageDecoder.decode() / decode_continuation() but NOT into ReadBuffer's own public API. A caller holding a raw ReadBuffer (exported from the dqlitewire package) could keep calling feed()/read_message()/peek_header()/... after poisoning and re-desynchronize the stream silently. Gate every mutating/consuming public method on ReadBuffer on self._check_poisoned(): feed, read_message, skip_message, read_bytes, peek_bytes, peek_header. Observers (has_message, available, is_poisoned, is_skipping) and recovery primitives (clear, reset, poison) remain total and do not raise. Two existing TestDecoderPoisonedState tests pushed bytes through feed() after a poison to assert that the sticky state survived. That contract is now stricter: feed() itself refuses, which surfaces the poison earlier. Update the tests to assert ProtocolError on the feed() call and still require reset() for recovery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent ec2bf58 commit c9201a1

3 files changed

Lines changed: 78 additions & 3 deletions

File tree

src/dqlitewire/buffer.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,11 @@ def feed(self, data: bytes) -> None:
9292
If an oversized message is being skipped (after skip_message() returned
9393
False), incoming bytes are silently discarded until the full oversized
9494
message has been consumed.
95+
96+
Raises ``ProtocolError`` if the buffer is poisoned — callers must
97+
``reset()`` (or ``clear()``) before feeding further data.
9598
"""
99+
self._check_poisoned()
96100
if self._skip_remaining > 0:
97101
discard = min(len(data), self._skip_remaining)
98102
data = data[discard:]
@@ -146,7 +150,10 @@ def peek_header(self) -> tuple[int, int, int] | None:
146150
``max_message_size`` guard the caller is presumably relying on. The
147151
error message and exception type match ``read_message()`` so the
148152
consume-side recovery path (``skip_message()``) applies the same way.
153+
154+
Raises ``ProtocolError`` if the buffer is poisoned.
149155
"""
156+
self._check_poisoned()
150157
available = len(self._data) - self._pos
151158

152159
if available < HEADER_SIZE:
@@ -174,7 +181,9 @@ def read_message(self) -> bytes | None:
174181
Returns the message data (including header) or None if not enough data.
175182
Raises ``DecodeError`` if the next buffered header claims a message
176183
larger than ``max_message_size``. Use ``skip_message()`` to recover.
184+
Raises ``ProtocolError`` if the buffer is poisoned.
177185
"""
186+
self._check_poisoned()
178187
available = len(self._data) - self._pos
179188
if available < HEADER_SIZE:
180189
return None
@@ -212,7 +221,10 @@ def skip_message(self) -> bool:
212221
Returns True if a message was fully skipped, False if not enough data
213222
for a header, a normal-sized message is still incomplete, or an
214223
oversized message skip is still in progress (check ``is_skipping``).
224+
225+
Raises ``ProtocolError`` if the buffer is poisoned.
215226
"""
227+
self._check_poisoned()
216228
available = len(self._data) - self._pos
217229
if available < HEADER_SIZE:
218230
return False
@@ -244,7 +256,9 @@ def read_bytes(self, n: int) -> bytes | None:
244256
"""Read exactly n bytes from the buffer.
245257
246258
Returns None if not enough data available.
259+
Raises ``ProtocolError`` if the buffer is poisoned.
247260
"""
261+
self._check_poisoned()
248262
available = len(self._data) - self._pos
249263
if available < n:
250264
return None
@@ -260,7 +274,10 @@ def peek_bytes(self, n: int) -> bytes | None:
260274
Returns None if fewer than n bytes are available. Symmetric with
261275
``read_bytes(n)`` but non-consuming — use this when you need to
262276
validate the bytes before deciding whether to consume them.
277+
278+
Raises ``ProtocolError`` if the buffer is poisoned.
263279
"""
280+
self._check_poisoned()
264281
available = len(self._data) - self._pos
265282
if available < n:
266283
return None

tests/test_buffer.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,57 @@ def test_clear_also_unpoisons(self) -> None:
216216
buf.feed(b"\x01\x00\x00\x00\x00\x00\x00\x00")
217217
assert buf.available() == 8
218218

219+
def test_public_api_honors_poison(self) -> None:
220+
"""Regression for issue 038.
221+
222+
Issue 026 introduced the poison flag so that once a decode
223+
error has desynchronized the stream, subsequent operations
224+
fail fast with ``ProtocolError`` instead of silently reading
225+
from an unknown offset. The check was wired into
226+
``MessageDecoder.decode`` / ``decode_continuation`` but NOT
227+
into ``ReadBuffer``'s own public API. A caller holding a raw
228+
``ReadBuffer`` (exported from ``dqlitewire`` since
229+
``__init__.py``) could keep calling ``feed()``, ``read_message()``,
230+
etc., after a poison was set and re-desynchronize the stream
231+
silently.
232+
233+
Every mutating/consuming public method on ``ReadBuffer`` must
234+
raise ``ProtocolError`` when poisoned. Pure observers
235+
(``has_message``, ``available``, ``is_poisoned``, ``is_skipping``)
236+
and recovery primitives (``clear``, ``reset``, ``poison``)
237+
must remain callable.
238+
"""
239+
import pytest
240+
241+
from dqlitewire.exceptions import DecodeError, ProtocolError
242+
243+
buf = ReadBuffer()
244+
buf.poison(DecodeError("original cause"))
245+
246+
cases: list[tuple[str, object]] = [
247+
("feed", lambda: buf.feed(b"x" * 8)),
248+
("read_message", lambda: buf.read_message()),
249+
("skip_message", lambda: buf.skip_message()),
250+
("read_bytes", lambda: buf.read_bytes(4)),
251+
("peek_bytes", lambda: buf.peek_bytes(4)),
252+
("peek_header", lambda: buf.peek_header()),
253+
]
254+
for name, call in cases:
255+
with pytest.raises(ProtocolError, match="poisoned") as ei:
256+
call() # type: ignore[operator]
257+
assert isinstance(ei.value.__cause__, DecodeError), name
258+
259+
# Observers must remain callable on a poisoned buffer.
260+
_ = buf.available()
261+
_ = buf.has_message()
262+
_ = buf.is_poisoned
263+
_ = buf.is_skipping
264+
265+
# Recovery path must still work: reset() unpoisons.
266+
buf.reset()
267+
assert not buf.is_poisoned
268+
buf.feed(b"\x00" * 8)
269+
219270
def test_has_message_is_total_on_oversized(self) -> None:
220271
"""has_message() must not raise — oversized headers surface at consume time.
221272

tests/test_codec.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,8 +1015,11 @@ def test_decode_poisons_on_unknown_message_type(self) -> None:
10151015
assert decoder.is_poisoned is True
10161016

10171017
# Subsequent operations fail fast with a ProtocolError from poisoning.
1018-
# decode() must raise regardless of what is in the buffer.
1019-
decoder.feed(struct.pack("<IBBH", 0, 0xFE, 0, 0))
1018+
# Per issue 038, every mutating/consuming entry point on the buffer
1019+
# and decoder — including feed() — refuses to run on a poisoned
1020+
# buffer. Recovery requires reset().
1021+
with pytest.raises(ProtocolError, match="poisoned"):
1022+
decoder.feed(struct.pack("<IBBH", 0, 0xFE, 0, 0))
10201023
with pytest.raises(ProtocolError, match="poisoned"):
10211024
decoder.decode()
10221025

@@ -1108,8 +1111,12 @@ def boom(_data: bytes) -> object:
11081111
# Subsequent decode() must raise poisoned, even though decode_bytes
11091112
# is monkey-patched. Restore the real decode_bytes first to make
11101113
# sure the poison check fires before any decode_bytes call.
1114+
# Per issue 038, feed() on a poisoned buffer also raises, so we
1115+
# cannot push more bytes through without a reset — the poison
1116+
# check fires directly on decode().
11111117
del decoder.decode_bytes # type: ignore[attr-defined]
1112-
decoder.feed(msg)
1118+
with pytest.raises(ProtocolError, match="poisoned"):
1119+
decoder.feed(msg)
11131120
with pytest.raises(ProtocolError, match="poisoned"):
11141121
decoder.decode()
11151122

0 commit comments

Comments
 (0)