You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@couchdb.apache.org by va...@apache.org on 2026/08/16 17:00:22 UTC
(couchdb-mochiweb) 01/04: Switch to passive mode and handle more corner cases
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 <va...@gmail.com>
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").