Move most transport_opt() validation into diameter_config

Faulty configuration was previously passed directly on to watchdog and
peer_fsm processes, diameter:add_transport/2 happily returning ok and
the error resulting on failure of watchdog and/or peer_fsm processes.
Now check for errors before getting this far, returning {error, Reason}
from diameter:add_transport/2 when one is detected. There are still
some errors that can only be detected after transport start (eg. a
misbehaving callback) but most will be caught early.

3 files changed, 67 insertions, 12 deletions

diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl
index 3a2e0d2140..899c909ebe 100644
--- a/lib/diameter/src/base/diameter_config.erl
+++ b/lib/diameter/src/base/diameter_config.erl
@@ -103,6 +103,10 @@
 %% Time to lay low before restarting a dead service.
 -define(RESTART_SLEEP, 2000).
 
+%% Test for a valid timeout.
+-define(IS_UINT32(N),
+        is_integer(N) andalso 0 =< N andalso 0 == N bsr 32).
+
 %% A minimal diameter_caps for checking for valid capabilities values.
 -define(EXAMPLE_CAPS,
         #diameter_caps{origin_host = "TheHost",
@@ -490,13 +494,11 @@ stop(SvcName) ->
 %% has many.
 
 add(SvcName, Type, Opts) ->
-    %% Ensure usable capabilities. diameter_service:merge_service/2
-    %% depends on this.
-    lists:foreach(fun(Os) ->
-                          is_list(Os) orelse ?THROW({capabilities, Os}),
-                          ok = encode_CER(Os)
-                  end,
-                  [Os || {capabilities, Os} <- Opts, is_list(Os)]),
+    %% Ensure acceptable transport options. This won't catch all
+    %% possible errors (faulty callbacks for example) but it catches
+    %% many. diameter_service:merge_service/2 depends on usable
+    %% capabilities for example.
+    ok = transport_opts(Opts),
 
     Ref = make_ref(),
     T = {Ref, Type, Opts},
@@ -514,6 +516,61 @@ add(SvcName, Type, Opts) ->
             No
     end.
 
+transport_opts(Opts) ->
+    lists:foreach(fun(T) -> opt(T) orelse ?THROW({invalid, T}) end, Opts).
+
+opt({transport_module, M}) ->
+    is_atom(M);
+
+opt({transport_config, _, Tmo}) ->
+    ?IS_UINT32(Tmo) orelse Tmo == infinity;
+
+opt({applications, As}) ->
+    is_list(As);
+
+opt({capabilities, Os}) ->
+    is_list(Os) andalso ok == encode_CER(Os);
+
+opt({capx_timeout, Tmo}) ->
+    ?IS_UINT32(Tmo);
+
+opt({length_errors, T}) ->
+    lists:member(T, [exit, handle, discard]);
+
+opt({reconnect_timer, Tmo}) ->
+    ?IS_UINT32(Tmo);
+
+opt({watchdog_timer, {M,F,A}})
+  when is_atom(M), is_atom(F), is_list(A) ->
+    true;
+opt({watchdog_timer, Tmo}) ->
+    ?IS_UINT32(Tmo);
+
+opt({watchdog_config, L}) ->
+    is_list(L) andalso lists:all(fun wdopt/1, L);
+
+%% Options that we can't validate.
+opt({K, _})
+  when K == transport_config;
+       K == capabilities_cb;
+       K == disconnect_cb;
+       K == private ->
+    true;
+
+%% Anything else, which is ignored by us. This makes options sensitive
+%% to spelling mistakes but arbitrary options are passed by some users
+%% as a way to identify transports. (That is, can't just do away with
+%% it.)
+opt(_) ->
+    true.
+
+wdopt({K,N}) ->
+    (K == okay orelse K == suspect) andalso is_integer(N) andalso 0 =< N;
+wdopt(_) ->
+    false.
+
+%% start_transport/2
+
 start_transport(SvcName, T) ->
     case diameter_service:start_transport(SvcName, T) of
         {ok, _Pid} ->
diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl
index 66342f7b62..3b8c49b7e8 100644
--- a/lib/diameter/src/base/diameter_peer_fsm.erl
+++ b/lib/diameter/src/base/diameter_peer_fsm.erl
@@ -198,6 +198,7 @@ i({Ack, WPid, {M, Ref} = T, Opts, {Mask,
     OnLengthErr = proplists:get_value(length_errors, Opts, exit),
     lists:member(OnLengthErr, [exit, handle, discard])
         orelse ?ERROR({invalid, {length_errors, OnLengthErr}}),
+    %% Error checking is for configuration added in old code.
 
     {TPid, Addrs} = start_transport(T, Rest, Svc),
 
@@ -212,9 +213,6 @@ i({Ack, WPid, {M, Ref} = T, Opts, {Mask,
 %% transports on the same service can use different local addresses.
 %% The local addresses are put into Host-IP-Address avps here when
 %% sending capabilities exchange messages.
-%%
-%% Invalid transport config may cause us to crash but note that the
-%% watchdog start (start/2) succeeds regardless.
 
 %% Wait for the caller to have a monitor to avoid a race with our
 %% death. (Since the exit reason is used in diameter_service.)
diff --git a/lib/diameter/src/base/diameter_watchdog.erl b/lib/diameter/src/base/diameter_watchdog.erl
index 82ca603cf3..3cbf91c574 100644
--- a/lib/diameter/src/base/diameter_watchdog.erl
+++ b/lib/diameter/src/base/diameter_watchdog.erl
@@ -158,7 +158,7 @@ wait(Ref, Pid) ->
 config(Opts) ->
     Config = proplists:get_value(watchdog_config, Opts, []),
     is_list(Config) orelse config_error({watchdog_config, Config}),
-    lists:foldl(fun config/2, #config{}, Config).
+    lists:foldl(fun config/2, #config{}, Config).      %% ^ added in old code
 
 config({suspect, N}, Rec)
   when ?IS_NATURAL(N) ->
@@ -168,7 +168,7 @@ config({okay, N}, Rec)
   when ?IS_NATURAL(N) ->
     Rec#config{okay = N};
 
-config(T, _) ->
+config(T, _) ->  %% added in old code
     config_error(T).
 
 %% start/5