]> granicus.if.org Git - ejabberd/commitdiff
Stop ejabberd initialization on invalid/unknown options
authorEvgeniy Khramtsov <ekhramtsov@process-one.net>
Wed, 9 May 2018 08:44:24 +0000 (11:44 +0300)
committerEvgeniy Khramtsov <ekhramtsov@process-one.net>
Wed, 9 May 2018 08:44:24 +0000 (11:44 +0300)
Since now, ejabberd doesn't ignore unknown options and doesn't
allow to have options with malformed values. The rationale for
this is to avoid unexpected behaviour during runtime, i.e. to
conform to "fail early" approach. Note that it's safe to reload
a configuration with potentialy invalid and/or unknown options:
this will not halt ejabberd, but will only prevent the configuration
from loading.

***NOTE FOR PACKAGE BUILDERS***
This new behaviour should be documented in the upgrade notes.

src/ejabberd_app.erl
src/ejabberd_config.erl
src/gen_mod.erl

index f1066815fa09671232160de878e09cfdc8a3b85b..c7486a5bec9fea39c95aaa114b41a1e06d089f2f 100644 (file)
@@ -46,22 +46,27 @@ start(normal, _Args) ->
     start_elixir_application(),
     ejabberd:check_app(ejabberd),
     setup_if_elixir_conf_used(),
-    ejabberd_config:start(),
-    ejabberd_mnesia:start(),
-    file_queue_init(),
-    maybe_add_nameservers(),
-    ejabberd_system_monitor:start(),
-    case ejabberd_sup:start_link() of
-       {ok, SupPid} ->
-           register_elixir_config_hooks(),
-           ejabberd_cluster:wait_for_sync(infinity),
-           {T2, _} = statistics(wall_clock),
-           ?INFO_MSG("ejabberd ~s is started in the node ~p in ~.2fs",
-                     [?VERSION, node(), (T2-T1)/1000]),
-           lists:foreach(fun erlang:garbage_collect/1, processes()),
-           {ok, SupPid};
-       Err ->
-           ?CRITICAL_MSG("Failed to start ejabberd application: ~p", [Err]),
+    case ejabberd_config:start() of
+       ok ->
+           ejabberd_mnesia:start(),
+           file_queue_init(),
+           maybe_add_nameservers(),
+           ejabberd_system_monitor:start(),
+           case ejabberd_sup:start_link() of
+               {ok, SupPid} ->
+                   register_elixir_config_hooks(),
+                   ejabberd_cluster:wait_for_sync(infinity),
+                   {T2, _} = statistics(wall_clock),
+                   ?INFO_MSG("ejabberd ~s is started in the node ~p in ~.2fs",
+                             [?VERSION, node(), (T2-T1)/1000]),
+                   lists:foreach(fun erlang:garbage_collect/1, processes()),
+                   {ok, SupPid};
+               Err ->
+                   ?CRITICAL_MSG("Failed to start ejabberd application: ~p", [Err]),
+                   ejabberd:halt()
+           end;
+       {error, Reason} ->
+           ?CRITICAL_MSG("Failed to start ejabberd application: ~p", [Reason]),
            ejabberd:halt()
     end;
 start(_, _) ->
index 3f88df59aedbf4d7ac1acf204eb99a236ddb6811..3fe4d9c715cddf567cbdb31151bbed8baaa2d94e 100644 (file)
 -include_lib("stdlib/include/ms_transform.hrl").
 
 -callback opt_type(atom()) -> function() | [atom()].
+-type bad_option() :: invalid_option | unknown_option.
 
-%% @type macro() = {macro_key(), macro_value()}
-
-%% @type macro_key() = atom().
-%% The atom must have all characters in uppercase.
-
-%% @type macro_value() = term().
-
+-spec start() -> ok | {error, bad_option()}.
 start() ->
     ConfigFile = get_ejabberd_config_path(),
     ?INFO_MSG("Loading configuration from ~s", [ConfigFile]),
@@ -75,17 +70,23 @@ start() ->
                  [named_table, public, {read_concurrency, true}]),
     catch ets:new(ejabberd_db_modules,
                  [named_table, public, {read_concurrency, true}]),
-    State1 = load_file(ConfigFile),
-    UnixTime = p1_time_compat:system_time(seconds),
-    SharedKey = case erlang:get_cookie() of
-                    nocookie ->
-                        str:sha(randoms:get_string());
-                    Cookie ->
-                        str:sha(misc:atom_to_binary(Cookie))
-                end,
-    State2 = set_option({node_start, global}, UnixTime, State1),
-    State3 = set_option({shared_key, global}, SharedKey, State2),
-    set_opts(State3).
+    case load_file(ConfigFile) of
+       {ok, State1} ->
+           UnixTime = p1_time_compat:system_time(seconds),
+           SharedKey = case erlang:get_cookie() of
+                           nocookie ->
+                               str:sha(randoms:get_string());
+                           Cookie ->
+                               str:sha(misc:atom_to_binary(Cookie))
+                       end,
+           State2 = set_option({node_start, global}, UnixTime, State1),
+           State3 = set_option({shared_key, global}, SharedKey, State2),
+           set_opts(State3),
+           ok;
+       {error, _} = Err ->
+           ?ERROR_MSG("Failed to load configuration file ~s", [ConfigFile]),
+           Err
+    end.
 
 %% When starting ejabberd for testing, we sometimes want to start a
 %% subset of hosts from the one define in the config file.
@@ -189,7 +190,7 @@ read_file(File, Opts) ->
     State1 = lists:foldl(fun process_term/2, State, Head ++ Tail),
     State1#state{opts = compact(State1#state.opts)}.
 
--spec load_file(string()) -> #state{}.
+-spec load_file(string()) -> {ok, #state{}} | {error, bad_option()}.
 
 load_file(File) ->
     State0 = read_file(File),
@@ -199,23 +200,27 @@ load_file(File) ->
     ModOpts = get_modules_with_options(AllMods),
     validate_opts(State1, ModOpts).
 
--spec reload_file() -> ok.
+-spec reload_file() -> ok | {error, bad_option()}.
 
 reload_file() ->
     Config = get_ejabberd_config_path(),
     OldHosts = get_myhosts(),
-    State = load_file(Config),
-    set_opts(State),
-    NewHosts = get_myhosts(),
-    lists:foreach(
-      fun(Host) ->
-             ejabberd_hooks:run(host_up, [Host])
-      end, NewHosts -- OldHosts),
-    lists:foreach(
-      fun(Host) ->
-             ejabberd_hooks:run(host_down, [Host])
-      end, OldHosts -- NewHosts),
-    ejabberd_hooks:run(config_reloaded, []).
+    case load_file(Config) of
+       {error, _} = Err ->
+           Err;
+       {ok, State} ->
+           set_opts(State),
+           NewHosts = get_myhosts(),
+           lists:foreach(
+             fun(Host) ->
+                     ejabberd_hooks:run(host_up, [Host])
+             end, NewHosts -- OldHosts),
+           lists:foreach(
+             fun(Host) ->
+                     ejabberd_hooks:run(host_down, [Host])
+             end, OldHosts -- NewHosts),
+           ejabberd_hooks:run(config_reloaded, [])
+    end.
 
 -spec convert_to_yaml(file:filename()) -> ok | {error, any()}.
 
@@ -1017,33 +1022,39 @@ get_modules_with_options(Modules) ->
              end
       end, dict:new(), Modules).
 
+-spec validate_opts(#state{}, dict:dict()) -> {ok, #state{}} | {error, bad_option()}.
 validate_opts(#state{opts = Opts} = State, ModOpts) ->
-    NewOpts = lists:filtermap(
-               fun(#local_config{key = {Opt, _Host}, value = Val} = In) ->
-                       case dict:find(Opt, ModOpts) of
-                           {ok, [Mod|_]} ->
-                               VFun = Mod:opt_type(Opt),
-                               try VFun(Val) of
-                                   NewVal ->
-                                       {true, In#local_config{value = NewVal}}
-                               catch {invalid_syntax, Error} ->
-                                       ?ERROR_MSG("ignoring option '~s' with "
-                                                  "invalid value: ~p: ~s",
-                                                  [Opt, Val, Error]),
-                                       false;
-                                     _:_ ->
-                                       ?ERROR_MSG("ignoring option '~s' with "
-                                                  "invalid value: ~p",
-                                                  [Opt, Val]),
-                                       false
-                               end;
-                           _ ->
-                               ?ERROR_MSG("unknown option '~s' will be likely"
-                                          " ignored", [Opt]),
-                               true
-                       end
-               end, Opts),
-    State#state{opts = NewOpts}.
+    try
+       NewOpts = lists:map(
+                   fun(#local_config{key = {Opt, _Host}, value = Val} = In) ->
+                           case dict:find(Opt, ModOpts) of
+                               {ok, [Mod|_]} ->
+                                   VFun = Mod:opt_type(Opt),
+                                   try VFun(Val) of
+                                       NewVal ->
+                                           In#local_config{value = NewVal}
+                                   catch {invalid_syntax, Error} ->
+                                           ?ERROR_MSG("Invalid value '~p' for "
+                                                      "option '~s': ~s",
+                                                      [Val, Opt, Error]),
+                                           erlang:error(invalid_option);
+                                         _:_ ->
+                                           ?ERROR_MSG("Invalid value '~p' for "
+                                                      "option '~s'",
+                                                      [Val, Opt]),
+                                           erlang:error(invalid_option)
+                                   end;
+                               _ ->
+                                   ?ERROR_MSG("Unknown option '~s'", [Opt]),
+                                   erlang:error(unknown_option)
+                           end
+                   end, Opts),
+       {ok, State#state{opts = NewOpts}}
+    catch _:invalid_option ->
+           {error, invalid_option};
+         _:unknown_option ->
+           {error, unknown_option}
+    end.
 
 %% @spec (Path::string()) -> true | false
 is_file_readable(Path) ->
index 1e069be7323b2abbe26e1ad021958d25cc18bcb8..ed04767ca621b9da4b9ea701ab123220f731d243 100644 (file)
@@ -546,89 +546,94 @@ validate_opts(Host, Module, Opts0) ->
                            catch _:undef -> []
                            end
                    end, [Module|SubMods]),
-    Required = lists:filter(fun is_atom/1, DefaultOpts),
     try
        Opts = merge_opts(Opts0, DefaultOpts, Module),
        {ok, case get_validators(Host, {Module, SubMods}) of
                 undef ->
                     Opts;
                 Validators ->
-                    Opts1 = validate_opts(Host, Module, Opts, Required, Validators),
+                    Opts1 = validate_opts(Host, Module, Opts, Validators),
                     remove_duplicated_opts(Opts1)
             end}
     catch _:{missing_required_option, Opt} ->
            ErrTxt = io_lib:format("Module '~s' is missing required option '~s'",
                                   [Module, Opt]),
-           ?ERROR_MSG(ErrTxt, []),
-           {error, ErrTxt}
+           module_error(ErrTxt);
+         _:{invalid_option, Opt, Val} ->
+           ErrTxt = io_lib:format("Invalid value '~p' for option '~s' of "
+                                  "module '~s'", [Val, Opt, Module]),
+           module_error(ErrTxt);
+         _:{invalid_option, Opt, Val, Reason} ->
+           ErrTxt = io_lib:format("Invalid value '~p' for option '~s' of "
+                                  "module '~s': ~s", [Val, Opt, Module, Reason]),
+           module_error(ErrTxt);
+         _:{unknown_option, Opt, []} ->
+           ErrTxt = io_lib:format("Unknown option '~s' of module '~s': "
+                                  "the module doesn't have any options",
+                                  [Opt, Module]),
+           module_error(ErrTxt);
+         _:{unknown_option, Opt, KnownOpts} ->
+           ErrTxt = io_lib:format("Unknown option '~s' of module '~s',"
+                                  " available options are: ~s",
+                                  [Opt, Module,
+                                   misc:join_atoms(KnownOpts, <<", ">>)]),
+           module_error(ErrTxt)
     end.
 
-validate_opts(Host, Module, Opts, Required, Validators) when is_list(Opts) ->
+-spec module_error(iolist()) -> {error, iolist()}.
+module_error(ErrTxt) ->
+    ?ERROR_MSG(ErrTxt, []),
+    {error, ErrTxt}.
+
+-spec err_invalid_option(atom(), any()) -> no_return().
+err_invalid_option(Opt, Val) ->
+    erlang:error({invalid_option, Opt, Val}).
+
+-spec err_invalid_option(atom(), any(), iolist()) -> no_return().
+err_invalid_option(Opt, Val, Reason) ->
+    erlang:error({invalid_option, Opt, Val, Reason}).
+
+-spec err_unknown_option(atom(), [atom()]) -> no_return().
+err_unknown_option(Opt, KnownOpts) ->
+    erlang:error({unknown_option, Opt, KnownOpts}).
+
+-spec err_missing_required_option(atom()) -> no_return().
+err_missing_required_option(Opt) ->
+    erlang:error({missing_required_option, Opt}).
+
+validate_opts(Host, Module, Opts, Validators) when is_list(Opts) ->
     lists:flatmap(
       fun({Opt, Val}) when is_atom(Opt) ->
              case lists:keyfind(Opt, 1, Validators) of
                  {_, L} ->
                      case lists:partition(fun is_function/1, L) of
                          {[VFun|_], []} ->
-                             validate_opt(Module, Opt, Val, Required, VFun);
+                             validate_opt(Opt, Val, VFun);
                          {[VFun|_], SubValidators} ->
-                             try validate_opts(Host, Module, Val, Required, SubValidators) of
+                             try validate_opts(Host, Module, Val, SubValidators) of
                                  SubOpts ->
-                                     validate_opt(Module, Opt, SubOpts, Required, VFun)
+                                     validate_opt(Opt, SubOpts, VFun)
                              catch _:bad_option ->
-                                     ?ERROR_MSG("Ignoring invalid value '~p' for "
-                                                "option '~s' of module '~s'",
-                                                [Val, Opt, Module]),
-                                     fail_if_option_is_required(Opt, Required),
-                                     []
+                                     err_invalid_option(Opt, Val)
                              end
                      end;
                  false ->
-                     case Validators of
-                         [] ->
-                             ?ERROR_MSG("Ignoring unknown option '~s' of '~s':"
-                                        " the module doesn't have any options",
-                                        [Opt, Module]);
-                         _ ->
-                             ?ERROR_MSG("Ignoring unknown option '~s' of '~s',"
-                                        " available options are: ~s",
-                                        [Opt, Module,
-                                         misc:join_atoms(
-                                           [K || {K, _} <- Validators],
-                                           <<", ">>)])
-                     end,
-                     []
+                     err_unknown_option(Opt, [K || {K, _} <- Validators])
              end;
         (_) ->
              erlang:error(bad_option)
       end, Opts);
-validate_opts(_, _, _, _, _) ->
+validate_opts(_, _, _, _) ->
     erlang:error(bad_option).
 
--spec validate_opt(module(), atom(), any(), [atom()],
-                  [{atom(), check_fun(), any()}]) -> [{atom(), any()}].
-validate_opt(Module, Opt, Val, Required, VFun) ->
+-spec validate_opt(atom(), any(), check_fun()) -> [{atom(), any()}].
+validate_opt(Opt, Val, VFun) ->
     try VFun(Val) of
        NewVal -> [{Opt, NewVal}]
     catch {invalid_syntax, Error} ->
-           ?ERROR_MSG("Ignoring invalid value '~p' for "
-                      "option '~s' of module '~s': ~s",
-                      [Val, Opt, Module, Error]),
-           fail_if_option_is_required(Opt, Required),
-           [];
+           err_invalid_option(Opt, Val, Error);
          _:_ ->
-           ?ERROR_MSG("Ignoring invalid value '~p' for "
-                      "option '~s' of module '~s'",
-                      [Val, Opt, Module]),
-           fail_if_option_is_required(Opt, Required),
-           []
-    end.
-
--spec fail_if_option_is_required(atom(), [atom()]) -> ok | no_return().
-fail_if_option_is_required(Opt, Required) ->
-    case lists:member(Opt, Required) of
-       true -> erlang:error({missing_required_option, Opt});
-       false -> ok
+           err_invalid_option(Opt, Val)
     end.
 
 -spec list_known_opts(binary(), module()) -> [atom() | {atom(), atom()}].
@@ -658,11 +663,7 @@ merge_opts(Opts, DefaultOpts, Module) ->
                                      true ->
                                          [{Opt, merge_opts(Val, Default, Module)}|Acc];
                                      false ->
-                                         ?ERROR_MSG(
-                                            "Ignoring invalid value '~p' for "
-                                            "option '~s' of module '~s'",
-                                            [Val, Opt, Module]),
-                                         [{Opt, Default}|Acc]
+                                         err_invalid_option(Opt, Val)
                                  end;
                              Val ->
                                  [{Opt, Default}|Acc];
@@ -677,7 +678,7 @@ merge_opts(Opts, DefaultOpts, Module) ->
                      {_, Val} ->
                          [{Opt, Val}|Acc];
                      false ->
-                         erlang:error({missing_required_option, Opt})
+                         err_missing_required_option(Opt)
                  end
          end, [], DefaultOpts),
     lists:foldl(