Race condition in TLS distribution

Hi all,

I'm trying to use Erlang distribution over TLS ("-proto_dist 
inet_tls"), and I've stumbled upon an interesting race condition.

The kernel supervisor starts the distribution subsystem before it 
starts the code server.  Therefore, it's possible for another node 
to establish a connection to the distribution port while the code 
server is not yet running.  (Apologies for not providing a recipe 
for reproducing this; I could work on that if that would be 
useful.)

In that case, the TLS distribution module eventually calls 
ssl:ssl_accept/2 on the connection socket.  This in turn will 
eventually call crypto:supports/0.  That's when I got this error:

{error_logger,{{2015,10,19},{15,1,22}},
 supervisor_report,
 [{supervisor,{local,ssl_dist_sup}},
  {errorContext,child_terminated},
  {reason,{undef,[{crypto,supports,[],[]},
                  {tls_record,supported_protocol_versions,1,[{file,"tls_record.erl"},{line,322}]},
                  {tls_record,supported_protocol_versions,0, 
                  [{file,"tls_record.erl"},{line,257}]},
                  {ssl,handle_options,1,[{file,"ssl.erl"},{line,617}]},
                  {ssl,ssl_accept,3,[{file,"ssl.erl"},{line,228}]},
                  {ssl_tls_dist_proxy,accept_loop,4,[{file,"ssl_tls_dist_proxy.erl"},{line,152}]}]}},
  {offender,[{pid,<0.22.0>},
             {name,ssl_tls_dist_proxy},
             {mfargs,{ssl_tls_dist_proxy,start_link,[]}},
             {restart_type,permanent},
             {shutdown,4000},
             {child_type,worker}]}]}

(though it was formatted as one long line, using the kernel's 
primitive error reporter.)

Why is that function undefined, you ask.  That's because the 
crypto module has an on_load function, which calls code:priv_dir/1 
to figure out where the NIF library is.  Since the code server 
isn't running yet, code:priv_dir/1 raises an exception, and as I 
just learnt from reading the documentation, if an on_load function 
raises an exception (or returns anything but 'ok'), the module is 
unloaded - and thus we get an 'undef' error.

(This will make the ssl_tls_dist_proxy process terminate.  Its 
supervisor will restart it, but that doesn't help: it has lost its 
listening socket, and net_kernel won't ask it to open another one, 
rendering the node "alive" but unable to receive connections for 
distribution - but that's a separate issue.)

I came up with the attached patch, which waits for the code server 
to start before proceeding, and that fixes the problem for me. 
What do you think about it?  Might there be a better way to solve 
this?

Regards,
Magnus

_______________________________________________
erlang-bugs mailing list
[email protected]
http://erlang.org/mailman/listinfo/erlang-bugs

diff --git a/lib/ssl/src/ssl_tls_dist_proxy.erl b/lib/ssl/src/ssl_tls_dist_proxy.erl
index a22af6b..cc4c410 100644
--- a/lib/ssl/src/ssl_tls_dist_proxy.erl
+++ b/lib/ssl/src/ssl_tls_dist_proxy.erl
@@ -149,6 +149,7 @@ accept_loop(Proxy, world = Type, Listen, Extra) ->
     case gen_tcp:accept(Listen) of
 	{ok, Socket} ->
 	    Opts = get_ssl_options(server),
+	    wait_for_code_server(),
 	    case ssl:ssl_accept(Socket, Opts) of
 		{ok, SslSocket} ->
 		    PairHandler =
@@ -165,6 +166,35 @@ accept_loop(Proxy, world = Type, Listen, Extra) ->
     end,
     accept_loop(Proxy, Type, Listen, Extra).
 
+wait_for_code_server() ->
+    %% This is an ugly hack.  Upgrading a socket to TLS requires the
+    %% crypto module to be loaded.  Loading the crypto module triggers
+    %% its on_load function, which calls code:priv_dir/1 to find the
+    %% directory where its NIF library is.  However, distribution is
+    %% started earlier than the code server, so the code server is not
+    %% necessarily started yet, and code:priv_dir/1 might fail because
+    %% of that, if we receive an incoming connection on the
+    %% distribution port early enough.
+    %%
+    %% If the on_load function of a module fails, the module is
+    %% unloaded, and the function call that triggered loading it fails
+    %% with 'undef', which is rather confusing.
+    %%
+    %% Thus, the ssl_tls_dist_proxy process will terminate, and be
+    %% restarted by ssl_dist_sup.  However, it won't have any memory
+    %% of being asked by net_kernel to listen for incoming
+    %% connections.  Hence, the node will believe that it's open for
+    %% distribution, but it actually isn't.
+    %%
+    %% So let's avoid that by waiting for the code server to start.
+    case whereis(code_server) of
+	undefined ->
+	    timer:sleep(10),
+	    wait_for_code_server();
+	Pid when is_pid(Pid) ->
+	    ok
+    end.
+
 try_connect(Port) ->
     case gen_tcp:connect({127,0,0,1}, Port, [{active, false}, {packet,?PPRE}]) of
 	R = {ok, _S} ->