close/1 does not close socket properly on externally closed connection

[bitberry-dev]

Hello, guys!

I was testing my application to work in extremely edge cases and noticed that mint does not close sockets correctly under certain circumstances, this leads to a leak.

Mint does not close a socket when two circumstances coincide:

1. Connection was closed externally, for example, by timeout
2. Your process for some reason does not process ssl_closed message

I wrote simple test to demonstrate this

 @tag timeout: :infinity
  test "close/1 does not close socket properly on externally closed connection" do
    # Without timeout
    assert {:ok, %HTTP2{} = conn1} = HTTP2.connect(:https, "nghttp2.org", 443)
    IO.puts "Opened first connection ✓"

    assert {:ok, conn1} = HTTP2.close(conn1)
    IO.puts "Closed first connection ✓"

    refute HTTP2.open?(conn1)
    IO.puts "Connection closed successfully ✓"

    refute (conn1.socket |> elem(2) |> Enum.all?(fn pid -> Process.alive?(pid) end))
    IO.puts "Connection socket's processes died sucessfully ✓"

    # With timeout
    assert {:ok, %HTTP2{} = conn2} = HTTP2.connect(:https, "nghttp2.org", 443)
    IO.puts "Opened second connection ✓"

    IO.puts "Sleep 5 minutes zzzZZ"
    :timer.sleep(300_000) # yes, I know that we should listen messages and update connection, but wi ar bad

    assert {:error, conn2, %Mint.TransportError{reason: :closed}} = Mint.HTTP.request(conn2, "GET", "/", [], "")
    IO.puts "Tried to request with this connection and it ends with Mint.TransportError, connection dead for sure"

    assert {:ok, conn2} = HTTP2.close(conn2)
    IO.puts "Closed second connection ✓"

    refute HTTP2.open?(conn2)
    IO.puts "Connection closed successfully ✓"

    refute (conn2.socket |> elem(2) |> Enum.all?(fn pid -> Process.alive?(pid) end)), "Connection socket's processes not died :("
  end

Test results:

I quickly found the error by looking at the source code:

1. When close/1 called we are here https://github.com/elixir-mint/mint/blob/7aaf0377343f63dca87d8007fd404253a44598b5/lib/mint/http2.ex#L387
2. Then move on to send_connection_error!/3 https://github.com/elixir-mint/mint/blob/7aaf0377343f63dca87d8007fd404253a44598b5/lib/mint/http2.ex#L2027
3. And then move on to send!/2 https://github.com/elixir-mint/mint/blob/7aaf0377343f63dca87d8007fd404253a44598b5/lib/mint/http2.ex#L2094

  defp send!(%Mint.HTTP2{transport: transport, socket: socket} = conn, bytes) do
    case transport.send(socket, bytes) do
      :ok ->
        conn

      {:error, %TransportError{reason: :closed} = error} ->
        throw({:mint, %{conn | state: :closed}, error})

      {:error, reason} ->
        throw({:mint, conn, reason})
    end
  end

And there are problem, transport.send(socket, bytes) returns TransportError then you just throw this error and because of it we ignore this command _ = conn.transport.close(conn.socket) at send_connection_error!/3 and the socket is not closed correctly

  defp send_connection_error!(conn, error_code, debug_data) do
    frame =
      goaway(stream_id: 0, last_stream_id: 2, error_code: error_code, debug_data: debug_data)

    conn = send!(conn, Frame.encode(frame))
    _ = conn.transport.close(conn.socket) # <<<--- this line ignored because send! explodes
    conn = put_in(conn.state, :closed)
    throw({:mint, conn, wrap_error({error_code, debug_data})})
  end

I hope I made my point clear. Looking at the code, I see that you wanted to handle such extreme edge cases, but something went wrong 😂

[whatyouhide]

@bitberry-dev awesome catch! Any chance you'd be able to send a PR to fix this? 🙃

[bitberry-dev]

@whatyouhide Yep, I can send a quick fix like below - just ensure connection closed

defp send_connection_error!(conn, error_code, debug_data) do
  frame =
    goaway(stream_id: 0, last_stream_id: 2, error_code: error_code, debug_data: debug_data)

  try do
    conn = send!(conn, Frame.encode(frame))
  after
    _ = conn.transport.close(conn.socket)
  end

  conn = put_in(conn.state, :closed)
  throw({:mint, conn, wrap_error({error_code, debug_data})})
end

But this is a weak solution and I would not want to implement it, judging by the bug - the code needs to be refactored. And other methods that use send_connection_error! should be checked. I can also do this in my free time, the only thing I need is a mint core developer with whom I could consult on certain architectural decisions. Do you have slack?

[whatyouhide]

@bitberry-dev yes, you can reach out to me on the Elixir Slack (@whatyouhide there too I believe). 🙃

[bitberry-dev]

So, I looked at the code and was a little surprised by the widespread use of throw. Initially, I wanted to try to abandon throw altogether and control execution more explicitly, this would help to avoid a whole class of errors such as described in the issue, but this will require a deep dive into these 2k+ lines of code and take a lot of time, and, most importantly, it is not clear whether this is necessary at all (obviously, this style of code was used intentionally) :)

In the end, I just wrote a couple of tests and made a simple fix. Added a pull request, take a look 😉