Skip to content

Commit 9d17383

Browse files
fix: close signal window between decode and continuation flag set
decode() previously set _continuation_expected outside the try/except BaseException block. An asynchronously-delivered KeyboardInterrupt landing between decode_bytes success and the flag store consumed the bytes without poisoning the buffer; the next decode() would silently mis-frame the continuation frame as a top-level message. Move the flag assignment inside the try block so any exception in that window routes through poison(). The residual window between flag-set and `return msg` is harmless: either the message returns cleanly or the caller must call decode_continuation() to consume the expected continuation frame. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 64247d0 commit 9d17383

2 files changed

Lines changed: 69 additions & 3 deletions

File tree

src/dqlitewire/codec.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,15 @@ def decode(self) -> Message | None:
383383
# etc., and all of them mean the stream is desynchronized.
384384
try:
385385
msg = self.decode_bytes(data)
386+
# The flag store MUST be inside the try block: the bytes
387+
# have been consumed, so any asynchronously-delivered
388+
# exception here (KeyboardInterrupt, PyErr_SetAsyncExc)
389+
# between the successful decode and the flag store would
390+
# otherwise leave the stream desynchronized without
391+
# poisoning — the next ``decode()`` would mis-frame the
392+
# continuation frame as a top-level message (issue 233).
393+
if isinstance(msg, RowsResponse) and msg.has_more:
394+
self._continuation_expected = True
386395
except BaseException as e:
387396
# poison() stores Exception | None; wrap non-Exception
388397
# BaseException subclasses so the poison cause is still a
@@ -394,9 +403,6 @@ def decode(self) -> Message | None:
394403
)
395404
raise
396405

397-
if isinstance(msg, RowsResponse) and msg.has_more:
398-
self._continuation_expected = True
399-
400406
return msg
401407

402408
def decode_bytes(self, data: bytes) -> Message:

tests/test_codec_signal_safety.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,30 @@ def tracer(frame: FrameType, event: str, arg: object) -> Any:
7373
return tracer
7474

7575

76+
def _tracer_raising_in_after_call(
77+
func_name: str,
78+
callee_name: str,
79+
exc: BaseException = _DEFAULT_EXC,
80+
) -> Any:
81+
"""Raise ``exc`` on the first line event in ``func_name`` AFTER a
82+
call to ``callee_name`` has returned. Used to target the window
83+
between a sub-call and the post-call statements in the caller.
84+
"""
85+
state = {"saw_return": False, "raised": False}
86+
87+
def tracer(frame: FrameType, event: str, arg: object) -> Any:
88+
if state["raised"]:
89+
return tracer
90+
if event == "return" and frame.f_code.co_name == callee_name:
91+
state["saw_return"] = True
92+
elif event == "line" and frame.f_code.co_name == func_name and state["saw_return"]:
93+
state["raised"] = True
94+
raise exc
95+
return tracer
96+
97+
return tracer
98+
99+
76100
class TestDecodeSignalSafety:
77101
"""Regression tests for issue 045 (consumed-but-unpoisoned window
78102
in ``decode()`` / ``decode_continuation()``).
@@ -122,6 +146,42 @@ def test_decode_does_not_poison_on_oversized_header(self) -> None:
122146

123147
assert not dec.is_poisoned
124148

149+
def test_keyboard_interrupt_between_parse_and_flag_set_poisons(self) -> None:
150+
"""233: a KeyboardInterrupt delivered in ``decode()`` AFTER
151+
``decode_bytes`` returns but BEFORE ``_continuation_expected``
152+
is set used to propagate without poisoning, because the flag
153+
assignment lived outside the ``except BaseException`` block.
154+
The next ``decode()`` call would then read the continuation
155+
frame as a top-level message (silent stream desync).
156+
157+
The fix moves the flag store inside the try/except block, so
158+
any exception in that window poisons the buffer.
159+
"""
160+
# Feed any valid message — a DbResponse is fine because the
161+
# flag-check line runs before the isinstance() short-circuits.
162+
dec = MessageDecoder(is_request=False)
163+
dec.feed(_make_db(1) + _make_db(2))
164+
165+
tracer = _tracer_raising_in_after_call("decode", "decode_bytes")
166+
167+
sys.settrace(tracer)
168+
try:
169+
with contextlib.suppress(KeyboardInterrupt):
170+
dec.decode()
171+
finally:
172+
sys.settrace(None)
173+
174+
assert dec.is_poisoned, (
175+
"decode() must poison when an interrupt lands after "
176+
"decode_bytes returns but before the flag store"
177+
)
178+
179+
# The retry must fail fast with ProtocolError rather than
180+
# silently returning the next message from a desynchronized
181+
# stream.
182+
with pytest.raises(ProtocolError, match="poisoned"):
183+
dec.decode()
184+
125185
def test_keyboard_interrupt_in_decode_continuation_poisons(self) -> None:
126186
"""Same hazard as decode(): decode_continuation() also needs
127187
BaseException handling.

0 commit comments

Comments
 (0)