From 97a3108576c6a9d64c03e1455654dba88367992a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Hoguin?= Date: Sat, 12 Jul 2014 12:09:43 +0200 Subject: Reply with 400 on header parsing crash This is a first step to improve the HTTP status codes returned by Cowboy on crashes. We will tweak it over time. Also fixes a small bug where two replies may have been sent when using loop handlers under rare conditions. --- src/cowboy_handler.erl | 27 +++++++++++++++------------ src/cowboy_protocol.erl | 2 +- src/cowboy_req.erl | 13 ++++++++++--- src/cowboy_rest.erl | 10 ++++++---- src/cowboy_spdy.erl | 4 ++-- src/cowboy_websocket.erl | 13 +++++++++---- test/handlers/input_crash_h.erl | 10 ++++++++++ test/http_SUITE.erl | 2 ++ 8 files changed, 55 insertions(+), 26 deletions(-) create mode 100644 test/handlers/input_crash_h.erl diff --git a/src/cowboy_handler.erl b/src/cowboy_handler.erl index 263c659..d78321b 100644 --- a/src/cowboy_handler.erl +++ b/src/cowboy_handler.erl @@ -85,11 +85,12 @@ handler_init(Req, State, Handler, HandlerOpts) -> {upgrade, protocol, Module, Req2, HandlerOpts2} -> upgrade_protocol(Req2, State, Handler, HandlerOpts2, Module) catch Class:Reason -> - cowboy_req:maybe_reply(500, Req), + Stacktrace = erlang:get_stacktrace(), + cowboy_req:maybe_reply(Stacktrace, Req), erlang:Class([ {reason, Reason}, {mfa, {Handler, init, 3}}, - {stacktrace, erlang:get_stacktrace()}, + {stacktrace, Stacktrace}, {req, cowboy_req:to_list(Req)}, {opts, HandlerOpts} ]) @@ -113,12 +114,13 @@ handler_handle(Req, State, Handler, HandlerState) -> terminate_request(Req2, State, Handler, HandlerState2, {normal, shutdown}) catch Class:Reason -> - cowboy_req:maybe_reply(500, Req), + Stacktrace = erlang:get_stacktrace(), + cowboy_req:maybe_reply(Stacktrace, Req), handler_terminate(Req, Handler, HandlerState, Reason), erlang:Class([ {reason, Reason}, {mfa, {Handler, handle, 2}}, - {stacktrace, erlang:get_stacktrace()}, + {stacktrace, Stacktrace}, {req, cowboy_req:to_list(Req)}, {state, HandlerState} ]) @@ -170,8 +172,8 @@ handler_loop_timeout(State=#state{loop_timeout=Timeout, -> {ok, Req, cowboy_middleware:env()} | {suspend, module(), atom(), [any()]} when Req::cowboy_req:req(). handler_loop(Req, State=#state{loop_buffer_size=NbBytes, - loop_max_buffer=Threshold, loop_timeout_ref=TRef}, - Handler, HandlerState) -> + loop_max_buffer=Threshold, loop_timeout_ref=TRef, + resp_sent=RespSent}, Handler, HandlerState) -> [Socket, Transport] = cowboy_req:get([socket, transport], Req), {OK, Closed, Error} = Transport:messages(), receive @@ -180,7 +182,9 @@ handler_loop(Req, State=#state{loop_buffer_size=NbBytes, if NbBytes2 > Threshold -> _ = handler_terminate(Req, Handler, HandlerState, {error, overflow}), - cowboy_req:maybe_reply(500, Req), + _ = if RespSent -> ok; true -> + cowboy_req:reply(500, Req) + end, exit(normal); true -> Req2 = cowboy_req:append_buffer(Data, Req), @@ -229,16 +233,15 @@ handler_call(Req, State=#state{resp_sent=RespSent}, handler_after_callback(Req2, State#state{hibernate=true}, Handler, HandlerState2) catch Class:Reason -> - if RespSent -> - ok; - true -> - cowboy_req:maybe_reply(500, Req) + Stacktrace = erlang:get_stacktrace(), + if RespSent -> ok; true -> + cowboy_req:maybe_reply(Stacktrace, Req) end, handler_terminate(Req, Handler, HandlerState, Reason), erlang:Class([ {reason, Reason}, {mfa, {Handler, info, 3}}, - {stacktrace, erlang:get_stacktrace()}, + {stacktrace, Stacktrace}, {req, cowboy_req:to_list(Req)}, {state, HandlerState} ]) diff --git a/src/cowboy_protocol.erl b/src/cowboy_protocol.erl index 3ac967e..84c4401 100644 --- a/src/cowboy_protocol.erl +++ b/src/cowboy_protocol.erl @@ -494,7 +494,7 @@ error_terminate(Status, State=#state{socket=Socket, transport=Transport, -spec error_terminate(cowboy:http_status(), cowboy_req:req(), #state{}) -> ok. error_terminate(Status, Req, State) -> - cowboy_req:maybe_reply(Status, Req), + _ = cowboy_req:reply(Status, Req), terminate(State). -spec terminate(#state{}) -> ok. diff --git a/src/cowboy_req.erl b/src/cowboy_req.erl index 02089c2..86f06ec 100644 --- a/src/cowboy_req.erl +++ b/src/cowboy_req.erl @@ -1036,15 +1036,22 @@ continue(#http_req{socket=Socket, transport=Transport, << HTTPVer/binary, " ", (status(100))/binary, "\r\n\r\n" >>). %% Meant to be used internally for sending errors after crashes. --spec maybe_reply(cowboy:http_status(), req()) -> ok. -maybe_reply(Status, Req) -> +-spec maybe_reply([{module(), atom(), arity() | [term()], _}], req()) -> ok. +maybe_reply(Stacktrace, Req) -> receive {cowboy_req, resp_sent} -> ok after 0 -> - _ = cowboy_req:reply(Status, Req), + _ = do_maybe_reply(Stacktrace, Req), ok end. +do_maybe_reply([ + {cow_http_hd, _, _, _}, + {cowboy_req, parse_header, _, _}|_], Req) -> + cowboy_req:reply(400, Req); +do_maybe_reply(_, Req) -> + cowboy_req:reply(500, Req). + -spec ensure_response(req(), cowboy:http_status()) -> ok. %% The response has already been fully sent to the client. ensure_response(#http_req{resp_state=done}, _) -> diff --git a/src/cowboy_rest.erl b/src/cowboy_rest.erl index fd1588a..f779612 100644 --- a/src/cowboy_rest.erl +++ b/src/cowboy_rest.erl @@ -66,11 +66,12 @@ upgrade(Req, Env, Handler, HandlerOpts) -> service_available(Req2, #state{env=Env, method=Method, handler=Handler, handler_state=HandlerState}) catch Class:Reason -> - cowboy_req:maybe_reply(500, Req), + Stacktrace = erlang:get_stacktrace(), + cowboy_req:maybe_reply(Stacktrace, Req), erlang:Class([ {reason, Reason}, {mfa, {Handler, rest_init, 2}}, - {stacktrace, erlang:get_stacktrace()}, + {stacktrace, Stacktrace}, {req, cowboy_req:to_list(Req)}, {opts, HandlerOpts} ]) @@ -999,11 +1000,12 @@ terminate(Req, State=#state{env=Env}) -> error_terminate(Req, State=#state{handler=Handler, handler_state=HandlerState}, Class, Reason, Callback) -> rest_terminate(Req, State), - cowboy_req:maybe_reply(500, Req), + Stacktrace = erlang:get_stacktrace(), + cowboy_req:maybe_reply(Stacktrace, Req), erlang:Class([ {reason, Reason}, {mfa, {Handler, Callback, 2}}, - {stacktrace, erlang:get_stacktrace()}, + {stacktrace, Stacktrace}, {req, cowboy_req:to_list(Req)}, {state, HandlerState} ]). diff --git a/src/cowboy_spdy.erl b/src/cowboy_spdy.erl index faea142..8da9613 100644 --- a/src/cowboy_spdy.erl +++ b/src/cowboy_spdy.erl @@ -422,7 +422,7 @@ execute(Req, Env, [Middleware|Tail]) -> {halt, Req2} -> cowboy_req:ensure_response(Req2, 204); {error, Status, Req2} -> - cowboy_req:maybe_reply(Status, Req2) + cowboy_req:reply(Status, Req2) end. -spec resume(cowboy_middleware:env(), [module()], @@ -437,7 +437,7 @@ resume(Env, Tail, Module, Function, Args) -> {halt, Req2} -> cowboy_req:ensure_response(Req2, 204); {error, Status, Req2} -> - cowboy_req:maybe_reply(Status, Req2) + cowboy_req:reply(Status, Req2) end. %% Reply functions used by cowboy_req. diff --git a/src/cowboy_websocket.erl b/src/cowboy_websocket.erl index 6c41137..d13441d 100644 --- a/src/cowboy_websocket.erl +++ b/src/cowboy_websocket.erl @@ -71,8 +71,12 @@ upgrade(Req, Env, Handler, HandlerOpts) -> {ok, State2, Req2} -> handler_init(State2, Req2, HandlerOpts) catch _:_ -> - cowboy_req:maybe_reply(400, Req), - exit(normal) + receive + {cowboy_req, resp_sent} -> ok + after 0 -> + cowboy_req:reply(400, Req), + exit(normal) + end end. -spec websocket_upgrade(#state{}, Req) @@ -144,11 +148,12 @@ handler_init(State=#state{env=Env, transport=Transport, cowboy_req:ensure_response(Req2, 400), {ok, Req2, [{result, closed}|Env]} catch Class:Reason -> - cowboy_req:maybe_reply(400, Req), + Stacktrace = erlang:get_stacktrace(), + cowboy_req:maybe_reply(Stacktrace, Req), erlang:Class([ {reason, Reason}, {mfa, {Handler, websocket_init, 3}}, - {stacktrace, erlang:get_stacktrace()}, + {stacktrace, Stacktrace}, {req, cowboy_req:to_list(Req)}, {opts, HandlerOpts} ]) diff --git a/test/handlers/input_crash_h.erl b/test/handlers/input_crash_h.erl new file mode 100644 index 0000000..668d053 --- /dev/null +++ b/test/handlers/input_crash_h.erl @@ -0,0 +1,10 @@ +%% This module crashes on request input data +%% depending on the given option. + +-module(input_crash_h). + +-export([init/3]). + +init(_, Req, content_length) -> + cowboy_error_h:ignore(cow_http_hd, number, 2), + cowboy_req:parse_header(<<"content-length">>, Req). diff --git a/test/http_SUITE.erl b/test/http_SUITE.erl index 5888421..e5a9256 100644 --- a/test/http_SUITE.erl +++ b/test/http_SUITE.erl @@ -193,6 +193,7 @@ init_dispatch(Config) -> {"/multipart/large", http_multipart_stream, []}, {"/echo/body", http_echo_body, []}, {"/echo/body_qs", http_body_qs, []}, + {"/crash/content-length", input_crash_h, content_length}, {"/param_all", rest_param_all, []}, {"/bad_accept", rest_simple_resource, []}, {"/bad_content_type", rest_patch_resource, []}, @@ -274,6 +275,7 @@ The document has moved {400, "GET / HTTP/1.1\r\nHost: ninenines.eu\r\n\r\n"}, {400, "GET http://proxy/ HTTP/1.1\r\n\r\n"}, {400, "GET / HTTP/1.1\r\nHost: localhost:bad_port\r\n\r\n"}, + {400, ["POST /crash/content-length HTTP/1.1\r\nHost: localhost\r\nContent-Length: 5000,5000\r\n\r\n", Huge]}, {505, ResponsePacket}, {408, "GET / HTTP/1.1\r\n"}, {408, "GET / HTTP/1.1\r\nHost: localhost"}, -- cgit v1.2.3