diff --git a/src/hackney_conn.erl b/src/hackney_conn.erl index a38b67cd..625fb41c 100644 --- a/src/hackney_conn.erl +++ b/src/hackney_conn.erl @@ -898,7 +898,7 @@ connected({call, From}, is_ready, #conn_data{transport = Transport, socket = Soc connected({call, From}, {upgrade_to_ssl, _SslOpts, _UpgradeOpts}, #conn_data{transport = hackney_ssl} = _Data) -> %% Already SSL - no upgrade needed {keep_state_and_data, [{reply, From, ok}]}; -connected({call, From}, {upgrade_to_ssl, SslOpts, UpgradeOpts}, #conn_data{socket = Socket, host = Host, connect_options = ConnectOpts} = Data) -> +connected({call, From}, {upgrade_to_ssl, SslOpts, UpgradeOpts}, #conn_data{socket = Socket, host = Host, connect_options = ConnectOpts, connect_timeout = HandshakeTimeout} = Data) -> %% Upgrade TCP socket to SSL (e.g., after CONNECT proxy tunnel) FinalSslOpts = case maps:get(final, UpgradeOpts, false) of true -> @@ -926,7 +926,11 @@ connected({call, From}, {upgrade_to_ssl, SslOpts, UpgradeOpts}, #conn_data{socke Resumable = hackney_ssl:auto_tickets(FinalSslOpts), Cached = hackney_ssl:recall_alpn(Host, AlpnProtos), GatedSslOpts = gate_resumption(FinalSslOpts, Cached), - case ssl:connect(Socket, GatedSslOpts) of + %% Bound the pooled TLS upgrade handshake with the connection's + %% connect_timeout. ssl:connect/2 has no handshake deadline, so a server + %% that stalls after TCP accept would pin this process and its pool slot + %% indefinitely, past connect_timeout/recv_timeout. + case ssl:connect(Socket, GatedSslOpts, HandshakeTimeout) of {ok, SslSocket} -> %% Detect negotiated protocol, carrying ALPN across resumption Protocol = hackney_ssl:negotiated_protocol(SslSocket, Host, AlpnProtos, Cached, Resumable), diff --git a/test/hackney_conn_upgrade_timeout_tests.erl b/test/hackney_conn_upgrade_timeout_tests.erl new file mode 100644 index 00000000..66a91fcb --- /dev/null +++ b/test/hackney_conn_upgrade_timeout_tests.erl @@ -0,0 +1,35 @@ +%%% Pooled TLS upgrade must not hang on a stalled handshake. +%%% +%%% ssl:connect/2 has no handshake deadline, so a peer that accepts TCP but +%%% never completes the TLS handshake would pin the connection process (and its +%%% pool slot) forever. The upgrade must be bounded by connect_timeout. +-module(hackney_conn_upgrade_timeout_tests). + +-include_lib("eunit/include/eunit.hrl"). + +pooled_tls_upgrade_times_out_test_() -> + %% Without the bound the upgrade never returns and this test times out. + {timeout, 10, fun pooled_tls_upgrade_times_out/0}. + +pooled_tls_upgrade_times_out() -> + {ok, _} = application:ensure_all_started(hackney), + %% Listener that accepts the TCP connection but never speaks TLS. + {ok, LSock} = gen_tcp:listen(0, [binary, {active, false}, {ip, {127, 0, 0, 1}}]), + {ok, Port} = inet:port(LSock), + %% Let the conn open (and thus own) its own TCP socket to the stalled peer. + Opts = #{host => "127.0.0.1", port => Port, transport => hackney_tcp, + connect_timeout => 500}, + {ok, Pid} = hackney_conn:start_link(Opts), + ok = hackney_conn:connect(Pid, 1000), + {ok, _ServerSock} = gen_tcp:accept(LSock, 1000), + ?assertEqual({ok, connected}, hackney_conn:get_state(Pid)), + %% verify_none so the handshake proceeds and then stalls waiting for the + %% ServerHello that never arrives; only the timeout can end it. + T0 = erlang:monotonic_time(millisecond), + Result = hackney_conn:upgrade_to_ssl(Pid, [{verify, verify_none}], #{final => true}), + Elapsed = erlang:monotonic_time(millisecond) - T0, + ?assertMatch({error, _}, Result), + %% 500ms bound with generous slack; a regression hangs instead. + ?assert(Elapsed < 4000), + catch hackney_conn:stop(Pid), + gen_tcp:close(LSock).