Skip to content

Commit 97c74e6

Browse files
fix: wrap ReadBuffer.feed() in BaseException poison guard
`feed()` used to mutate `_skip_remaining` and `_data` without any try/except wrapper. A BaseException leaking out between `_maybe_compact()` returning and `self._data.extend(data)` — a reachable RESUME-delivery point in CPython 3.11+ — would leave the buffer silently torn: fed bytes dropped from the caller's frame, compact run but extend not, `is_poisoned` still False, and the next `feed()` call appending post-gap bytes into a desynchronized buffer. Wrap the mutation block in `try/except BaseException: poison; raise` matching the `_maybe_compact()` template from issue 037. The oversized-buffer `DecodeError` size check stays OUTSIDE the try block to preserve its non-poisoning recoverable-via-reset contract; the check now accounts for the would-be skip-discard so a buffer in skip mode can still legitimately absorb incoming bytes whose post-discard remainder fits within max_message_size. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e45c920 commit 97c74e6

2 files changed

Lines changed: 157 additions & 10 deletions

File tree

src/dqlitewire/buffer.py

Lines changed: 46 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -95,19 +95,55 @@ def feed(self, data: bytes) -> None:
9595
9696
Raises ``ProtocolError`` if the buffer is poisoned — callers must
9797
``reset()`` (or ``clear()``) before feeding further data.
98+
Raises ``DecodeError`` (non-poisoning, recoverable via
99+
``reset()``/``clear()``) if the resulting buffer size would
100+
exceed ``max_message_size``.
101+
102+
Signal-safety note (issue 048): the mutation block below is
103+
wrapped in ``try/except BaseException`` so that any async
104+
exception leaking out — most notably between the
105+
``_maybe_compact()`` return and the subsequent
106+
``_data.extend(data)``, a reachable RESUME delivery point in
107+
3.11+ — poisons the buffer. The ``DecodeError`` size check
108+
is deliberately kept OUTSIDE the try block so that the
109+
documented "recoverable oversized-buffer" contract is
110+
preserved.
98111
"""
99112
self._check_poisoned()
113+
# Size check BEFORE any mutation: the DecodeError raised here
114+
# must NOT poison the buffer, because its caller-recovery
115+
# contract is "drain or reset() and continue". The check
116+
# accounts for the would-be skip-discard so that a buffer
117+
# in skip mode can legitimately accept incoming bytes whose
118+
# post-discard remainder fits within max_message_size.
100119
if self._skip_remaining > 0:
101-
discard = min(len(data), self._skip_remaining)
102-
data = data[discard:]
103-
self._skip_remaining -= discard
104-
if not data:
105-
return
106-
self._maybe_compact()
107-
unconsumed = len(self._data) - self._pos + len(data)
108-
if unconsumed > self._max_message_size:
109-
raise DecodeError(f"Buffer size {unconsumed} exceeds maximum {self._max_message_size}")
110-
self._data.extend(data)
120+
effective_len = max(0, len(data) - self._skip_remaining)
121+
else:
122+
effective_len = len(data)
123+
projected = len(self._data) - self._pos + effective_len
124+
if projected > self._max_message_size:
125+
raise DecodeError(f"Buffer size {projected} exceeds maximum {self._max_message_size}")
126+
try:
127+
if self._skip_remaining > 0:
128+
discard = min(len(data), self._skip_remaining)
129+
data = data[discard:]
130+
self._skip_remaining -= discard
131+
if not data:
132+
return
133+
self._maybe_compact()
134+
self._data.extend(data)
135+
except BaseException as e:
136+
# Any torn state here is unrecoverable — subsequent
137+
# callers MUST see poison rather than silently reading
138+
# from a buffer with a gap. ``poison()`` is first-error-
139+
# wins, so if ``_maybe_compact`` already poisoned with
140+
# its own cause, that original cause is preserved.
141+
if self._poisoned is None:
142+
if isinstance(e, Exception):
143+
self._poisoned = e
144+
else:
145+
self._poisoned = RuntimeError(f"feed interrupted: {type(e).__name__}")
146+
raise
111147

112148
def has_message(self) -> bool:
113149
"""Check if a complete message is available.

tests/test_buffer_signal_safety.py

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,3 +212,114 @@ def test_decode_error_is_still_recoverable_without_poison() -> None:
212212
buf.feed(b"\x00" * 16)
213213
assert not buf.is_poisoned
214214
_ = DecodeError # referenced for completeness
215+
216+
217+
class TestFeedSignalSafety:
218+
"""Regression tests for issue 048.
219+
220+
``ReadBuffer.feed`` used to mutate state (``_skip_remaining``,
221+
``_data``) without any try/except wrapper. A BaseException leaking
222+
out of any intermediate step — most notably between
223+
``_maybe_compact()`` returning and ``self._data.extend(data)``,
224+
where CPython's RESUME opcode IS a real async-exception delivery
225+
point — would leave the buffer in a silently inconsistent state:
226+
fed bytes dropped from the caller's stack frame, compact having
227+
run but extend having not. ``is_poisoned`` would remain ``False``
228+
and the next ``feed()`` call would silently append to a buffer
229+
with a gap.
230+
231+
The fix wraps the mutation block in ``try/except BaseException``
232+
analogous to ``_maybe_compact``'s own fix from issue 037, while
233+
leaving the oversized-``DecodeError`` size check OUTSIDE the try
234+
block so the non-poisoning "recoverable via drain/reset" contract
235+
is preserved.
236+
"""
237+
238+
def test_torn_feed_extend_leaves_buffer_poisoned(self) -> None:
239+
"""A BaseException raised between ``_maybe_compact`` and
240+
``_data.extend`` must poison the buffer so the next operation
241+
fails fast instead of silently returning partial data.
242+
"""
243+
buf = ReadBuffer()
244+
# Give the buffer enough prior data that _maybe_compact is a
245+
# no-op (we want the hazard to be specifically the extend
246+
# step, not any torn compact). _pos below 4096 → compact
247+
# returns early.
248+
buf.feed(b"\x00" * 32)
249+
buf.read_bytes(8)
250+
assert buf._pos == 8
251+
252+
# Inject the async exception on the extend line. The tracer
253+
# runs the line event before bytecode on that line executes,
254+
# so extend never runs — exactly the shape that a real RESUME
255+
# after _maybe_compact() returns can produce.
256+
tracer = _raise_on_source_match("feed", "self._data.extend(data)")
257+
258+
sys.settrace(tracer)
259+
try:
260+
with contextlib.suppress(KeyboardInterrupt):
261+
buf.feed(b"NEW-DATA")
262+
finally:
263+
sys.settrace(None)
264+
265+
# The fed bytes are lost (they only lived in the caller's
266+
# frame). The buffer MUST be poisoned so the next operation
267+
# raises ProtocolError rather than continuing on a torn
268+
# state.
269+
assert buf.is_poisoned, "feed() must poison when a BaseException escapes its mutation block"
270+
with pytest.raises(ProtocolError, match="poisoned"):
271+
buf.read_message()
272+
273+
def test_torn_feed_poison_cause_is_recorded(self) -> None:
274+
"""The poison cause must be a real exception that surfaces in
275+
the ProtocolError ``__cause__`` chain, matching the
276+
well-formed-poison contract from issue 037.
277+
"""
278+
buf = ReadBuffer()
279+
buf.feed(b"\x00" * 32)
280+
buf.read_bytes(8)
281+
282+
sentinel = RuntimeError("injected torn extend")
283+
tracer = _raise_on_source_match("feed", "self._data.extend(data)", exc=sentinel)
284+
285+
sys.settrace(tracer)
286+
try:
287+
with contextlib.suppress(RuntimeError):
288+
buf.feed(b"NEW-DATA")
289+
finally:
290+
sys.settrace(None)
291+
292+
assert buf.is_poisoned
293+
with pytest.raises(ProtocolError) as ei:
294+
buf._check_poisoned()
295+
cause = ei.value.__cause__
296+
assert cause is sentinel or (
297+
isinstance(cause, Exception) and "injected torn extend" in str(cause)
298+
)
299+
300+
def test_feed_decode_error_is_still_non_poisoning(self) -> None:
301+
"""Counter-test: the oversized-buffer ``DecodeError`` guard
302+
must remain non-poisoning, because its caller-recovery contract
303+
is "drain or reset and continue". The poison wrapper must not
304+
capture this specific error.
305+
"""
306+
buf = ReadBuffer(max_message_size=16)
307+
with pytest.raises(DecodeError):
308+
buf.feed(b"A" * 32)
309+
assert not buf.is_poisoned, (
310+
"oversized-buffer DecodeError must NOT poison the buffer; "
311+
"it is deliberately recoverable via reset()/clear()"
312+
)
313+
# And after reset the buffer is usable again.
314+
buf.reset()
315+
buf.feed(b"A" * 8)
316+
assert buf.available() == 8
317+
318+
def test_happy_path_feed_still_works(self) -> None:
319+
"""Sanity: without any interrupt, feed behaves normally."""
320+
buf = ReadBuffer()
321+
buf.feed(b"hello")
322+
buf.feed(b"world")
323+
assert not buf.is_poisoned
324+
assert buf.available() == 10
325+
assert buf.read_bytes(10) == b"helloworld"

0 commit comments

Comments
 (0)