From 30fb9dd7ae8c2684170155295eaf6b00335a353d Mon Sep 17 00:00:00 2001 From: zhongwencool Date: Mon, 22 Nov 2021 20:40:56 +0800 Subject: [PATCH] fix: name must be printable unicode and len < 256 --- .../src/emqx_trace/emqx_trace.erl | 28 +++--- src/emqx_trace_handler.erl | 90 +++++++++++-------- test/emqx_trace_handler_SUITE.erl | 32 +++---- 3 files changed, 86 insertions(+), 64 deletions(-) diff --git a/apps/emqx_plugin_libs/src/emqx_trace/emqx_trace.erl b/apps/emqx_plugin_libs/src/emqx_trace/emqx_trace.erl index eb41703fa..2190e5bab 100644 --- a/apps/emqx_plugin_libs/src/emqx_trace/emqx_trace.erl +++ b/apps/emqx_plugin_libs/src/emqx_trace/emqx_trace.erl @@ -128,7 +128,7 @@ create(Trace) -> {error, Reason} -> {error, Reason} end; false -> - {error, "The number of traces created has reached the maximum" + {error, "The number of traces created has reache the maximum" " please delete the useless ones first"} end. @@ -379,11 +379,16 @@ to_trace(TraceParam) -> {error, "type=[topic,clientid,ip_address] required"}; {ok, #?TRACE{filter = undefined}} -> {error, "topic/clientid/ip_address filter required"}; - {ok, TraceRec0} -> - case fill_default(TraceRec0) of - #?TRACE{start_at = Start, end_at = End} when End =< Start -> - {error, "failed by start_at >= end_at"}; - TraceRec -> {ok, TraceRec} + {ok, TraceRec0 = #?TRACE{name = Name, type = Type}} -> + case emqx_trace_handler:handler_id(Name, Type) of + {ok, _} -> + case fill_default(TraceRec0) of + #?TRACE{start_at = Start, end_at = End} when End =< Start -> + {error, "failed by start_at >= end_at"}; + TraceRec -> {ok, TraceRec} + end; + {error, Reason} -> + {error, Reason} end end. @@ -403,10 +408,13 @@ fill_default(Trace) -> Trace. to_trace([], Rec) -> {ok, Rec}; to_trace([{name, Name} | Trace], Rec) -> - case binary:match(Name, [<<"/">>], []) of - nomatch when byte_size(Name) < 200 -> to_trace(Trace, Rec#?TRACE{name = Name}); - nomatch -> {error, "name(latin1) length must < 200"}; - _ -> {error, "name cannot contain /"} + case io_lib:printable_unicode_list(binary_to_list(Name)) of + true -> + case binary:match(Name, [<<"/">>], []) of + nomatch -> to_trace(Trace, Rec#?TRACE{name = Name}); + _ -> {error, "name cannot contain /"} + end; + false -> {error, "name must printable unicode"} end; to_trace([{type, Type} | Trace], Rec) -> case lists:member(Type, [<<"clientid">>, <<"topic">>, <<"ip_address">>]) of diff --git a/src/emqx_trace_handler.erl b/src/emqx_trace_handler.erl index e4008405a..7006fc815 100644 --- a/src/emqx_trace_handler.erl +++ b/src/emqx_trace_handler.erl @@ -35,6 +35,8 @@ , filter_ip_address/2 ]). +-export([handler_id/2]). + -type tracer() :: #{ name := binary(), type := clientid | topic | ip_address, @@ -111,8 +113,10 @@ install(Who, Level, LogFile) -> -spec uninstall(Type :: clientid | topic | ip_address, Name :: binary() | list()) -> ok | {error, term()}. uninstall(Type, Name) -> - HandlerId = handler_id(#{type => Type, name => ensure_bin(Name)}), - uninstall(HandlerId). + case handler_id(ensure_bin(Name), Type) of + {ok, HandlerId} -> uninstall(HandlerId); + {error, Reason} -> {error, Reason} + end. -spec uninstall(HandlerId :: atom()) -> ok | {error, term()}. uninstall(HandlerId) -> @@ -135,64 +139,74 @@ uninstall(HandlerId) -> running() -> lists:foldl(fun filter_traces/2, [], emqx_logger:get_log_handlers(started)). --spec filter_clientid(logger:log_event(), string()) -> logger:log_event() | ignore. -filter_clientid(#{meta := #{clientid := ClientId}} = Log, ClientId) -> Log; +-spec filter_clientid(logger:log_event(), {string(), atom()}) -> logger:log_event() | ignore. +filter_clientid(#{meta := #{clientid := ClientId}} = Log, {ClientId, _Name}) -> Log; filter_clientid(_Log, _ExpectId) -> ignore. --spec filter_topic(logger:log_event(), string()) -> logger:log_event() | ignore. -filter_topic(#{meta := #{topic := Topic}} = Log, TopicFilter) -> +-spec filter_topic(logger:log_event(), {string(), atom()}) -> logger:log_event() | ignore. +filter_topic(#{meta := #{topic := Topic}} = Log, {TopicFilter, _Name}) -> case emqx_topic:match(Topic, TopicFilter) of true -> Log; false -> ignore end; filter_topic(_Log, _ExpectId) -> ignore. --spec filter_ip_address(logger:log_event(), string()) -> logger:log_event() | ignore. -filter_ip_address(#{meta := #{peername := Peername}} = Log, IP) -> +-spec filter_ip_address(logger:log_event(), {string(), atom()}) -> logger:log_event() | ignore. +filter_ip_address(#{meta := #{peername := Peername}} = Log, {IP, _Name}) -> case lists:prefix(IP, Peername) of true -> Log; false -> ignore end; filter_ip_address(_Log, _ExpectId) -> ignore. -install_handler(Who, Level, LogFile) -> - HandlerId = handler_id(Who), - Config = #{ - level => Level, - formatter => ?FORMAT, - filter_default => stop, - filters => filters(Who), - config => ?CONFIG(LogFile) - }, - Res = logger:add_handler(HandlerId, logger_disk_log_h, Config), - show_prompts(Res, Who, "Start trace"), - Res. +install_handler(Who = #{name := Name, type := Type}, Level, LogFile) -> + case handler_id(Name, Type) of + {ok, HandlerId} -> + Config = #{ + level => Level, + formatter => ?FORMAT, + filter_default => stop, + filters => filters(Who), + config => ?CONFIG(LogFile) + }, + Res = logger:add_handler(HandlerId, logger_disk_log_h, Config), + show_prompts(Res, Who, "Start trace"), + Res; + {error, _Reason} = Error -> + show_prompts(Error, Who, "Start trace"), + Error + end. -filters(#{type := clientid, filter := Filter}) -> - [{clientid, {fun ?MODULE:filter_clientid/2, ensure_list(Filter)}}]; -filters(#{type := topic, filter := Filter}) -> - [{topic, {fun ?MODULE:filter_topic/2, ensure_bin(Filter)}}]; -filters(#{type := ip_address, filter := Filter}) -> - [{ip_address, {fun ?MODULE:filter_ip_address/2, ensure_list(Filter)}}]. +filters(#{type := clientid, filter := Filter, name := Name}) -> + [{clientid, {fun ?MODULE:filter_clientid/2, {ensure_list(Filter), Name}}}]; +filters(#{type := topic, filter := Filter, name := Name}) -> + [{topic, {fun ?MODULE:filter_topic/2, {ensure_bin(Filter), Name}}}]; +filters(#{type := ip_address, filter := Filter, name := Name}) -> + [{ip_address, {fun ?MODULE:filter_ip_address/2, {ensure_list(Filter), Name}}}]. filter_traces(#{id := Id, level := Level, dst := Dst, filters := Filters}, Acc) -> Init = #{id => Id, level => Level, dst => Dst}, case Filters of - [{topic, {_FilterFun, Filter}}] -> - <<"trace_topic_", Name/binary>> = atom_to_binary(Id), - [Init#{type => topic, filter => Filter, name => Name} | Acc]; - [{clientid, {_FilterFun, Filter}}] -> - <<"trace_clientid_", Name/binary>> = atom_to_binary(Id), - [Init#{type => clientid, filter => Filter, name => Name} | Acc]; - [{ip_address, {_FilterFun, Filter}}] -> - <<"trace_ip_address_", Name/binary>> = atom_to_binary(Id), - [Init#{type => ip_address, filter => Filter, name => Name} | Acc]; + [{Type, {_FilterFun, {Filter, Name}}}] when + Type =:= topic orelse + Type =:= clientid orelse + Type =:= ip_address -> + [Init#{type => Type, filter => Filter, name => Name} | Acc]; _ -> Acc end. -handler_id(#{type := Type, name := Name}) -> - binary_to_atom(<<"trace_", (atom_to_binary(Type))/binary, "_", Name/binary>>). +handler_id(Name, Type) -> + TypeBin = atom_to_binary(Type), + case io_lib:printable_unicode_list(binary_to_list(Name)) of + true when byte_size(Name) < 256 -> + NameHash = base64:encode(crypto:hash(sha, Name)), + {ok, binary_to_atom(<<"trace_", TypeBin/binary, "_", NameHash/binary>>)}; + true -> + {error, <<"Name length must < 256">>}; + false -> + {error, <<"Name must printable unicode">>} + end. ensure_bin(List) when is_list(List) -> iolist_to_binary(List); ensure_bin(Bin) when is_binary(Bin) -> Bin. @@ -203,4 +217,4 @@ ensure_list(List) when is_list(List) -> List. show_prompts(ok, Who, Msg) -> ?LOG(info, Msg ++ " ~p " ++ "successfully~n", [Who]); show_prompts({error, Reason}, Who, Msg) -> - ?LOG(error, Msg ++ " ~p " ++ "failed by ~p~n", [Who, Reason]). + ?LOG(error, Msg ++ " ~p " ++ "failed with ~p~n", [Who, Reason]). diff --git a/test/emqx_trace_handler_SUITE.erl b/test/emqx_trace_handler_SUITE.erl index 5e909c45b..e314c7994 100644 --- a/test/emqx_trace_handler_SUITE.erl +++ b/test/emqx_trace_handler_SUITE.erl @@ -70,12 +70,12 @@ t_trace_clientid(_Config) -> ?assert(filelib:is_regular("tmp/client3.log")), %% Get current traces - ?assertEqual([#{type => clientid, filter => "client", name => <<"client">>, - id => trace_clientid_client, level => debug, dst => "tmp/client.log"}, - #{type => clientid, filter => "client2", name => <<"client2">>, - id => trace_clientid_client2, level => debug, dst => "tmp/client2.log"}, - #{type => clientid, filter => "client3", name => <<"client3">>, - id => trace_clientid_client3, level => debug, dst => "tmp/client3.log"} + ?assertMatch([#{type := clientid, filter := "client", name := <<"client">>, + level := debug, dst := "tmp/client.log"}, + #{type := clientid, filter := "client2", name := <<"client2">> + , level := debug, dst := "tmp/client2.log"}, + #{type := clientid, filter := "client3", name := <<"client3">>, + level := debug, dst := "tmp/client3.log"} ], emqx_trace_handler:running()), %% Client with clientid = "client" publishes a "hi" message to "a/b/c". @@ -116,10 +116,10 @@ t_trace_topic(_Config) -> ?assert(filelib:is_regular("tmp/topic_trace_y.log")), %% Get current traces - ?assertEqual([#{type => topic, filter => <<"x/#">>, id => 'trace_topic_x/#', - level => debug, dst => "tmp/topic_trace_x.log", name => <<"x/#">>}, - #{type => topic, filter => <<"y/#">>, id => 'trace_topic_y/#', - name => <<"y/#">>, level => debug, dst => "tmp/topic_trace_y.log"} + ?assertMatch([#{type := topic, filter := <<"x/#">>, + level := debug, dst := "tmp/topic_trace_x.log", name := <<"x/#">>}, + #{type := topic, filter := <<"y/#">>, + name := <<"y/#">>, level := debug, dst := "tmp/topic_trace_y.log"} ], emqx_trace_handler:running()), @@ -159,12 +159,12 @@ t_trace_ip_address(_Config) -> ?assert(filelib:is_regular("tmp/ip_trace_y.log")), %% Get current traces - ?assertEqual([#{type => ip_address, filter => "127.0.0.1", - id => 'trace_ip_address_127.0.0.1', name => <<"127.0.0.1">>, - level => debug, dst => "tmp/ip_trace_x.log"}, - #{type => ip_address, filter => "192.168.1.1", - id => 'trace_ip_address_192.168.1.1', name => <<"192.168.1.1">>, - level => debug, dst => "tmp/ip_trace_y.log"} + ?assertMatch([#{type := ip_address, filter := "127.0.0.1", + name := <<"127.0.0.1">>, + level := debug, dst := "tmp/ip_trace_x.log"}, + #{type := ip_address, filter := "192.168.1.1", + name := <<"192.168.1.1">>, + level := debug, dst := "tmp/ip_trace_y.log"} ], emqx_trace_handler:running()),