From ce8433489b6edd5dd9fc91044311ed133fce0854 Mon Sep 17 00:00:00 2001 From: DenysGonchar Date: Fri, 9 Jul 2021 01:57:17 +0200 Subject: [PATCH] allowing only external function references in gen_hook interfaces --- src/ejabberd_hooks.erl | 6 +- src/gen_hook.erl | 147 ++++++++++++++++++++-------------- test/ejabberd_hooks_SUITE.erl | 5 +- test/muc_light_SUITE.erl | 5 +- 4 files changed, 98 insertions(+), 65 deletions(-) diff --git a/src/ejabberd_hooks.erl b/src/ejabberd_hooks.erl index 440c7e7f67..4f86f12be2 100644 --- a/src/ejabberd_hooks.erl +++ b/src/ejabberd_hooks.erl @@ -34,6 +34,8 @@ -export([add/1, delete/1]). +-export([gen_hook_fn_wrapper/3]). + -include("mongoose.hrl"). -type hook() :: {HookName :: atom(), @@ -96,14 +98,14 @@ run_fold(HookName, HostType, Acc, Args) when is_binary(HostType); HostType =:= g -spec add_hook(hook()) -> ok. add_hook({HookName, HostType, Module, Function, Priority}) when is_atom(Function) -> gen_hook:add_handler(HookName, HostType, - fun gen_hook_fn_wrapper/3, + fun ?MODULE:gen_hook_fn_wrapper/3, #{module => Module, function =>Function}, Priority). -spec delete_hook(hook()) -> ok. delete_hook({HookName, HostType, Module, Function, Priority}) when is_atom(Function) -> gen_hook:delete_handler(HookName, HostType, - fun gen_hook_fn_wrapper/3, + fun ?MODULE:gen_hook_fn_wrapper/3, #{module => Module, function => Function}, Priority). diff --git a/src/gen_hook.erl b/src/gen_hook.erl index 61c35ee3c6..af6d73525a 100644 --- a/src/gen_hook.erl +++ b/src/gen_hook.erl @@ -40,15 +40,24 @@ -type key() :: {HookName :: atom(), Tag :: any()}. +-type hook_tuple() :: {HookName :: hook_name(), + Tag :: hook_tag(), + Function :: hook_fn(), + Extra :: hook_extra(), + Priority :: pos_integer()}. + +-type hook_list() :: [hook_tuple()]. + +-export_type([hook_fn/0, hook_list/0]). + -record(hook_handler, {key :: key(), %% 'prio' field must go right after the 'key', %% this is required for the proper sorting. prio :: pos_integer(), - fn :: hook_fn(), + module :: module(), + function :: atom(), %% function name extra :: map()}). --export_type([hook_fn/0]). - -define(TABLE, ?MODULE). %%%---------------------------------------------------------------------- %%% API @@ -63,16 +72,17 @@ start_link() -> Function :: hook_fn(), Extra :: hook_extra(), Priority :: pos_integer()) -> ok. -add_handler(HookName, Tag, Function, Extra, Priority) when is_atom(HookName), - is_function(Function, 3), - is_map(Extra), - is_integer(Priority), - Priority > 0 -> - NewExtra = extend_extra(HookName, Tag, Extra), - Handler = #hook_handler{key = hook_key(HookName, Tag), - prio = Priority, - fn = Function, - extra = NewExtra}, +add_handler(HookName, Tag, Function, Extra, Priority) -> + add_handler({HookName, Tag, Function, Extra, Priority}). + +-spec add_handlers(hook_list()) -> ok. +add_handlers(List) -> + [add_handler(HookTuple) || HookTuple <- List], + ok. + +-spec add_handler(hook_tuple()) -> ok. +add_handler(HookTuple) -> + Handler = make_hook_handler(HookTuple), gen_server:call(?MODULE, {add_handler, Handler}). %% @doc Delete a hook handler. @@ -82,46 +92,19 @@ add_handler(HookName, Tag, Function, Extra, Priority) when is_atom(HookName), Function :: hook_fn(), Extra :: hook_extra(), Priority :: pos_integer()) -> ok. -delete_handler(HookName, Tag, Function, Extra, Priority) when is_atom(HookName), - is_function(Function, 3), - is_map(Extra), - is_integer(Priority), - Priority > 0 -> - NewExtra = extend_extra(HookName, Tag, Extra), - Handler = #hook_handler{key = hook_key(HookName, Tag), - prio = Priority, - fn = Function, - extra = NewExtra}, - gen_server:call(?MODULE, {delete_handler, Handler}). +delete_handler(HookName, Tag, Function, Extra, Priority) -> + delete_handler({HookName, Tag, Function, Extra, Priority}). --spec add_handlers([{HookName :: hook_name(), - Tag :: hook_tag(), - Function :: hook_fn(), - Extra :: hook_extra(), - Priority :: pos_integer()}]) -> ok. -add_handlers(List) -> - [add_handler(HookName, Tag, Function, Extra, Priority) || - {HookName, Tag, Function, Extra, Priority} <- List, is_atom(HookName), - is_function(Function, 3), - is_map(Extra), - is_integer(Priority), - Priority > 0], - ok. - --spec delete_handlers([{HookName :: hook_name(), - Tag :: hook_tag(), - Function :: hook_fn(), - Extra :: hook_extra(), - Priority :: pos_integer()}]) -> ok. +-spec delete_handlers(hook_list()) -> ok. delete_handlers(List) -> - [delete_handler(HookName, Tag, Function, Extra, Priority) || - {HookName, Tag, Function, Extra, Priority} <- List, is_atom(HookName), - is_function(Function, 3), - is_map(Extra), - is_integer(Priority), - Priority > 0], + [delete_handler(HookTuple) || HookTuple <- List], ok. +-spec delete_handler(hook_tuple()) -> ok. +delete_handler(HookTuple) -> + Handler = make_hook_handler(HookTuple), + gen_server:call(?MODULE, {delete_handler, Handler}). + %% @doc Run hook handlers in order of priority (lower number means higher priority). %% * if hook handler returns {ok, NewAcc}, the NewAcc value is used %% as an accumulator parameter for the following hook handler. @@ -208,8 +191,8 @@ code_change(_OldVsn, State, _Extra) -> -spec run_hook([#hook_handler{}], hook_acc(), hook_params()) -> hook_fn_ret_value(). run_hook([], Acc, _Params) -> {ok, Acc}; -run_hook([#hook_handler{fn = Function, extra = Extra} = Handler | Ls], Acc, Params) -> - case apply_hook_function(Function, Acc, Params, Extra) of +run_hook([Handler | Ls], Acc, Params) -> + case apply_hook_function(Handler, Acc, Params) of {'EXIT', Reason} -> ?MODULE:error_running_hook(Reason, Handler, Acc, Params), run_hook(Ls, Acc, Params); @@ -219,10 +202,11 @@ run_hook([#hook_handler{fn = Function, extra = Extra} = Handler | Ls], Acc, Para run_hook(Ls, NewAcc, Params) end. --spec apply_hook_function(hook_fn(), hook_acc(), hook_params(), hook_extra()) -> +-spec apply_hook_function(#hook_handler{}, hook_acc(), hook_params()) -> hook_fn_ret_value() | {'EXIT', Reason :: any()}. -apply_hook_function(Function, Acc, Params, Extra) -> - safely:apply(Function, [Acc, Params, Extra]). +apply_hook_function(#hook_handler{module = Module, function = Function, extra = Extra}, + Acc, Params) -> + safely:apply(Module, Function, [Acc, Params, Extra]). error_running_hook(Reason, Handler, Acc, Params) -> ?LOG_ERROR(#{what => hook_failed, @@ -232,15 +216,58 @@ error_running_hook(Reason, Handler, Acc, Params) -> params => Params, reason => Reason}). --spec hook_key(hook_name(), hook_tag()) -> key(). -hook_key(HookName, Tag) -> {HookName, Tag}. +-spec make_hook_handler(hook_tuple()) -> #hook_handler{}. +make_hook_handler({HookName, _Tag, Function, Extra, Priority} = HookTuple) + when is_atom(HookName), is_function(Function, 3), is_map(Extra), + is_integer(Priority), Priority > 0 -> + NewExtra = extend_extra(HookTuple), + {Module, FunctionName} = check_hook_function(Function), + #hook_handler{key = hook_key(HookTuple), + prio = Priority, + module = Module, + function = FunctionName, + extra = NewExtra}. + +-spec check_hook_function(hook_fn()) -> {module(), atom()}. +check_hook_function(Function) when is_function(Function, 3) -> + case erlang:fun_info(Function, type) of + {type, external} -> + {module, Module} = erlang:fun_info(Function, module), + {name, FunctionName} = erlang:fun_info(Function, name), + case code:ensure_loaded(Module) of + {module, Module} -> ok; + Error -> + throw_error(#{what => module_is_not_loaded, + module => Module, error => Error}) + end, + case erlang:function_exported(Module, FunctionName, 3) of + true -> ok; + false -> + throw_error(#{what => function_is_not_exported, + function => Function}) + end, + {Module, FunctionName}; + {type, local} -> + throw_error(#{what => only_external_function_references_allowed, + function => Function}) + end. + +-spec throw_error(map()) -> no_return(). +throw_error(ErrorMap) -> + ?LOG_ERROR(ErrorMap), + error(ErrorMap). + +-spec hook_key(hook_tuple()) -> key(). +hook_key({HookName, Tag, _Function, _Extra, _Priority}) -> + {HookName, Tag}. -extend_extra(HookName, Tag, OriginalExtra) -> - ExtendedExtra = #{hook_name => HookName, hook_tag => Tag}, +-spec extend_extra(hook_tuple()) -> hook_extra(). +extend_extra({HookName, Tag, _Function, OriginalExtra, _Priority}) -> + ExtraExtension = #{hook_name => HookName, hook_tag => Tag}, %% KV pairs of the OriginalExtra map will remain unchanged, - %% only the new keys from the ExtendedExtra map will be added + %% only the new keys from the ExtraExtension map will be added %% to the NewExtra map - maps:merge(ExtendedExtra, OriginalExtra). + maps:merge(ExtraExtension, OriginalExtra). -spec create_hook_metric(Key :: key()) -> any(). create_hook_metric({HookName, Tag}) -> diff --git a/test/ejabberd_hooks_SUITE.erl b/test/ejabberd_hooks_SUITE.erl index aa235de514..d7907083c3 100644 --- a/test/ejabberd_hooks_SUITE.erl +++ b/test/ejabberd_hooks_SUITE.erl @@ -45,8 +45,9 @@ a_module_fun_can_be_added(_) -> % then [{{test_run_hook,<<"localhost">>}, - [{hook_handler, {test_run_hook,<<"localhost">>}, - 1,_FN,#{function := fun_a, module := hook_mod}}]}] = get_hooks(). + [{hook_handler, {test_run_hook,<<"localhost">>}, 1, + ejabberd_hooks, gen_hook_fn_wrapper, + #{function := fun_a, module := hook_mod}}]}] = get_hooks(). a_module_fun_can_be_removed(_) -> given_hooks_started(), diff --git a/test/muc_light_SUITE.erl b/test/muc_light_SUITE.erl index e8dc9c88f4..23cafbbcfa 100644 --- a/test/muc_light_SUITE.erl +++ b/test/muc_light_SUITE.erl @@ -119,7 +119,7 @@ codec_calls(_Config) -> HandleFun = fun(_, _, _) -> count_call(handler) end, gen_hook:add_handler(filter_room_packet, <<"localhost">>, - fun(Acc, _Params, _Extra) -> count_call(hook), {ok, Acc} end, + fun ?MODULE:filter_room_packet_handler/3, #{}, 50), @@ -154,6 +154,9 @@ codec_calls(_Config) -> check_count(1, 2), ok. +filter_room_packet_handler(Acc, _Params, _Extra) -> + count_call(hook), + {ok, Acc}. %% ----------------- Room config schema ---------------------- simple_config_items_are_parsed(_Config) ->