From a700368b2cb021bb1cd007697db4a3bae0099f2d Mon Sep 17 00:00:00 2001
From: James Fish <james@fishcakez.com>
Date: Mon, 1 Apr 2013 23:15:45 +0100
Subject: Fix ranch_server:set_connections_sup/2 race conditions

A ranch_conns_sup could be (re)started and call
ranch_server:set_connections_sup/2 before ranch_server has handled the
predecessor's exit. This would cause the ranch_server to crash because
ets:insert_new/2 would return false.

This change allows ranch_server to handle this case by crashing the
calling process instead of itself.
---
 src/ranch_server.erl    | 15 ++++++++++-----
 test/acceptor_SUITE.erl | 24 +++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/src/ranch_server.erl b/src/ranch_server.erl
index 7557d53..200e29c 100644
--- a/src/ranch_server.erl
+++ b/src/ranch_server.erl
@@ -68,7 +68,8 @@ cleanup_listener_opts(Ref) ->
 %% @doc Set a connection supervisor associated with specific listener.
 -spec set_connections_sup(any(), pid()) -> ok.
 set_connections_sup(Ref, Pid) ->
-	gen_server:call(?MODULE, {set_connections_sup, Ref, Pid}).
+	true = gen_server:call(?MODULE, {set_connections_sup, Ref, Pid}),
+	ok.
 
 %% @doc Return the connection supervisor used by specific listener.
 -spec get_connections_sup(any()) -> pid().
@@ -125,10 +126,14 @@ handle_call({set_new_listener_opts, Ref, MaxConns, Opts}, _, State) ->
 	{reply, ok, State};
 handle_call({set_connections_sup, Ref, Pid}, _,
 		State=#state{monitors=Monitors}) ->
-	true = ets:insert_new(?TAB, {{conns_sup, Ref}, Pid}),
-	MonitorRef = erlang:monitor(process, Pid),
-	{reply, ok, State#state{
-		monitors=[{{MonitorRef, Pid}, Ref}|Monitors]}};
+	case ets:insert_new(?TAB, {{conns_sup, Ref}, Pid}) of
+		true ->
+			MonitorRef = erlang:monitor(process, Pid),
+			{reply, true,
+				State#state{monitors=[{{MonitorRef, Pid}, Ref}|Monitors]}};
+		false ->
+			{reply, false, State}
+	end;
 handle_call({set_port, Ref, Port}, _, State) ->
 	true = ets:insert(?TAB, {{port, Ref}, Port}),
 	{reply, ok, State};
diff --git a/test/acceptor_SUITE.erl b/test/acceptor_SUITE.erl
index ca8c856..861ea33 100644
--- a/test/acceptor_SUITE.erl
+++ b/test/acceptor_SUITE.erl
@@ -49,6 +49,7 @@
 -export([supervisor_clean_child_restart/1]).
 -export([supervisor_conns_alive/1]).
 -export([supervisor_server_recover_state/1]).
+-export([supervisor_clean_conns_sup_restart/1]).
 
 %% ct.
 
@@ -77,7 +78,8 @@ groups() ->
 		supervisor_clean_restart,
 		supervisor_clean_child_restart,
 		supervisor_conns_alive,
-		supervisor_server_recover_state
+		supervisor_server_recover_state,
+		supervisor_clean_conns_sup_restart
 	]}].
 
 init_per_suite(Config) ->
@@ -499,6 +501,26 @@ supervisor_server_recover_state(_) ->
 	_ = erlang:trace(all, false, [all]),
 	ok = clean_traces().
 
+supervisor_clean_conns_sup_restart(_) ->
+	%% Verify that a conns_sup can not register with the same Name as an already
+	%% registered conns_sup that is still alive. Make sure this does not crash
+	%% the ranch_server.
+	Name = supervisor_clean_conns_sup_restart,
+	{ok, _} = ranch:start_listener(Name,
+		1, ranch_tcp, [{port, 0}], echo_protocol, []),
+	Server = erlang:whereis(ranch_server),
+	ServerMonRef = erlang:monitor(process, Server),
+	%% Exit because Name already registered and is alive.
+	{'EXIT', _}  = (catch ranch_server:set_connections_sup(Name, self())),
+	receive
+		{'DOWN', ServerMonRef, process, Server, _} ->
+			error(ranch_server_down)
+	after
+		1000 ->
+			ok
+	end,
+	ranch:stop_listener(Name).
+
 %% Utility functions.
 
 connect_loop(_, 0, _) ->
-- 
cgit v1.2.3