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

@@ -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},
+		loop_max_buffer=Threshold, loop_timeout_ref=TRef,
-		Handler, HandlerState) ->
+		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 ->
+		Stacktrace = erlang:get_stacktrace(),
-			ok;
+		if RespSent -> ok; true ->
-		true ->
+			cowboy_req:maybe_reply(Stacktrace, Req)
-			cowboy_req:maybe_reply(500, 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}
 		])

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.

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.
+-spec maybe_reply([{module(), atom(), arity() | [term()], _}], req()) -> ok.
-maybe_reply(Status, Req) ->
+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}, _) ->

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}
 	]).

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.

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),
+		receive
-		exit(normal)
+			{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}
 		])

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).

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"},