Skip to content

Loading…

long http headers and http error packets (zd864 + bz1291) #3

Open
wants to merge 3 commits into from

3 participants

@jaredmorrow

Merging a combination of Vinoski's header fix zd864 and Sean Cribbs' http error fix bz1291

vinoski and others added some commits
@vinoski vinoski fix zd864: long HTTP header lines not handled properly
Due to Erlang bug OTP-9389, the handling of HTTP request header lines
that exceed the size of the gen_tcp receive buffer is broken when
using {packet,http} and {packet,httph} modes.

Modify mochiweb to work around this bug by using {packet,line} mode
instead to collect request header lines and then using
erlang:decode_packet to parse them. Using {packet,line} allows us to
avoid reading past the headers into the request body (if present),
since doing that would require us to carry the body data from the
header handling functions over into the body handling functions, which
could be a pretty intrusive and complicated change.

Also add regression unit tests that verify that long HTTP request
lines and long HTTP header lines are handled correctly.
2ffb00b
@seancribbs seancribbs HttpError parses are returned as normal packets, not error tuples. caa6618
@seancribbs seancribbs Merge pull request #2 from basho/bz1291-http-error-packets
BZ1291 HttpError packet parsing fix
2984b74
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 29, 2011
  1. @vinoski

    fix zd864: long HTTP header lines not handled properly

    vinoski committed
    Due to Erlang bug OTP-9389, the handling of HTTP request header lines
    that exceed the size of the gen_tcp receive buffer is broken when
    using {packet,http} and {packet,httph} modes.
    
    Modify mochiweb to work around this bug by using {packet,line} mode
    instead to collect request header lines and then using
    erlang:decode_packet to parse them. Using {packet,line} allows us to
    avoid reading past the headers into the request body (if present),
    since doing that would require us to carry the body data from the
    header handling functions over into the body handling functions, which
    could be a pretty intrusive and complicated change.
    
    Also add regression unit tests that verify that long HTTP request
    lines and long HTTP header lines are handled correctly.
Commits on Nov 22, 2011
  1. @seancribbs
Commits on Nov 23, 2011
  1. @seancribbs

    Merge pull request #2 from basho/bz1291-http-error-packets

    seancribbs committed
    BZ1291 HttpError packet parsing fix
Showing with 118 additions and 24 deletions.
  1. +118 −24 src/mochiweb_http.erl
View
142 src/mochiweb_http.erl
@@ -96,22 +96,31 @@ default_body(Req) ->
default_body(Req, Req:get(method), Req:get(path)).
loop(Socket, Body) ->
- mochiweb_socket:setopts(Socket, [{packet, http}]),
- request(Socket, Body).
+ ok = mochiweb_socket:setopts(Socket, [{packet, line}]),
+ request(Socket, Body, <<>>).
-request(Socket, Body) ->
- mochiweb_socket:setopts(Socket, [{active, once}]),
+request(Socket, Body, Prev) ->
+ ok = mochiweb_socket:setopts(Socket, [{active, once}]),
receive
- {Protocol, _, {http_request, Method, Path, Version}} when Protocol == http orelse Protocol == ssl ->
- mochiweb_socket:setopts(Socket, [{packet, httph}]),
- headers(Socket, {Method, Path, Version}, [], Body, 0);
- {Protocol, _, {http_error, "\r\n"}} when Protocol == http orelse Protocol == ssl ->
- request(Socket, Body);
- {Protocol, _, {http_error, "\n"}} when Protocol == http orelse Protocol == ssl ->
- request(Socket, Body);
+ {Protocol, _, Bin} when Protocol =:= tcp orelse Protocol =:= ssl ->
+ FullBin = <<Prev/binary, Bin/binary>>,
+ case erlang:decode_packet(http, FullBin, []) of
+ {ok, {http_request, Method, Path, Version}, <<>>} ->
+ collect_headers(Socket, {Method, Path, Version}, Body,
+ <<>>, false, 0);
+ {ok, {http_error, "\r\n"}, <<>>} ->
+ request(Socket, Body, <<>>);
+ {ok, {http_error, "\n"}, <<>>} ->
+ request(Socket, Body, <<>>);
+ {more, _} ->
+ request(Socket, Body, FullBin)
+ end;
{tcp_closed, _} ->
mochiweb_socket:close(Socket),
exit(normal);
+ {ssl_closed, _} ->
+ mochiweb_socket:close(Socket),
+ exit(normal);
_Other ->
handle_invalid_request(Socket)
after ?REQUEST_RECV_TIMEOUT ->
@@ -124,30 +133,64 @@ reentry(Body) ->
?MODULE:after_response(Body, Req)
end.
-headers(Socket, Request, Headers, _Body, ?MAX_HEADERS) ->
+collect_headers(Socket, Request, _Body, _Collected, _Trunc, ?MAX_HEADERS) ->
%% Too many headers sent, bad request.
- mochiweb_socket:setopts(Socket, [{packet, raw}]),
- handle_invalid_request(Socket, Request, Headers);
-headers(Socket, Request, Headers, Body, HeaderCount) ->
- mochiweb_socket:setopts(Socket, [{active, once}]),
+ handle_invalid_request(Socket, Request, []);
+collect_headers(Socket, Request, Body, Collected, Trunc, HeaderCount) ->
+ ok = mochiweb_socket:setopts(Socket, [{active, once}]),
receive
- {Protocol, _, http_eoh} when Protocol == http orelse Protocol == ssl ->
- Req = new_request(Socket, Request, Headers),
- call_body(Body, Req),
- ?MODULE:after_response(Body, Req);
- {Protocol, _, {http_header, _, Name, _, Value}} when Protocol == http orelse Protocol == ssl ->
- headers(Socket, Request, [{Name, Value} | Headers], Body,
- 1 + HeaderCount);
+ {Protocol, _, More} when Protocol =:= tcp orelse Protocol =:= ssl ->
+ case {Trunc, More} of
+ {false, <<"\n">>} ->
+ ok = mochiweb_socket:setopts(Socket, [{packet, raw}]),
+ parse_headers(Socket, Request, Body,
+ <<Collected/binary, "\r\n">>, []);
+ {false, <<"\r\n">>} ->
+ ok = mochiweb_socket:setopts(Socket, [{packet, raw}]),
+ parse_headers(Socket, Request, Body,
+ <<Collected/binary, "\r\n">>, []);
+ {_, More} ->
+ NewBin = <<Collected/binary, More/binary>>,
+ AllButOne= size(More) - 1,
+ {Truncated, NewHdrCount} =
+ case More of
+ <<_:AllButOne/binary, "\n">> ->
+ {false, 1 + HeaderCount};
+ _ ->
+ {true, HeaderCount}
+ end,
+ collect_headers(Socket, Request, Body, NewBin,
+ Truncated, NewHdrCount)
+ end;
{tcp_closed, _} ->
mochiweb_socket:close(Socket),
exit(normal);
+ {ssl_closed, _} ->
+ mochiweb_socket:close(Socket),
+ exit(normal);
_Other ->
- handle_invalid_request(Socket, Request, Headers)
+ handle_invalid_request(Socket, Request, [])
after ?HEADERS_RECV_TIMEOUT ->
mochiweb_socket:close(Socket),
exit(normal)
end.
+parse_headers(Socket, Request, Body, <<"\r\n">>, Headers) ->
+ Req = new_request(Socket, Request, lists:reverse(Headers)),
+ call_body(Body, Req),
+ ?MODULE:after_response(Body, Req);
+parse_headers(Socket, Request, Body, Bin, Headers) ->
+ case erlang:decode_packet(httph, Bin, []) of
+ {ok, {http_header, _, Name, _, Value}, More} ->
+ parse_headers(Socket, Request, Body, More,
+ [{Name, Value} | Headers]);
+ {more, _} ->
+ handle_invalid_request(Socket, Request, Headers);
+ {error, _Reason} ->
+ mochiweb_socket:close(Socket),
+ exit(normal)
+ end.
+
call_body({M, F, A}, Req) ->
erlang:apply(M, F, [Req | A]);
call_body({M, F}, Req) ->
@@ -290,4 +333,55 @@ range_skip_length_test() ->
range_skip_length({BodySize, none}, BodySize)),
ok.
+long_request_line_test() ->
+ {ok, LS} = gen_tcp:listen(0, [binary, {active, false}]),
+ {ok, Port} = inet:port(LS),
+ spawn_link(fun() ->
+ {ok, S} = gen_tcp:accept(LS),
+ try
+ loop(S, {?MODULE, default_body})
+ after
+ gen_tcp:close(S),
+ gen_tcp:close(LS)
+ end
+ end),
+ {ok, S} = gen_tcp:connect("localhost", Port, [binary, {active, false}]),
+ try
+ Req = "GET /" ++ string:chars($X, 8192) ++ " HTTP/1.1\r\n"
+ ++ "Host: localhost\r\n\r\n",
+ ok = gen_tcp:send(S, Req),
+ inet:setopts(S, [{packet, http}]),
+ ?assertEqual({ok, {http_response, {1,1}, 200, "OK"}},
+ gen_tcp:recv(S, 0)),
+ ok
+ after
+ gen_tcp:close(S)
+ end.
+
+long_header_test() ->
+ {ok, LS} = gen_tcp:listen(0, [binary, {active, false}]),
+ {ok, Port} = inet:port(LS),
+ spawn_link(fun() ->
+ {ok, S} = gen_tcp:accept(LS),
+ try
+ loop(S, {?MODULE, default_body})
+ after
+ gen_tcp:close(S),
+ gen_tcp:close(LS)
+ end
+ end),
+ {ok, S} = gen_tcp:connect("localhost", Port, [binary, {active, false}]),
+ try
+ Req = "GET / HTTP/1.1\r\n"
+ ++ "Host: localhost\r\n"
+ ++ "Link: /" ++ string:chars($X, 8192) ++ "\r\n\r\n",
+ ok = gen_tcp:send(S, Req),
+ inet:setopts(S, [{packet, http}]),
+ ?assertEqual({ok, {http_response, {1,1}, 200, "OK"}},
+ gen_tcp:recv(S, 0)),
+ ok
+ after
+ gen_tcp:close(S)
+ end.
+
-endif.
Something went wrong with that request. Please try again.