From 414ff2af38ed93daabb383a0abaac0bee0ab0504 Mon Sep 17 00:00:00 2001 From: Mat Trudel Date: Fri, 24 May 2024 16:30:36 -0400 Subject: [PATCH] Raise error on send errors for HTTP/1 responses --- lib/bandit/adapter.ex | 9 ++- lib/bandit/http1/socket.ex | 40 +++++++--- test/bandit/http1/request_test.exs | 115 +++++++++++++++++++++++++++++ test/support/transport.ex | 7 +- 4 files changed, 159 insertions(+), 12 deletions(-) diff --git a/lib/bandit/adapter.ex b/lib/bandit/adapter.ex index 62074254..41de06e0 100644 --- a/lib/bandit/adapter.ex +++ b/lib/bandit/adapter.ex @@ -195,7 +195,14 @@ defmodule Bandit.Adapter do # this entire section of the API is a bit slanty regardless. validate_calling_process!(adapter) - {:ok, nil, send_data(adapter, chunk, IO.iodata_length(chunk) == 0)} + + # chunk/2 is unique among Plug.Conn.Adapter's sending callbacks in that it can return an error + # tuple instead of just raising or dying on error. Rescue here to implement this + try do + {:ok, nil, send_data(adapter, chunk, IO.iodata_length(chunk) == 0)} + rescue + error -> {:error, Exception.format_banner(:error, error)} + end end @impl Plug.Conn.Adapter diff --git a/lib/bandit/http1/socket.ex b/lib/bandit/http1/socket.ex index e1a01bbd..25c183c9 100644 --- a/lib/bandit/http1/socket.ex +++ b/lib/bandit/http1/socket.ex @@ -332,15 +332,15 @@ defmodule Bandit.HTTP1.Socket do :chunk_encoded -> headers = [{"transfer-encoding", "chunked"} | headers] - _ = ThousandIsland.Socket.send(socket.socket, [resp_line | encode_headers(headers)]) + send!(socket.socket, [resp_line | encode_headers(headers)]) %{socket | write_state: :chunking} :no_body -> - _ = ThousandIsland.Socket.send(socket.socket, [resp_line | encode_headers(headers)]) + send!(socket.socket, [resp_line | encode_headers(headers)]) %{socket | write_state: :sent} :inform -> - _ = ThousandIsland.Socket.send(socket.socket, [resp_line | encode_headers(headers)]) + send!(socket.socket, [resp_line | encode_headers(headers)]) %{socket | write_state: :unsent} end end @@ -352,23 +352,38 @@ defmodule Bandit.HTTP1.Socket do end def send_data(%@for{write_state: :writing} = socket, data, end_request) do - _ = ThousandIsland.Socket.send(socket.socket, [socket.send_buffer | data]) + send!(socket.socket, [socket.send_buffer | data]) write_state = if end_request, do: :sent, else: :writing %{socket | write_state: write_state, send_buffer: []} end def send_data(%@for{write_state: :chunking} = socket, data, end_request) do byte_size = data |> IO.iodata_length() - payload = [Integer.to_string(byte_size, 16), "\r\n", data, "\r\n"] - _ = ThousandIsland.Socket.send(socket.socket, payload) + send!(socket.socket, [Integer.to_string(byte_size, 16), "\r\n", data, "\r\n"]) write_state = if end_request, do: :sent, else: :chunking %{socket | write_state: write_state} end def sendfile(%@for{write_state: :writing} = socket, path, offset, length) do - _ = ThousandIsland.Socket.send(socket.socket, socket.send_buffer) - _ = ThousandIsland.Socket.sendfile(socket.socket, path, offset, length) - %{socket | write_state: :sent} + send!(socket.socket, socket.send_buffer) + + case ThousandIsland.Socket.sendfile(socket.socket, path, offset, length) do + {:ok, _bytes_written} -> %{socket | write_state: :sent} + {:error, reason} -> request_error!(reason) + end + end + + @spec send!(ThousandIsland.Socket.t(), iolist()) :: :ok | no_return() + defp send!(socket, payload) do + case ThousandIsland.Socket.send(socket, payload) do + :ok -> + :ok + + {:error, reason} -> + # Prevent error handlers from possibly trying to send again + send(self(), {:plug_conn, :sent}) + request_error!(reason) + end end def ensure_completed(%@for{read_state: :read} = socket), do: socket @@ -388,7 +403,12 @@ defmodule Bandit.HTTP1.Socket do after 0 -> status = error |> Plug.Exception.status() |> Plug.Conn.Status.code() - send_headers(socket, status, [], :no_body) + + try do + send_headers(socket, status, [], :no_body) + rescue + Bandit.HTTPError -> :ok + end end end diff --git a/test/bandit/http1/request_test.exs b/test/bandit/http1/request_test.exs index b2d0bb57..6a8a282c 100644 --- a/test/bandit/http1/request_test.exs +++ b/test/bandit/http1/request_test.exs @@ -7,6 +7,8 @@ defmodule HTTP1RequestTest do import ExUnit.CaptureLog + require Logger + setup :http_server setup :req_http1_client @@ -2089,4 +2091,117 @@ defmodule HTTP1RequestTest do assert output =~ "(RuntimeError) boom" end end + + describe "connection closure / error handling" do + test "raises an error if client closes while headers are being read", context do + client = SimpleHTTP1Client.tcp_client(context) + Transport.send(client, "GET / HTTP/1.1\r\nHost:") + Transport.close(client) + Process.sleep(100) + assert ThousandIsland.connection_pids(context.server_pid) == {:ok, []} + end + + test "raises an error if client closes while body is being read", context do + client = SimpleHTTP1Client.tcp_client(context) + + SimpleHTTP1Client.send(client, "POST", "/expect_incomplete_body", [ + "host: localhost", + "content-length: 6" + ]) + + Transport.send(client, "ABC") + + output = + capture_log(fn -> + Transport.close(client) + Process.sleep(500) + end) + + assert output =~ "(Bandit.HTTPError) closed" + refute output =~ "IMPOSSIBLE" + assert ThousandIsland.connection_pids(context.server_pid) == {:ok, []} + end + + def expect_incomplete_body(conn) do + {:ok, _body, _conn} = Plug.Conn.read_body(conn) + Logger.error("IMPOSSIBLE") + end + + test "raises an error if client closes while body is being written", context do + client = SimpleHTTP1Client.tcp_client(context) + + SimpleHTTP1Client.send(client, "GET", "/sleep_and_send", ["host: localhost"]) + Process.sleep(100) + Transport.close(client) + + output = capture_log(fn -> Process.sleep(500) end) + assert output =~ "(Bandit.HTTPError) closed" + refute output =~ "IMPOSSIBLE" + assert ThousandIsland.connection_pids(context.server_pid) == {:ok, []} + end + + def sleep_and_send(conn) do + Process.sleep(200) + + conn = send_resp(conn, 200, "IMPOSSIBLE") + + Logger.error("IMPOSSIBLE") + conn + end + + test "returns an error if client closes while chunked body is being written", context do + client = SimpleHTTP1Client.tcp_client(context) + + SimpleHTTP1Client.send(client, "GET", "/sleep_and_send_chunked", ["host: localhost"]) + Process.sleep(100) + Transport.close(client) + + output = capture_log(fn -> Process.sleep(500) end) + assert output == "" + assert ThousandIsland.connection_pids(context.server_pid) == {:ok, []} + end + + def sleep_and_send_chunked(conn) do + conn = send_chunked(conn, 200) + + Process.sleep(200) + assert chunk(conn, "IMPOSSIBLE") == {:error, "** (Bandit.HTTPError) closed"} + + conn + end + + test "raises an error if client closes before sendfile body is being written", context do + client = SimpleHTTP1Client.tcp_client(context) + + SimpleHTTP1Client.send(client, "GET", "/sleep_and_sendfile", ["host: localhost"]) + Process.sleep(100) + Transport.close(client) + + output = capture_log(fn -> Process.sleep(500) end) + assert output =~ "(Bandit.HTTPError) closed" + refute output =~ "IMPOSSIBLE" + assert ThousandIsland.connection_pids(context.server_pid) == {:ok, []} + end + + def sleep_and_sendfile(conn) do + Process.sleep(200) + + conn = send_file(conn, 204, Path.join([__DIR__, "../../support/sendfile"]), 0, :all) + + Logger.error("IMPOSSIBLE") + conn + end + + test "silently exits if client closes during keepalive", context do + client = SimpleHTTP1Client.tcp_client(context) + + SimpleHTTP1Client.send(client, "GET", "/hello_world", ["host: localhost"]) + Process.sleep(100) + SimpleHTTP1Client.recv_reply(client) + Transport.close(client) + Process.sleep(500) + + assert ThousandIsland.connection_pids(context.server_pid) == {:ok, []} + end + end end diff --git a/test/support/transport.ex b/test/support/transport.ex index 587ac570..525734da 100644 --- a/test/support/transport.ex +++ b/test/support/transport.ex @@ -3,7 +3,12 @@ defmodule Transport do def tcp_client(context) do {:ok, socket} = - :gen_tcp.connect(~c"localhost", context[:port], active: false, mode: :binary, nodelay: true) + :gen_tcp.connect(~c"localhost", context[:port], + active: false, + linger: {true, 0}, + mode: :binary, + nodelay: true + ) {:client, %{socket: socket, transport: :gen_tcp}} end