diff --git a/CHANGES/13222.bugfix.rst b/CHANGES/13222.bugfix.rst new file mode 100644 index 00000000000..dcdda17051c --- /dev/null +++ b/CHANGES/13222.bugfix.rst @@ -0,0 +1 @@ +Fixed error message construction in the C HTTP parser -- by :user:`bdraco`. diff --git a/aiohttp/_http_parser.pyx b/aiohttp/_http_parser.pyx index 19273ac31f7..73e73cc6773 100644 --- a/aiohttp/_http_parser.pyx +++ b/aiohttp/_http_parser.pyx @@ -677,11 +677,12 @@ cdef class HttpParser: ex = self._last_error self._last_error = None else: - after = cparser.llhttp_get_error_pos(self._cparser) - before = data[:after - base] - after_b = after.split(b"\r\n", 1)[0] + error_pos = cparser.llhttp_get_error_pos(self._cparser) + error_off = error_pos - base + before = data[:error_off] + after = data[error_off:].split(b"\r\n", 1)[0] before = before.rsplit(b"\r\n", 1)[-1] - data = before + after_b + data = before + after pointer = " " * (len(repr(before))-1) + "^" ex = parser_error_from_errno(self._cparser, data, pointer) self._payload = None diff --git a/tests/test_http_parser.py b/tests/test_http_parser.py index b37c71f397f..71e9c783ea0 100644 --- a/tests/test_http_parser.py +++ b/tests/test_http_parser.py @@ -2124,6 +2124,99 @@ async def test_http_response_parser_bad_chunked_strict_c() -> None: response.feed_data(text) +# The C parser builds its error message from a snippet of the fed buffer. That +# snippet must be sliced out of the bounded ``data`` bytes object; materializing the +# raw llhttp error-position pointer instead runs strlen past the end of the buffer. +# These tests pin the snippet to bytes that lie inside the fed input, and cover error +# positions right at the buffer end where the over-read used to happen. + +_CHUNKED_RESP_HDR = b"HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n" + +# (malformed body appended to a chunked response header, expected error snippet, +# llhttp error reason) +_BAD_CHUNKED_RESPONSES = [ + (b"0\rX\r\n\r\n", b"0\rX", "Expected LF after chunk size"), + (b"5\r\nhello\rX\r\n0\r\n\r\n", b"hello\rX", "Expected LF after chunk data"), + (b"5\r\nhelloXY\r\n0\r\n\r\n", b"helloXY", "Expected LF after chunk data"), + (b"1\r\nA\rB\r\n0\r\n\r\n", b"A\rB", "Expected LF after chunk data"), + (b"0_2e\r\n\r\n", b"0_2e", "Invalid character in chunk size"), +] + +# Same, but the buffer ends exactly at the offending byte so the error position sits +# at the end of the input with no trailing CRLF to bound the old strlen-based slice. +_BAD_CHUNKED_RESPONSES_AT_END = [ + (b"0\rX", b"0\rX", "Expected LF after chunk size"), + (b"5\r\nhello\rX", b"hello\rX", "Expected LF after chunk data"), + (b"0_", b"0_", "Invalid character in chunk size"), +] + +_BAD_REQUESTS_AT_END = [ + (b"GET / HTTP/1.1\r\nHost: x\n", b"Host: x\n"), + (b"POST / HTTP/1.1\r\nHost: x\r\nA: b\x01", b"A: b\x01"), +] + + +@pytest.mark.skipif(NO_EXTENSIONS, reason="Only tests C parser.") +@pytest.mark.parametrize( + ("body", "snippet", "reason"), + _BAD_CHUNKED_RESPONSES + _BAD_CHUNKED_RESPONSES_AT_END, +) +async def test_c_parser_error_snippet_bounded_response( + body: bytes, snippet: bytes, reason: str +) -> None: + loop = asyncio.get_running_loop() + protocol = ResponseHandler(loop) + response = HttpResponseParserC( + protocol, loop, 2**16, max_line_size=8190, max_field_size=8190 + ) + protocol._parser = response + text = _CHUNKED_RESP_HDR + body + assert snippet in text + match = f"{re.escape(reason)}[\\s\\S]*{re.escape(repr(snippet))}" + with pytest.raises(http_exceptions.BadHttpMessage, match=match): + response.feed_data(text) + + +@pytest.mark.skipif(NO_EXTENSIONS, reason="Only tests C parser.") +@pytest.mark.parametrize(("text", "snippet"), _BAD_REQUESTS_AT_END) +def test_c_parser_error_snippet_at_buffer_end_request( + event_loop: asyncio.AbstractEventLoop, + server: Server[Request], + text: bytes, + snippet: bytes, +) -> None: + protocol = RequestHandler(server, loop=event_loop) + parser = HttpRequestParserC( + protocol, event_loop, 2**16, max_line_size=8190, max_field_size=8190 + ) + protocol._parser = parser + assert snippet in text + with pytest.raises(http_exceptions.BadHttpMessage, match=re.escape(repr(snippet))): + parser.feed_data(text) + + +@pytest.mark.skipif(NO_EXTENSIONS, reason="Only tests C parser.") +@pytest.mark.parametrize("split", [1, 2, 3, 5]) +@pytest.mark.parametrize(("body", "snippet", "reason"), _BAD_CHUNKED_RESPONSES) +async def test_c_parser_error_snippet_fragmented_response( + body: bytes, snippet: bytes, reason: str, split: int +) -> None: + # Feed the malformed body in two pieces so the second feed runs through the + # tail-prepend path before the error is reported; the parser must report a clean + # error rather than reading past the buffer. The snippet offset shifts with the + # split, so pin the stable llhttp reason instead. + loop = asyncio.get_running_loop() + protocol = ResponseHandler(loop) + response = HttpResponseParserC( + protocol, loop, 2**16, max_line_size=8190, max_field_size=8190 + ) + protocol._parser = response + response.feed_data(_CHUNKED_RESP_HDR) + with pytest.raises(http_exceptions.BadHttpMessage, match=re.escape(reason)): + response.feed_data(body[:split]) + response.feed_data(body[split:]) + + async def test_http_response_parser_notchunked( response: HttpResponseParser, ) -> None: