ssl: Buffer unsent encrypted data on send timeout

When using gen_tcp with {inet_backend, socket} and send_timeout,
gen_tcp:send may return {error, {timeout, RestData}} with the unsent
encrypted data. Previously this fell through to the generic error
handler which killed the connection.

Buffer the RestData in a new #sync{} record in the tls_sender state
and retry sending it together with new data on the next ssl:send call.
Reply {error, timeout} to the caller to simulate {inet_backend, inet}
behavior.

When alerts, post-handshake data or renegotiation need to send while
a #sync{} buffer exists, attempt to flush the buffer first. If the
flush succeeds, proceed normally. If it times out again, postpone the
event and re-buffer the remaining data.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Author: zuiderkwast

lib/ssl/src/tls_sender.erl

@@ -91,6 +91,13 @@
          low   = undefined
         }).
 
+%% Buffer for unsent encrypted data returned by gen_tcp:send
+%% as {error, {timeout, RestData}} when using {inet_backend, socket}
+-record(sync,
+        {
+         q_rev = []    %% Remaining encrypted data (iodata)
+        }).
+
 -define(IS_ASYNC(Tag), Tag =:= select; Tag =:= completion).
 
 %%%===================================================================
@@ -285,24 +292,49 @@ connection({call, From}, {post_handshake_data, HSData}, #data{buff = Buff} = Sta
     case Buff of
         undefined ->
             send_post_handshake_data(HSData, From, connection, StateData, [{reply, From, ok}]);
-        Async ->
-            {next_state, async_wait, StateData#data{buff = Async#async{low = 0}}, [postpone]}
+        #async{} = Async ->
+            {next_state, async_wait, StateData#data{buff = Async#async{low = 0}}, [postpone]};
+        #sync{} ->
+            StateData1 = flush_sync_buffer(StateData),
+            case StateData1#data.buff of
+                undefined ->
+                    send_post_handshake_data(HSData, From, connection, StateData1, [{reply, From, ok}]);
+                #sync{} ->
+                    {keep_state, StateData1, [postpone]}
+            end
     end;
 connection({call, From}, {ack_alert, #alert{} = Alert}, #data{buff = Buff} = StateData0) ->
     case Buff of
         undefined ->
             StateData = send_tls_alert(Alert, StateData0),
             {next_state, connection, StateData, [{reply,From,ok}]};
-        Async ->
-            {next_state, async_wait, StateData0#data{buff = Async#async{low = 0}}, [postpone]}
+        #async{} = Async ->
+            {next_state, async_wait, StateData0#data{buff = Async#async{low = 0}}, [postpone]};
+        #sync{} ->
+            StateData1 = flush_sync_buffer(StateData0),
+            case StateData1#data.buff of
+                undefined ->
+                    StateData = send_tls_alert(Alert, StateData1),
+                    {next_state, connection, StateData, [{reply,From,ok}]};
+                #sync{} ->
+                    {keep_state, StateData1, [postpone]}
+            end
     end;
 connection({call, From}, renegotiate,
            #data{connection_states = #{current_write := Write}, buff = Buff} = StateData) ->
     case Buff of
         undefined ->
             {next_state, handshake, StateData, [{reply, From, {ok, Write}}]};
-        Async ->
-            {next_state, async_wait, StateData#data{buff = Async#async{low = 0}}, [postpone]}
+        #async{} = Async ->
+            {next_state, async_wait, StateData#data{buff = Async#async{low = 0}}, [postpone]};
+        #sync{} ->
+            StateData1 = flush_sync_buffer(StateData),
+            case StateData1#data.buff of
+                undefined ->
+                    {next_state, handshake, StateData1, [{reply, From, {ok, Write}}]};
+                #sync{} ->
+                    {keep_state, StateData1, [postpone]}
+            end
     end;
 connection({call, From}, downgrade, #data{connection_states =
                                               #{current_write := Write}} = StateData) ->
@@ -329,17 +361,34 @@ connection(internal, {post_handshake_data, From, HSData}, #data{buff = Buff} = S
      case Buff of
          undefined ->
              send_post_handshake_data(HSData, From, connection, StateData, []);
-         Async ->
-             {next_state, async_wait, StateData#data{buff = Async#async{low = 0}}, [postpone]}
+         #async{} = Async ->
+             {next_state, async_wait, StateData#data{buff = Async#async{low = 0}}, [postpone]};
+         #sync{} ->
+             StateData1 = flush_sync_buffer(StateData),
+             case StateData1#data.buff of
+                 undefined ->
+                     send_post_handshake_data(HSData, From, connection, StateData1, []);
+                 #sync{} ->
+                     {keep_state, StateData1, [postpone]}
+             end
      end;
 
 connection(cast, #alert{} = Alert,  #data{buff = Buff} = StateData0) ->
      case Buff of
          undefined ->
              StateData = send_tls_alert(Alert, StateData0),
              {next_state, connection, StateData};
-         Async ->
-             {next_state, async_wait, StateData0#data{buff = Async#async{low = 0}}, [postpone]}
+         #async{} = Async ->
+             {next_state, async_wait, StateData0#data{buff = Async#async{low = 0}}, [postpone]};
+         #sync{} ->
+             StateData1 = flush_sync_buffer(StateData0),
+             case StateData1#data.buff of
+                 undefined ->
+                     StateData = send_tls_alert(Alert, StateData1),
+                     {next_state, connection, StateData};
+                 #sync{} ->
+                     {keep_state, StateData1, [postpone]}
+             end
      end;
 connection(cast, {new_write, WritesState, Version, MaxFragLen},
            #data{connection_states = ConnectionStates0, env = Env} = StateData) ->
@@ -589,6 +638,12 @@ send_or_buffer(Transport, Socket, Msgs, From, #data{buff = undefined} = StateDat
         ok ->
             send_reply(From, ok),
             {ok, StateData0};
+        {error, {timeout, RestData}} ->
+            %% gen_tcp:send with {inet_backend, socket} returns unsent
+            %% encrypted data on timeout. Buffer it for retry on next send.
+            %% Reply {error, timeout} to simulate {inet_backend, inet} behavior.
+            send_reply(From, {error, timeout}),
+            {ok, StateData0#data{buff = #sync{q_rev = RestData}}};
         {error, timeout} = Error ->
             %% This clause is to retain some backwards compatibility with
             %% inet-driver behavior for gen_tcp:send timeout. That
@@ -626,7 +681,25 @@ send_or_buffer(Transport, Socket, Msgs, From, #data{buff = undefined} = StateDat
                     {block, StateData0#data{buff = Async#async{reply_to = From}}}
             end
     end;
-%% Buffer exists, push more data to buffer
+%% Timeout buffer exists, flush buffered data together with new data.
+%% Transport is gen_tcp (not tls_socket_tcp) so only sync results.
+send_or_buffer(Transport, Socket, Msgs, From,
+               #data{buff = #sync{q_rev = BuffData}} = StateData0) ->
+    case tls_socket:send(Transport, Socket, [BuffData | Msgs], nowait) of
+        ok ->
+            send_reply(From, ok),
+            {ok, StateData0#data{buff = undefined}};
+        {error, {timeout, RestData}} ->
+            send_reply(From, {error, timeout}),
+            {ok, StateData0#data{buff = #sync{q_rev = RestData}}};
+        {error, timeout} = Error ->
+            send_reply(From, Error),
+            {ok, StateData0#data{buff = undefined}};
+        {error, _Err} = Error ->
+            send_reply(From, Error),
+            Error
+    end;
+%% Async buffer exists, push more data to buffer
 send_or_buffer(_Transport, _Socket, Msgs, From, #data{buff = Async0} = StateData) ->
     #async{high = High, size = Sz0, q_rev = Q} = Async0,
     Sz = Sz0 + iolist_size(Msgs),
@@ -639,6 +712,22 @@ send_or_buffer(_Transport, _Socket, Msgs, From, #data{buff = Async0} = StateData
             {block, StateData#data{buff = Async#async{reply_to = From}}}
     end.
 
+%% Try to flush the #sync{} buffer. Returns the updated #data{}
+%% with buff set to undefined on success, or a new #sync{} on timeout.
+flush_sync_buffer(#data{env = #env{socket = Socket,
+                                   transport_cb = Transport},
+                        buff = #sync{q_rev = BuffData}} = StateData) ->
+    case tls_socket:send(Transport, Socket, BuffData, nowait) of
+        ok ->
+            StateData#data{buff = undefined};
+        {error, {timeout, RestData}} ->
+            StateData#data{buff = #sync{q_rev = RestData}};
+        {error, timeout} ->
+            StateData#data{buff = undefined};
+        {error, _} ->
+            StateData#data{buff = undefined}
+    end.
+
 do_async_send(_Transport, _Socket, _Handle, _Nextstate, {error, Err} = Error,
               #data{buff = #async{reply_to = From}} = StateData) ->
     send_reply(From, Error),

lib/ssl/test/ssl_api_SUITE.erl

@@ -63,6 +63,8 @@
          select_sha1_cert/1,
          inet_backend_option_order/0,
          inet_backend_option_order/1,
+         send_timeout_buffering/0,
+         send_timeout_buffering/1,
          root_any_sign/0,
          root_any_sign/1,
          connection_information/0,
@@ -227,6 +229,8 @@
          protocol_version_check/2,
          suite_check/2,
          check_peercert/2,
+         send_timeout_sink/1,
+         send_timeout_fill/1,
          %%TODO Keep?
          run_error_server/1,
          run_client_error/1
@@ -264,9 +268,11 @@ groups() ->
      {'tlsv1.1', [parallel],  gen_api_tests() ++ handshake_paus_tests() ++ pre_1_3() ++ pre_1_2()},
      {'tlsv1', [parallel],  gen_api_tests() ++ handshake_paus_tests() ++ pre_1_3() ++ pre_1_2() ++
           beast_mitigation_test()},
-     {'dtlsv1.2', [parallel], gen_api_tests() -- [new_options_in_handshake, hibernate_server] ++
+     {'dtlsv1.2', [parallel], gen_api_tests() -- [new_options_in_handshake, hibernate_server,
+                                                   send_timeout_buffering] ++
           handshake_paus_tests() -- [handshake_continue_tls13_client] ++ pre_1_3()},
-     {'dtlsv1', [parallel],  gen_api_tests() -- [new_options_in_handshake, hibernate_server] ++
+     {'dtlsv1', [parallel],  gen_api_tests() -- [new_options_in_handshake, hibernate_server,
+                                                   send_timeout_buffering] ++
           handshake_paus_tests() -- [handshake_continue_tls13_client] ++ pre_1_3() ++ pre_1_2()},
      {transport_socket,  [parallel], gen_api_tests() -- [ssl_not_started, dh_params]}
     ].
@@ -305,6 +311,7 @@ gen_api_tests() ->
      peercert_with_client_cert,
      select_sha1_cert,
      inet_backend_option_order,
+     send_timeout_buffering,
      connection_information,
      secret_connection_info,
      keylog_connection_info,
@@ -673,6 +680,73 @@ inet_backend_option_order(Config) when is_list(Config) ->
     ssl_test_lib:close(Server),
     ssl_test_lib:close(Client).
 
+%%--------------------------------------------------------------------
+send_timeout_buffering() ->
+    [{doc,"Test that ssl buffers unsent encrypted data on send timeout "
+      "when using {inet_backend, socket} and retries on next send"}].
+send_timeout_buffering(Config) when is_list(Config) ->
+    ClientOpts = ssl_test_lib:ssl_options(client_rsa_verify_opts, Config),
+    ServerOpts = ssl_test_lib:ssl_options(server_rsa_opts, Config),
+    {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
+    Server = ssl_test_lib:start_server([{node, ServerNode}, {port, 0},
+                                        {from, self()},
+                                        {mfa, {?MODULE, send_timeout_sink, []}},
+                                        {options, [{inet_backend, socket},
+                                                   {active, false}
+                                                   | ServerOpts]}]),
+    Port = ssl_test_lib:inet_port(Server),
+    Client = ssl_test_lib:start_client([{node, ClientNode}, {port, Port},
+                                        {host, Hostname},
+                                        {from, self()},
+                                        {mfa, {?MODULE, send_timeout_fill, []}},
+                                        {options, [{inet_backend, socket},
+                                                   {active, false},
+                                                   {sndbuf, 4096},
+                                                   {send_timeout, 1}
+                                                   | ClientOpts]}]),
+
+    ssl_test_lib:check_result(Server, ok, Client, ok),
+
+    ssl_test_lib:close(Server),
+    ssl_test_lib:close(Client).
+
+send_timeout_sink(Socket) ->
+    %% Server side: register, wait for signal, drain, signal back
+    register(send_timeout_sink_server, self()),
+    receive start_recv -> ok end,
+    send_timeout_recv_loop(Socket),
+    send_timeout_sink_client ! drained,
+    ok.
+
+send_timeout_recv_loop(Socket) ->
+    case ssl:recv(Socket, 0, 1000) of
+        {ok, _} -> send_timeout_recv_loop(Socket);
+        {error, timeout} -> ok;
+        {error, closed} -> ok
+    end.
+
+send_timeout_fill(Socket) ->
+    %% Client side: fill buffer, signal server, wait for drain, send again
+    register(send_timeout_sink_client, self()),
+    Data = <<0:(1024*8)>>,
+    send_timeout_fill_loop(Socket, Data, 0).
+
+send_timeout_fill_loop(Socket, Data, N) ->
+    case ssl:send(Socket, Data) of
+        ok ->
+            send_timeout_fill_loop(Socket, Data, N + 1);
+        {error, timeout} when N > 0 ->
+            %% Buffer filled. Signal server to start draining.
+            send_timeout_sink_server ! start_recv,
+            %% Wait for server to finish draining.
+            receive drained -> ok end,
+            %% Verify connection is still usable.
+            ok = ssl:send(Socket, <<"still alive">>),
+            ok;
+        {error, _} = Error ->
+            Error
+    end.
+
 %%--------------------------------------------------------------------
 connection_information() ->
     [{doc,"Test the API function ssl:connection_information/1"}].