This is an automated email from the ASF dual-hosted git repository. nickva pushed a commit to tag v3.5.0 in repository https://gitbox.apache.org/repos/asf/couchdb-mochiweb.git
commit 03ce3a9973fe0cb3dafb80d80222ad7be725f4d3 Author: Nick Vatamaniuc <[email protected]> AuthorDate: Mon Aug 10 18:04:39 2026 -0400 Switch to passive mode and handle more corner cases Switch to passive mode to avoid having to toggle active `once` constantly. Passive mode is sufficient for pattern we're using. Not having to talk to the port to hav to toggle active once after every message should also help with the performance a bit. Bringing in a few tests cases for RFC7230 from Cowboy helped fix some corner cases such as: - Bocking until 30sec timeout on invalid headers (headers without colons) instead returning immediately with a 400 - When the socket was about to close we didn't always send `connection: close` but now we do. Granted, if a client was in the process of sending the request still, they might not noticed and just hit a reset but we can still do the right thing anyway. - SSL didn't honor the emsgsize setting. So use the SSL packet_size to apply a limit and a test to cover it --- src/mochiweb_http.erl | 57 +++++++-------- src/mochiweb_socket.erl | 18 ++++- test/mochiweb_http_tests.erl | 160 ++++++++++++++++++++++++++++++++++++++++++- test/mochiweb_test_util.erl | 3 +- 4 files changed, 201 insertions(+), 37 deletions(-) diff --git a/src/mochiweb_http.erl b/src/mochiweb_http.erl index 7d5af66..f9c72c7 100644 --- a/src/mochiweb_http.erl +++ b/src/mochiweb_http.erl @@ -87,33 +87,26 @@ loop(Socket, Opts, Body) -> request(Socket, Opts, Body). request(Socket, Opts, Body) -> - ok = - mochiweb_socket:exit_if_closed(mochiweb_socket:setopts(Socket, - [{active, - once}])), - receive - {Protocol, _, {http_request, Method, Path, Version}} - when Protocol == http orelse Protocol == ssl -> + case mochiweb_socket:recv(Socket, 0, ?REQUEST_RECV_TIMEOUT) of + {ok, {http_request, Method, Path, Version}} -> ok = mochiweb_socket:exit_if_closed(mochiweb_socket:setopts(Socket, [{packet, httph}])), headers(Socket, Opts, {Method, Path, Version}, [], Body, 0); - {Protocol, _, {http_error, "\r\n"}} - when Protocol == http orelse Protocol == ssl -> - request(Socket, Opts, Body); - {Protocol, _, {http_error, "\n"}} - when Protocol == http orelse Protocol == ssl -> - request(Socket, Opts, Body); - {tcp_closed = Error, _} -> + %% skip stray CRLF (or LF) before a request (rfc7230 sec 3.5) + {ok, {http_error, "\r\n"}} -> request(Socket, Opts, Body); + {ok, {http_error, "\n"}} -> request(Socket, Opts, Body); + {error, closed = Error} -> mochiweb_socket:close(Socket), exit({shutdown, Error}); - {tcp_error, _, emsgsize} -> + {error, timeout} -> + mochiweb_socket:close(Socket), + exit({shutdown, request_recv_timeout}); + {error, emsgsize} -> handle_invalid_request(Socket, Opts); - {ssl_closed = Error, _} -> - mochiweb_socket:close(Socket), exit({shutdown, Error}) - after ?REQUEST_RECV_TIMEOUT -> - mochiweb_socket:close(Socket), exit({shutdown, request_recv_timeout}) + _Other -> + handle_invalid_request(Socket, Opts) end. reentry(Body) -> @@ -129,26 +122,23 @@ headers(Socket, Opts, Request, Headers, _Body, handle_invalid_request(Socket, Opts, Request, Headers); headers(Socket, Opts, Request, Headers, Body, HeaderCount) -> - ok = - mochiweb_socket:exit_if_closed(mochiweb_socket:setopts(Socket, - [{active, - once}])), - receive - {Protocol, _, http_eoh} - when Protocol == http orelse Protocol == ssl -> + case mochiweb_socket:recv(Socket, 0, ?HEADERS_RECV_TIMEOUT) of + {ok, http_eoh} -> Req = new_request(Socket, Opts, Request, Headers), call_body(Body, Req), (?MODULE):after_response(Body, Req); - {Protocol, _, {http_header, _, Name, _, Value}} - when Protocol == http orelse Protocol == ssl -> + {ok, {http_header, _, Name, _, Value}} -> headers(Socket, Opts, Request, [{Name, Value} | Headers], Body, 1 + HeaderCount); - {tcp_closed = Error, _} -> + {error, closed = Error} -> mochiweb_socket:close(Socket), exit({shutdown, Error}); - {tcp_error, _, emsgsize} -> + {error, timeout} -> + mochiweb_socket:close(Socket), + exit({shutdown, headers_recv_timeout}); + {error, emsgsize} -> + handle_invalid_request(Socket, Opts, Request, Headers); + _Other -> handle_invalid_request(Socket, Opts, Request, Headers) - after ?HEADERS_RECV_TIMEOUT -> - mochiweb_socket:close(Socket), exit({shutdown, headers_recv_timeout}) end. call_body({M, F, A}, Req) when is_atom(M) -> @@ -169,7 +159,8 @@ handle_invalid_request(Socket, Opts, Request, RevHeaders) -> {ReqM, _} = Req = new_request(Socket, Opts, Request, RevHeaders), - ReqM:respond({400, [], []}, Req), + %% Advertise socket closure with connection:close (rfc7230 sec 6.6) + ReqM:respond({400, [{"Connection", "close"}], []}, Req), mochiweb_socket:close(Socket), exit({shutdown, invalid_request}). diff --git a/src/mochiweb_socket.erl b/src/mochiweb_socket.erl index 053f6e1..ba7b53e 100644 --- a/src/mochiweb_socket.erl +++ b/src/mochiweb_socket.erl @@ -4,6 +4,8 @@ -module(mochiweb_socket). +-include("internal.hrl"). + -export([listen/4, accept/1, transport_accept/1, finish_accept/1, recv/3, send/2, close/1, port/1, peername/1, @@ -20,7 +22,8 @@ listen(Ssl, Port, Opts, SslOpts) -> true -> Opts1 = add_safe_protocol_versions(Opts), Opts2 = add_unbroken_ciphers_default(Opts1 ++ SslOpts), - case ssl:listen(Port, Opts2) of + Opts3 = add_packet_size_limit(Opts2), + case ssl:listen(Port, Opts3) of {ok, ListenSocket} -> {ok, {ssl, ListenSocket}}; {error, _} = Err -> @@ -30,6 +33,19 @@ listen(Ssl, Port, Opts, SslOpts) -> gen_tcp:listen(Port, Opts) end. +%% SSL equivalent for max buffer limit (emsgize) is packet_size. Set it so +%% we get the same behavior for TCP and SSL transports. On error the long +%% header lines will faul w/ {error, {invalid_packet, _}} in both cases. +add_packet_size_limit(Opts) -> + case proplists:is_defined(packet_size, Opts) of + true -> + Opts; + false -> + RecBuf = proplists:get_value(recbuf, Opts, ?RECBUF_SIZE), + Buffer = proplists:get_value(buffer, Opts, RecBuf), + [{packet_size, Buffer} | Opts] + end. + add_unbroken_ciphers_default(Opts) -> %% add_safe_protocol_versions/1 must have been called to ensure a {versions, _} tuple is present Versions = proplists:get_value(versions, Opts), diff --git a/test/mochiweb_http_tests.erl b/test/mochiweb_http_tests.erl index e442384..2218f99 100644 --- a/test/mochiweb_http_tests.erl +++ b/test/mochiweb_http_tests.erl @@ -9,9 +9,19 @@ has_acceptor_bug_test_() -> start_server() -> + start_server(plain). + +start_server(Transport) -> application:start(inets), - {ok, Pid} = mochiweb_http:start_link([{port, 0}, - {loop, fun responder/1}]), + Opts = [{port, 0}, {loop, fun responder/1}] ++ + case Transport of + plain -> + []; + ssl -> + [{ssl, true}, + {ssl_opts, mochiweb_test_util:ssl_cert_opts()}] + end, + {ok, Pid} = mochiweb_http:start_link(Opts), Pid. chunked_server(Req) -> @@ -299,3 +309,149 @@ has_bug(Port, Len) -> %% It is expected that the request will fail because the header is too long {ok, {{"HTTP/1.1", 400, "Bad Request"}, _, []}} -> false end. + +%% rfc7230 cases taken from cowboy's test/rfc7230_SUITE.erl (ISC license) +rfc7230_test_() -> + %% Go over tcp and tls + [{setup, fun () -> start_server(Transport) end, + fun mochiweb_http:stop/1, + fun (Server) -> + Port = mochiweb_socket_server:get(Server, port), + [{lists:concat([Transport, ": ", Doc]), + {timeout, 15, + ?_assertEqual(expect_for(Transport, Expect), + raw_exchange(Transport, Port, Raw))}} + || {Doc, Expect, Raw} <- rfc7230_cases()] + end} || Transport <- [plain, ssl]]. + +%% Most of the time we expect both transport to have the same behavior but not +%% always they can differ (see header line too long case below) +expect_for(plain, {per_transport, Plain, _Ssl}) -> Plain; +expect_for(ssl, {per_transport, _Plain, Ssl}) -> Ssl; +expect_for(_Transport, Expect) -> Expect. + +rfc7230_cases() -> + [{"empty line before the request line is skipped", + {response, 200}, + <<"\r\nGET / HTTP/1.1\r\nHost: l\r\n\r\n">>}, + {"stray LF alone before request line is skipped", + {response, 200}, + <<"\nGET / HTTP/1.1\r\nHost: l\r\n\r\n">>}, + {"bunch of empty lines before request are skipped", + {response, 200}, + <<"\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n" + "GET / HTTP/1.1\r\nHost: l\r\n\r\n">>}, + {"response as a request doesn't work", + {response, 400}, + <<"HTTP/1.1 200 OK\r\n\r\n">>}, + {"a malformed request line is rejected (3.1.1)", + {response, 400}, + <<"GET\r\n">>}, + %% TCP limits long lines with inet buffer size (emsgsize). There we + %% reject it with a 400. For SSL connections we limit it with packet_size. + %% If we get a line that's too long then the connection is torn down so + %% we get a connection closed case + {"request line longer than the buffer", + {per_transport, {response, 400}, closed}, + iolist_to_binary(["GET /", binary:copy(<<"a">>, 10240), + " HTTP/1.1\r\nHost: l\r\n\r\n"])}, + {"header line longer than the buffer", + {per_transport, {response, 400}, closed}, + iolist_to_binary(["GET / HTTP/1.1\r\nHost: l\r\nx-huge: ", + binary:copy(<<"a">>, 10240), "\r\n\r\n"])}, + {"absolute form paths are accepted", + {response, 200}, + <<"GET http://example.org/ HTTP/1.1\r\nHost: l\r\n\r\n">>}, + {"star form path is ok", + {response, 200}, + <<"OPTIONS * HTTP/1.1\r\nHost: l\r\n\r\n">>}, + {"no whitespace before header colon", + {response, 400}, + <<"GET / HTTP/1.1\r\nHost : l\r\n\r\n">>}, + {"header lines need a colon", + {response, 400}, + <<"GET / HTTP/1.1\r\nHost: l\r\nheader-line-without-a-colon\r\n\r\n">>}, + {"header continuations are allowed (sec 3.2.4)", + {response, 200}, + <<"GET / HTTP/1.1\r\nHost: l\r\nX-A: 1\r\n\tfolded\r\n\r\n">>}, + {"bare LF line endings are ok (sec 3.5)", + {response, 200}, + <<"GET / HTTP/1.1\nHost: l\n\n">>}, + {"missing Host header is not enforced (sec 5.4, apps can chose here)", + {response, 200}, + <<"GET / HTTP/1.1\r\n\r\n">>}, + {"can have spaces in request line are tolerated (sec 3.1.1)", + {response, 200}, + <<"GET / HTTP/1.1\r\nHost: l\r\n\r\n">>}, + {"no ridiculous number of headers 1000 (sec 3.2.5)", + {response, 400}, + iolist_to_binary(["GET / HTTP/1.1\r\nHost: l\r\n", + [["X-", integer_to_list(I), ": a\r\n"] + || I <- lists:seq(1, 10001)], + "\r\n"])}]. + +raw_exchange(plain, Port, Raw) -> + {ok, S} = gen_tcp:connect("127.0.0.1", Port, + [binary, {active, false}, {packet, http}, + {nodelay, true}]), + raw_exchange1(gen_tcp, S, Raw); +raw_exchange(ssl, Port, Raw) -> + ClientOpts = mochiweb_test_util:ssl_client_opts( + [binary, {active, false}, {packet, http}, + {nodelay, true}]), + {ok, S} = ssl:connect("127.0.0.1", Port, ClientOpts), + raw_exchange1(ssl, S, Raw). + +raw_exchange1(Mod, S, Raw) -> + ok = Mod:send(S, Raw), + R = case Mod:recv(S, 0, 2000) of + {ok, {http_response, _, Code, _}} -> {response, Code}; + {error, closed} -> closed; + {error, timeout} -> no_response; + Other -> Other + end, + close_socket(Mod, S), + R. + +close_socket(gen_tcp, S) -> + gen_tcp:close(S); +close_socket(ssl, S) -> + %% use a bounded time for cleanup as ssl connection can take a while to tear down + try ssl:close(S, 1000) catch _:_ -> ok end, + ok. + +%% Check what happens if client goes away while sneding header. Server should cleanup +%% and then continue serving other requests +client_disconnect_mid_headers_test() -> + Res = mochiweb_test_util:with_server( + plain, + fun responder/1, + fun (plain, Port) -> + {ok, S} = gen_tcp:connect("127.0.0.1", Port, + [binary, {active, false}]), + ok = gen_tcp:send(S, <<"GET / HTTP/1.1\r\nHost: l\r\n">>), + ok = gen_tcp:close(S), + ?assertEqual({response, 200}, + raw_exchange(plain, Port, + <<"GET / HTTP/1.1\r\nHost: l\r\n\r\n">>)) + end + ), + ?assertEqual(ok, Res). + +%% If we're about to close, send a 400 response with connection:close (sec 6.6) +invalid_request_connection_close_test() -> + Res = mochiweb_test_util:with_server( + plain, + fun responder/1, + fun (Transport, Port) -> + SockFun = mochiweb_test_util:sock_fun(Transport, Port), + ok = SockFun({send, <<"GET / HTTP/1.1\r\nbadheader\r\n" + "Host: l\r\n\r\n">>}), + {ok, {http_response, {1, 1}, 400, _}} = SockFun(recv), + Headers = mochiweb_test_util:read_server_headers(SockFun), + ?assertEqual("close", + mochiweb_headers:get_value("Connection", Headers)), + ok + end + ), + ?assertEqual(ok, Res). diff --git a/test/mochiweb_test_util.erl b/test/mochiweb_test_util.erl index 0076b04..e00a328 100644 --- a/test/mochiweb_test_util.erl +++ b/test/mochiweb_test_util.erl @@ -1,6 +1,7 @@ -module(mochiweb_test_util). -export([with_server/3, client_request/4, sock_fun/2, - read_server_headers/1, drain_reply/3, ssl_client_opts/1]). + read_server_headers/1, drain_reply/3, ssl_client_opts/1, + ssl_cert_opts/0]). -include("mochiweb_test_util.hrl"). -include_lib("eunit/include/eunit.hrl").
