From 6a5db75fe0e1d232d87ba38c4d749e3f6683266f Mon Sep 17 00:00:00 2001 From: Benoit Chesneau Date: Fri, 17 Jul 2026 01:24:17 +0200 Subject: [PATCH] Wait for the LF when a chunk-size line splits on the CR A chunked response failed with {error, invalid_chunk_size} when the CRLF ending a chunk-size line was split across two socket reads, leaving the buffer on a lone \r. read_size/3 now treats that as incomplete and waits for more data. te_chunked/2 also propagates the errors of read_size/3 and read_chunk/2 instead of crashing with a case_clause, and its spec now matches its real returns. Fixes #901 --- NEWS.md | 12 +++++++ src/hackney_http.erl | 22 ++++++++++--- test/hackney_http_tests.erl | 63 +++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index 32cbc0e0..132c0fa4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,17 @@ # NEWS +unreleased +---------- + +### Fixed + +- Chunked decoding no longer fails with `{error, invalid_chunk_size}` when + the CRLF terminating a chunk-size line is split across two socket reads + (buffer ending on a lone `\r`). The parser now waits for the `\n` (#901). +- A malformed chunk-size line or chunk terminator now fails cleanly with + `{error, invalid_chunk_size}` or `{error, poorly_formatted_chunked_size}` + instead of crashing the parser with a `case_clause` error. + 4.7.0 - 2026-07-17 ------------------ diff --git a/src/hackney_http.erl b/src/hackney_http.erl index d302766f..c603f302 100644 --- a/src/hackney_http.erl +++ b/src/hackney_http.erl @@ -493,9 +493,12 @@ content_decode(ContentDecode, Data, St) -> %% @doc Decode a stream of chunks. -spec te_chunked(binary(), any()) - -> more | {ok, binary(), {non_neg_integer(), non_neg_integer()}} - | {ok, binary(), binary(), {non_neg_integer(), non_neg_integer()}} - | {done, non_neg_integer(), binary()} | {error, badarg}. + -> done + | {chunk_done, binary()} + | {chunk_ok, binary(), binary()} + | more + | {more, {non_neg_integer(), non_neg_integer()}} + | {error, invalid_chunk_size | poorly_formatted_chunked_size}. te_chunked(<<>>, _) -> done; te_chunked(Data, _) -> @@ -507,10 +510,14 @@ te_chunked(Data, _) -> {ok, Chunk, Rest1} -> {chunk_ok, Chunk, Rest1}; eof -> - {more, {byte_size(Rest), Size}} + {more, {byte_size(Rest), Size}}; + {error, _} = Error -> + Error end; eof -> - more + more; + {error, _} = Error -> + Error end. %% @doc Decode an identity stream. @@ -535,6 +542,11 @@ read_size(Data) -> read_size(<<>>, _, _) -> eof; +%% The size line's CRLF terminator is split across two reads and the buffer +%% ends on the lone \r: wait for the \n instead of treating the line as +%% malformed (issue #901). +read_size(<<"\r">>, _, _) -> + eof; read_size(<<"\r\n", Rest/binary>>, Size, Len) when Len > 0 -> {ok, Size, Rest}; diff --git a/test/hackney_http_tests.erl b/test/hackney_http_tests.erl index a1fed2e4..4ebc8eab 100644 --- a/test/hackney_http_tests.erl +++ b/test/hackney_http_tests.erl @@ -67,6 +67,69 @@ parse_chunked_response_trailers_test() -> {more, P4} = hackney_http:execute(P3, <<"0\r\nFoo: ">>), ?assertEqual({done, <<>>}, hackney_http:execute(P4, <<"Bar\r\n\r\n">>)). +%% Issue #901: the CRLF terminating a chunk-size line is split across two +%% reads, leaving the buffer on a lone \r. The parser must wait for the \n +%% instead of failing with invalid_chunk_size. +parse_chunked_size_crlf_split_test() -> + ?assertEqual({done, <<"HELLO">>}, + run_chunked([<<"5\r">>, <<"\nHELLO\r\n0\r\n\r\n">>])). + +parse_chunked_last_chunk_crlf_split_test() -> + ?assertEqual({done, <<"HELLO">>}, + run_chunked([<<"5\r\nHELLO\r\n0\r">>, <<"\n\r\n">>])). + +%% Exhaustive read-boundary sweep: a two-chunk body with a trailer, split at +%% every possible byte position, must always decode to the same body. +parse_chunked_split_sweep_test() -> + Body = <<"5\r\nHELLO\r\n6\r\n WORLD\r\n0\r\nX-Trail: 1\r\n\r\n">>, + [?assertEqual({done, <<"HELLO WORLD">>}, + run_chunked([binary:part(Body, 0, I), + binary:part(Body, I, byte_size(Body) - I)])) + || I <- lists:seq(0, byte_size(Body))]. + +%% A genuinely malformed size line must fail cleanly, not crash te_chunked. +parse_chunked_invalid_size_test() -> + ?assertEqual({error, invalid_chunk_size}, + run_chunked([<<"5\rX\nHELLO\r\n0\r\n\r\n">>])). + +%% A malformed chunk terminator must fail cleanly as well +%% (read_chunk's error was unhandled in te_chunked). +parse_chunked_bad_chunk_terminator_test() -> + ?assertEqual({error, poorly_formatted_chunked_size}, + run_chunked([<<"5\r\nHELLOXX0\r\n\r\n">>])). + +%% Drive a chunked response body through the parser, feeding Segments as +%% separate socket reads. Contract: after {ok, Chunk, P}, drain buffered data +%% with execute/1 until the parser asks for more, only then feed the next +%% segment. Returns {done, Body} when the response completed with no +%% leftover bytes. +run_chunked(Segments) -> + P0 = hackney_http:parser([response]), + {_, _, _, _, P1} = hackney_http:execute(P0, <<"HTTP/1.1 200 OK\r\n">>), + {_, _, P2} = hackney_http:execute(P1, <<"Transfer-Encoding: chunked\r\n">>), + {headers_complete, P3} = hackney_http:execute(P2, <<"\r\n">>), + feed_chunked(P3, Segments, <<>>). + +feed_chunked(_Parser, [], Acc) -> + {incomplete, Acc}; +feed_chunked(Parser, [Seg | Rest], Acc) -> + chunked_step(hackney_http:execute(Parser, Seg), Rest, Acc). + +chunked_step({ok, Chunk, Parser}, Segments, Acc) -> + chunked_step(hackney_http:execute(Parser), Segments, <>); +chunked_step({more, Parser}, Segments, Acc) -> + feed_chunked(Parser, Segments, Acc); +chunked_step({more, Parser, _Buffer}, Segments, Acc) -> + feed_chunked(Parser, Segments, Acc); +chunked_step({done, <<>>}, _Segments, Acc) -> + {done, Acc}; +chunked_step({done, Leftover}, _Segments, Acc) -> + {done_with_leftover, Acc, Leftover}; +chunked_step(done, _Segments, Acc) -> + {done, Acc}; +chunked_step({error, Reason}, _Segments, _Acc) -> + {error, Reason}. + %% Issue #697: Handle non-standard decimal status codes (e.g., 401.1 from IIS) parse_response_decimal_status_code_test() -> Response = <<"HTTP/1.1 401.1 Access Denied">>,