From 0058430352420a8c0dc053900f108e7086f52fad Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 22 May 2014 11:01:45 +0200 Subject: Replace traffic-related log reports with no-op function calls The former were a little over-enthusiastic and could cause a node to be logged to death if a peer Diameter node was sufficiently ill-willed. The function calls are to diameter_lib:log/4, the arguments of which identify the happening in question, and which does nothing but provide a function to trace on. Many existing log calls have been shrunk. The only remaining traffic-related report (hopefully) is that resulting from {answer_errors, report} config, and this has been slimmed. --- lib/diameter/include/diameter_gen.hrl | 6 ++- lib/diameter/src/base/diameter_codec.erl | 31 +++++++------- lib/diameter/src/base/diameter_lib.erl | 56 +++++++++++++++++++++++-- lib/diameter/src/base/diameter_peer_fsm.erl | 65 ++++++++++------------------- lib/diameter/src/base/diameter_service.erl | 48 ++++++++++++++------- lib/diameter/src/base/diameter_traffic.erl | 51 +++++++++++----------- lib/diameter/src/base/diameter_watchdog.erl | 5 ++- 7 files changed, 155 insertions(+), 107 deletions(-) diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index c8f706dc3e..319ad5a783 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -320,10 +320,12 @@ d(Name, Avp, {Avps, Acc}) -> %% respond sensibly to. Log the occurence for traceability, %% but the peer will also receive info in the resulting %% answer-message. - diameter_lib:log({decode, failure}, + Stack = diameter_lib:get_stacktrace(), + diameter_lib:log(decode_error, ?MODULE, ?LINE, - {Reason, Avp, erlang:get_stacktrace()}), + {Reason, AvpName, Stack}), + {Rec, Failed} = Acc, {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}} after diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index 9db3552286..84139d7f5e 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -70,18 +70,15 @@ encode(Mod, #diameter_packet{} = Pkt) -> try e(Mod, Pkt) catch - exit: {_, _, #diameter_header{}} = T -> + exit: {Reason, Stack, #diameter_header{} = H} = T -> %% Exit with a header in the reason to let the caller %% count encode errors. - X = {?MODULE, encode, T}, - diameter_lib:error_report(X, {?MODULE, encode, [Mod, Pkt]}), - exit(X); + ?LOG(encode_error, {Reason, Stack, H}), + exit({?MODULE, encode, T}); error: Reason -> - %% Be verbose since a crash report may be truncated and - %% encode errors are self-inflicted. - X = {?MODULE, encode, {Reason, ?STACK}}, - diameter_lib:error_report(X, {?MODULE, encode, [Mod, Pkt]}), - exit(X) + T = {Reason, diameter_lib:get_stacktrace()}, + ?LOG(encode_error, T), + exit({?MODULE, encode, T}) end; encode(Mod, Msg) -> @@ -115,7 +112,7 @@ e(_, #diameter_packet{msg = [#diameter_header{} = Hdr | As]} = Pkt) -> Avps/binary>>} catch error: Reason -> - exit({Reason, ?STACK, Hdr}) + exit({Reason, diameter_lib:get_stacktrace(), Hdr}) end; e(Mod, #diameter_packet{header = Hdr0, msg = Msg} = Pkt) -> @@ -147,7 +144,7 @@ e(Mod, #diameter_packet{header = Hdr0, msg = Msg} = Pkt) -> Avps/binary>>} catch error: Reason -> - exit({Reason, ?STACK, Hdr}) + exit({Reason, diameter_lib:get_stacktrace(), Hdr}) end. %% make_flags/2 @@ -278,23 +275,23 @@ decode(Id, Mod, Bin) decode(Id, Mod, #diameter_packet{header = decode_header(Bin), bin = Bin}). decode_avps(MsgName, Mod, Pkt, {E, Avps}) -> - ?LOG(invalid, Pkt#diameter_packet.bin), + ?LOG(invalid_avp_length, Pkt#diameter_packet.header), #diameter_packet{errors = Failed} = P = decode_avps(MsgName, Mod, Pkt, Avps), P#diameter_packet{errors = [E | Failed]}; -decode_avps('', Mod, Pkt, Avps) -> %% unknown message ... - ?LOG(unknown, {Mod, Pkt#diameter_packet.header}), +decode_avps('', _, Pkt, Avps) -> %% unknown message ... + ?LOG(unknown_message, Pkt#diameter_packet.header), Pkt#diameter_packet{avps = lists:reverse(Avps), errors = [3001]}; %% DIAMETER_COMMAND_UNSUPPORTED %% msg = undefined identifies this case. decode_avps(MsgName, Mod, Pkt, Avps) -> %% ... or not - {Rec, As, Failed} = Mod:decode_avps(MsgName, Avps), - ?LOGC([] /= Failed, failed, {Mod, Failed}), + {Rec, As, Errors} = Mod:decode_avps(MsgName, Avps), + ?LOGC([] /= Errors, decode_errors, Pkt#diameter_packet.header), Pkt#diameter_packet{msg = Rec, - errors = Failed, + errors = Errors, avps = As}. %%% --------------------------------------------------------------------------- diff --git a/lib/diameter/src/base/diameter_lib.erl b/lib/diameter/src/base/diameter_lib.erl index 44d81e2778..5b3a2063f8 100644 --- a/lib/diameter/src/base/diameter_lib.erl +++ b/lib/diameter/src/base/diameter_lib.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2013. All Rights Reserved. +%% Copyright Ericsson AB 2010-2014. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -25,12 +25,30 @@ now_diff/1, time/1, eval/1, + eval_name/1, + get_stacktrace/0, ipaddr/1, spawn_opts/2, wait/1, fold_tuple/3, log/4]). +%% --------------------------------------------------------------------------- +%% # get_stacktrace/0 +%% --------------------------------------------------------------------------- + +%% Return a stacktrace with a leading, potentially large, argument +%% list replaced by an arity. Trace on stacktrace/0 to see the +%% original. + +get_stacktrace() -> + stacktrace(erlang:get_stacktrace()). + +stacktrace([{M,F,A,L} | T]) when is_list(A) -> + [{M, F, length(A), L} | T]; +stacktrace(L) -> + L. + %% --------------------------------------------------------------------------- %% # info_report/2 %% --------------------------------------------------------------------------- @@ -60,9 +78,17 @@ warning_report(Reason, T) -> report(fun error_logger:warning_report/1, Reason, T). report(Fun, Reason, T) -> - Fun([{why, Reason}, {who, self()}, {what, T}]), + Fun(io_lib:format("diameter: ~" ++ fmt(Reason) ++ "~n ~p~n", + [Reason, T])), false. +fmt(T) -> + if is_list(T) -> + "s"; + true -> + "p" + end. + %% --------------------------------------------------------------------------- %% # now_diff/1 %% --------------------------------------------------------------------------- @@ -129,8 +155,8 @@ eval({M,F,A}) -> eval([{M,F,A} | X]) -> apply(M, F, X ++ A); -eval([[F|A] | X]) -> - eval([F | X ++ A]); +eval([[F|X] | A]) -> + eval([F | A ++ X]); eval([F|A]) -> apply(F,A); @@ -141,6 +167,28 @@ eval({F}) -> eval(F) -> F(). +%% --------------------------------------------------------------------------- +%% eval_name/1 +%% --------------------------------------------------------------------------- + +eval_name({M,F,A}) -> + {M, F, length(A)}; + +eval_name([{M,F,A} | X]) -> + {M, F, length(A) + length(X)}; + +eval_name([[F|A] | X]) -> + eval_name([F | X ++ A]); + +eval_name([F|_]) -> + F; + +eval_name({F}) -> + eval_name(F); + +eval_name(F) -> + F. + %% --------------------------------------------------------------------------- %% # ipaddr/1 %% diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl index 4b97fa96b3..e9b6a263c9 100644 --- a/lib/diameter/src/base/diameter_peer_fsm.erl +++ b/lib/diameter/src/base/diameter_peer_fsm.erl @@ -283,7 +283,7 @@ handle_info(T, #state{} = State) -> ok -> {noreply, State}; #state{state = X} = S -> - ?LOGC(X =/= State#state.state, transition, X), + ?LOGC(X /= State#state.state, transition, X), {noreply, S}; {stop, Reason} -> ?LOG(stop, Reason), @@ -295,13 +295,9 @@ handle_info(T, #state{} = State) -> exit: {diameter_codec, encode, T} = Reason -> incr_error(send, T), ?LOG(stop, Reason), - %% diameter_codec:encode/2 emits an error report. Only - %% indicate the probable reason here. - diameter_lib:info_report(probable_configuration_error, - insufficient_capabilities), {stop, {shutdown, Reason}, State}; {?MODULE, Tag, Reason} -> - ?LOG(Tag, {Reason, T}), + ?LOG(stop, Tag), {stop, {shutdown, Reason}, State} end. %% The form of the throw caught here is historical. It's @@ -477,12 +473,12 @@ send_CER(#state{state = {'Wait-Conn-Ack', Tmo}, orelse close({already_connected, Remote, LCaps}), CER = build_CER(S), - ?LOG(send, 'CER'), #diameter_packet{header = #diameter_header{end_to_end_id = Eid, hop_by_hop_id = Hid}} = Pkt = encode(CER, Dict), send(TPid, Pkt), + ?LOG(send, 'CER'), start_timer(Tmo, S#state{state = {'Wait-CEA', Hid, Eid}}). %% Register ourselves as connecting to the remote endpoint in @@ -554,26 +550,18 @@ recv(#diameter_header{length = Len} recv(#diameter_header{} = H, #diameter_packet{bin = Bin}, - #state{length_errors = E} - = S) -> - invalid(E, - invalid_message_length, - recv, - [size(Bin), bit_size(Bin) rem 8, H, S]); + #state{length_errors = E}) -> + T = {size(Bin), bit_size(Bin) rem 8, H}, + invalid(E, message_length_mismatch, T); -recv(false, Pkt, #state{length_errors = E} = S) -> - invalid(E, truncated_header, recv, [Pkt, S]). +recv(false, #diameter_packet{bin = Bin}, #state{length_errors = E}) -> + invalid(E, truncated_header, Bin). %% Note that counters here only count discarded messages. -invalid(E, Reason, F, A) -> +invalid(E, Reason, T) -> diameter_stats:incr(Reason), - abort(E, Reason, F, A). - -abort(exit, Reason, F, A) -> - diameter_lib:warning_report(Reason, {?MODULE, F, A}), - throw({?MODULE, abort, Reason}); - -abort(_, _, _, _) -> + E == exit andalso close({Reason, T}), + ?LOG(Reason, T), ok. msg_id({_,_,_} = T, _) -> @@ -674,8 +662,12 @@ send_answer(Type, ReqPkt, #state{transport = TPid, dictionary = Dict} = S) -> incr_rc(send, AnsPkt, Dict), send(TPid, AnsPkt), + ?LOG(send, ans(Type)), eval(PostF, S). +ans('CER') -> 'CEA'; +ans('DPR') -> 'DPA'. + eval([F|A], S) -> apply(F, A ++ [S]); eval(ok, S) -> @@ -897,9 +889,7 @@ handle_CEA(#diameter_packet{bin = Bin} %% connection with the peer. try - is_integer(RC) - orelse ?THROW(no_result_code), - ?IS_SUCCESS(RC) + is_integer(RC) andalso ?IS_SUCCESS(RC) orelse ?THROW(RC), [] == SApps andalso ?THROW(no_common_application), @@ -1015,19 +1005,13 @@ capz(#diameter_caps{} = L, #diameter_caps{} = R) -> tl(tuple_to_list(R)))]). %% close/1 +%% +%% A good function to trace on in case of problems with capabilities +%% exchange. close(Reason) -> - report(Reason), throw({?MODULE, close, Reason}). -%% Could possibly log more here. -report({M, _, _, _, _} = T) - when M == 'CER'; - M == 'CEA' -> - diameter_lib:error_report(failure, T); -report(_) -> - ok. - %% dpr/2 %% %% The RFC isn't clear on whether DPR should be send in a non-Open @@ -1061,7 +1045,7 @@ dpr(_Reason, _S) -> %% process and contact it. (eg. diameter:service_info/2) dpr([CB|Rest], [Reason | _] = Args, S) -> - try diameter_lib:eval([CB | Args]) of + case diameter_lib:eval([CB | Args]) of {dpr, Opts} when is_list(Opts) -> send_dpr(Reason, Opts, S); dpr -> @@ -1071,14 +1055,7 @@ dpr([CB|Rest], [Reason | _] = Args, S) -> ignore -> dpr(Rest, Args, S); T -> - No = {disconnect_cb, T}, - diameter_lib:error_report(invalid, No), - {stop, No} - catch - E:R -> - No = {disconnect_cb, E, R, ?STACK}, - diameter_lib:error_report(failure, No), - {stop, No} + ?ERROR({disconnect_cb, CB, Args, T}) end; dpr([], [Reason | _], S) -> diff --git a/lib/diameter/src/base/diameter_service.erl b/lib/diameter/src/base/diameter_service.erl index 0dc3eb7123..b7cd311e02 100644 --- a/lib/diameter/src/base/diameter_service.erl +++ b/lib/diameter/src/base/diameter_service.erl @@ -731,14 +731,27 @@ remotes(F) -> L when is_list(L) -> L; T -> - diameter_lib:error_report({invalid_return, T}, F), + ?LOG(invalid_return, {F,T}), + error_report(invalid_return, share_peers, F), [] catch E:R -> - diameter_lib:error_report({failure, {E, R, ?STACK}}, F), + ?LOG(failure, {E, R, F, diameter_lib:get_stacktrace()}), + error_report(failure, share_peers, F), [] end. +%% error_report/3 + +error_report(T, What, F) -> + Reason = io_lib:format("~s from ~p callback", [reason(T), What]), + diameter_lib:error_report(Reason, diameter_lib:eval_name(F)). + +reason(invalid_return) -> + "invalid return"; +reason(failure) -> + "failure". + %% --------------------------------------------------------------------------- %% # start/3 %% --------------------------------------------------------------------------- @@ -1038,8 +1051,11 @@ peer_cb(App, F, A) -> true catch E:R -> - diameter_lib:error_report({failure, {E, R, ?STACK}}, - {App, F, A}), + %% Don't include arguments since a #diameter_caps{} strings + %% from the peer, which could be anything (especially, large). + [Mod|X] = App#diameter_app.module, + ?LOG(failure, {E, R, Mod, F, diameter_lib:get_stacktrace()}), + error_report(failure, F, {Mod, F, A ++ X}), false end. @@ -1262,13 +1278,14 @@ cm([#diameter_app{alias = Alias} = App], Req, From, Svc) -> mod_state(Alias, ModS), {T, RC}; T -> - diameter_lib:error_report({invalid, T}, - {App, handle_call, Args}), + ModX = App#diameter_app.module, + ?LOG(invalid_return, {ModX, handle_call, Args, T}), invalid catch E: Reason -> - diameter_lib:error_report({failure, {E, Reason, ?STACK}}, - {App, handle_call, Args}), + ModX = App#diameter_app.module, + Stack = diameter_lib:get_stacktrace(), + ?LOG(failure, {E, Reason, ModX, handle_call, Stack}), failure end; @@ -1426,13 +1443,16 @@ pick_peer(Local, T; %% Accept returned state in the immutable {false = No, S} -> %% case as long it isn't changed. No; - T -> - diameter_lib:error_report({invalid, T, App}, - {App, pick_peer, Args}) + T when M -> + ModX = App#diameter_app.module, + ?LOG(invalid_return, {ModX, pick_peer, T}), + false catch - E: Reason -> - diameter_lib:error_report({failure, {E, Reason, ?STACK}}, - {App, pick_peer, Args}) + E: Reason when M -> + ModX = App#diameter_app.module, + Stack = diameter_lib:get_stacktrace(), + ?LOG(failure, {E, Reason, ModX, pick_peer, Stack}), + false end. %% peers/4 diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index f2ac745053..7c680f96b5 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -48,6 +48,8 @@ -include_lib("diameter/include/diameter.hrl"). -include("diameter_internal.hrl"). +-define(LOGX(Reason, T), begin ?LOG(Reason, T), x({Reason, T}) end). + -define(RELAY, ?DIAMETER_DICT_RELAY). -define(BASE, ?DIAMETER_DICT_COMMON). %% Note: the RFC 3588 dictionary @@ -154,9 +156,8 @@ incr_rc(Dir, Pkt, TPid, Dict0) -> try incr_rc(Dir, Pkt, Dict0, TPid, Dict0) catch - exit: {invalid_error_bit = E, _} -> - E; - exit: no_result_code = E -> + exit: {E,_} when E == no_result_code; + E == invalid_error_bit -> E end. @@ -234,7 +235,7 @@ spawn_request(TPid, Pkt, Dict0, Opts, RecvData) -> spawn_opt(fun() -> recv_request(TPid, Pkt, Dict0, RecvData) end, Opts) catch error: system_limit = E -> %% discard - ?LOG({error, E}, now()) + ?LOG(error, E) end. %% --------------------------------------------------------------------------- @@ -336,23 +337,25 @@ rc(N) -> %% This error is returned when a request is received with an invalid %% message length. -errors(_, #diameter_packet{header = #diameter_header{length = Len}, +errors(_, #diameter_packet{header = #diameter_header{length = Len} = H, bin = Bin, errors = Es} = Pkt) when Len < 20; 0 /= Len rem 4; 8*Len /= bit_size(Bin) -> + ?LOG(invalid_message_length, {H, bit_size(Bin)}), Pkt#diameter_packet{errors = [5015 | Es]}; %% DIAMETER_UNSUPPORTED_VERSION 5011 %% This error is returned when a request was received, whose version %% number is unsupported. -errors(_, #diameter_packet{header = #diameter_header{version = V}, +errors(_, #diameter_packet{header = #diameter_header{version = V} = H, errors = Es} = Pkt) when V /= ?DIAMETER_VERSION -> + ?LOG(unsupported_version, H), Pkt#diameter_packet{errors = [5011 | Es]}; %% DIAMETER_COMMAND_UNSUPPORTED 3001 @@ -360,12 +363,13 @@ errors(_, #diameter_packet{header = #diameter_header{version = V}, %% recognize or support. This MUST be used when a Diameter node %% receives an experimental command that it does not understand. -errors(Id, #diameter_packet{header = #diameter_header{is_proxiable = P}, +errors(Id, #diameter_packet{header = #diameter_header{is_proxiable = P} = H, msg = M, errors = Es} = Pkt) when ?APP_ID_RELAY /= Id, undefined == M; %% don't know the command ?APP_ID_RELAY == Id, not P -> %% command isn't proxiable + ?LOG(command_unsupported, H), Pkt#diameter_packet{errors = [3001 | Es]}; %% DIAMETER_INVALID_HDR_BITS 3008 @@ -374,9 +378,11 @@ errors(Id, #diameter_packet{header = #diameter_header{is_proxiable = P}, %% inconsistent with the command code's definition. errors(_, #diameter_packet{header = #diameter_header{is_request = true, - is_error = true}, + is_error = true} + = H, errors = Es} = Pkt) -> + ?LOG(invalid_hdr_bits, H), Pkt#diameter_packet{errors = [3008 | Es]}; %% Green. @@ -532,7 +538,6 @@ answer_message(RC, origin_realm = {OR,_}}, Dict0, Pkt) -> - ?LOG({error, RC}, Pkt), {Dict0, answer_message(OH, OR, RC, Dict0, Pkt)}. %% resend/7 @@ -1042,12 +1047,12 @@ incr_rc(Dir, Pkt, Dict, TPid, Dict0) -> %% Exit on a missing result code. T = rc_counter(Dict, Msg), - T == false andalso x(no_result_code, answer, [Dir, Pkt]), + T == false andalso ?LOGX(no_result_code, {Dict, Dir, Hdr}), {Ctr, RC} = T, %% Or on an inappropriate value. is_result(RC, E, Dict0) - orelse x({invalid_error_bit, RC}, answer, [Dir, Pkt]), + orelse ?LOGX(invalid_error_bit, {Dict, Dir, Hdr, RC}), incr(TPid, {Id, Dir, Ctr}). @@ -1112,13 +1117,6 @@ int(N) int(_) -> undefined. --spec x(any(), atom(), list()) -> no_return(). - -%% Warn and exit request process on errors in an incoming answer. -x(Reason, F, A) -> - diameter_lib:warning_report(Reason, {?MODULE, F, A}), - x(Reason). - x(T) -> exit(T). @@ -1430,7 +1428,7 @@ handle_A(Pkt, SvcName, Dict, Dict0, App, #request{transport = TPid} = Req) -> of _ -> answer(Pkt, SvcName, App, Req) catch - exit: no_result_code -> + exit: {no_result_code, _} -> %% RFC 6733 requires one of Result-Code or %% Experimental-Result, but the decode will have detected %% a missing AVP. If both are optional in the dictionary @@ -1462,11 +1460,16 @@ a(#diameter_packet{errors = Es} callback == AE -> cb(ModX, handle_answer, [Pkt, msg(P), SvcName, {TPid, Caps}]); -a(Pkt, SvcName, _, report, Req) -> - x(errors, handle_answer, [SvcName, Req, Pkt]); +a(Pkt, SvcName, _, AE, _) -> + a(Pkt#diameter_packet.header, SvcName, AE). + +a(Hdr, SvcName, report) -> + MFA = {?MODULE, handle_answer, [SvcName, Hdr]}, + diameter_lib:warning_report(errors, MFA), + a(Hdr, SvcName, discard); -a(Pkt, SvcName, _, discard, Req) -> - x({errors, handle_answer, [SvcName, Req, Pkt]}). +a(Hdr, SvcName, discard) -> + x({answer_errors, {SvcName, Hdr}}). %% Note that we don't check that the application id in the answer's %% header is what we expect. (TODO: Does the rfc says anything about @@ -1652,7 +1655,7 @@ resend_request(Pkt0, packet = Pkt0, caps = Caps}, - ?LOG(retransmission, Req), + ?LOG(retransmission, Pkt#diameter_packet.header), TRef = send_request(TPid, Pkt, Req, SvcName, Tmo), {TRef, Req}. diff --git a/lib/diameter/src/base/diameter_watchdog.erl b/lib/diameter/src/base/diameter_watchdog.erl index dbfe087659..5e27778432 100644 --- a/lib/diameter/src/base/diameter_watchdog.erl +++ b/lib/diameter/src/base/diameter_watchdog.erl @@ -219,7 +219,6 @@ dict0(_, _, Acc) -> Acc. config_error(T) -> - diameter_lib:error_report(configuration_error, T), exit({shutdown, {configuration_error, T}}). %% handle_call/3 @@ -268,7 +267,7 @@ event(Msg, TPid = tpid(F,T), E = {[TPid | data(Msg, TPid, From, To)], From, To}, send(Pid, {watchdog, self(), E}), - ?LOG(transition, {self(), E}). + ?LOG(transition, {From, To}). data(Msg, TPid, reopen, okay) -> {recv, TPid, 'DWA', _Pkt} = Msg, %% assert @@ -562,6 +561,7 @@ recv(Name, Pkt, S) -> rcv('DWR', Pkt, #watchdog{transport = TPid, dictionary = Dict0}) -> + ?LOG(recv, 'DWR'), DPkt = diameter_codec:decode(Dict0, Pkt), diameter_traffic:incr_error(recv, DPkt, TPid), EPkt = encode(dwa, Dict0, Pkt), @@ -572,6 +572,7 @@ rcv('DWR', Pkt, #watchdog{transport = TPid, rcv('DWA', Pkt, #watchdog{transport = TPid, dictionary = Dict0}) -> + ?LOG(recv, 'DWA'), diameter_traffic:incr_rc(recv, diameter_codec:decode(Dict0, Pkt), TPid, -- cgit v1.2.3 From df19c2724e985daed4ee56abeedca779d20bd194 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 23 May 2014 11:51:25 +0200 Subject: Don't count messages on arbitrary keys That is, don't use a key constructed from an incoming Diameter header unless the message is known to the dictionary in question. Otherwise there are 2^32 application ids, 2^24 command codes, and 2 R-bits for an ill-willed peer to choose from, each resulting in new keys in the counter table (diameter_stats). The usual {ApplicationId, CommandCode, Rbit} in a key is replaced by the atom 'unknown' if the message in question is unknown to the decoding dictionary. Counters for messages sent and received by a relay are (still) not implemented. --- lib/diameter/src/base/diameter_peer_fsm.erl | 62 +++++++++++++--------- lib/diameter/src/base/diameter_traffic.erl | 79 +++++++++++++++++++---------- lib/diameter/src/base/diameter_watchdog.erl | 14 +++-- 3 files changed, 102 insertions(+), 53 deletions(-) diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl index e9b6a263c9..58166349f0 100644 --- a/lib/diameter/src/base/diameter_peer_fsm.erl +++ b/lib/diameter/src/base/diameter_peer_fsm.erl @@ -293,7 +293,7 @@ handle_info(T, #state{} = State) -> {stop, {shutdown, T}, State} catch exit: {diameter_codec, encode, T} = Reason -> - incr_error(send, T), + incr_error(send, T, State#state.dictionary), ?LOG(stop, Reason), {stop, {shutdown, Reason}, State}; {?MODULE, Tag, Reason} -> @@ -523,7 +523,6 @@ recv(#diameter_packet{header = #diameter_header{} = Hdr} = S) -> Name = diameter_codec:msg_name(Dict0, Hdr), Pid ! {recv, self(), Name, Pkt}, - diameter_stats:incr({msg_id(Name, Hdr), recv}), %% count received rcv(Name, Pkt, S); recv(#diameter_packet{header = undefined, @@ -564,20 +563,16 @@ invalid(E, Reason, T) -> ?LOG(Reason, T), ok. -msg_id({_,_,_} = T, _) -> - T; -msg_id(_, Hdr) -> - {_,_,_} = diameter_codec:msg_id(Hdr). - %% rcv/3 %% Incoming CEA. -rcv('CEA', +rcv('CEA' = N, #diameter_packet{header = #diameter_header{end_to_end_id = Eid, hop_by_hop_id = Hid}} = Pkt, #state{state = {'Wait-CEA', Hid, Eid}} = S) -> + ?LOG(recv, N), handle_CEA(Pkt, S); %% Incoming CER @@ -598,29 +593,46 @@ rcv('DPR' = N, Pkt, S) -> %% DPA in response to DPR and with the expected identifiers. rcv('DPA' = N, #diameter_packet{header = #diameter_header{end_to_end_id = Eid, - hop_by_hop_id = Hid}} + hop_by_hop_id = Hid} + = H} = Pkt, #state{dictionary = Dict0, transport = TPid, dpr = {Hid, Eid}}) -> - + ?LOG(recv, N), + incr(recv, H, Dict0), incr_rc(recv, diameter_codec:decode(Dict0, Pkt), Dict0), diameter_peer:close(TPid), {stop, N}; %% Ignore anything else, an unsolicited DPA in particular. +rcv(N, #diameter_packet{header = H}, _) + when N == 'CER'; + N == 'CEA'; + N == 'DPR'; + N == 'DPA' -> + ?LOG(ignored, N), + %% Note that these aren't counted in the normal recv counter. + diameter_stats:incr({diameter_codec:msg_id(H), recv, ignored}), + ok; + rcv(_, _, _) -> ok. +%% incr/3 + +incr(Dir, Hdr, Dict0) -> + diameter_traffic:incr(Dir, Hdr, self(), Dict0). + %% incr_rc/3 incr_rc(Dir, Pkt, Dict0) -> diameter_traffic:incr_rc(Dir, Pkt, self(), Dict0). -%% incr_error/2 +%% incr_error/3 -incr_error(Dir, Pkt) -> - diameter_traffic:incr_error(Dir, Pkt, self()). +incr_error(Dir, Pkt, Dict0) -> + diameter_traffic:incr_error(Dir, Pkt, self(), Dict0). %% send/2 @@ -628,19 +640,23 @@ incr_error(Dir, Pkt) -> %% sending. In particular, the watchdog will send DWR as a binary %% while messages coming from clients will be in a #diameter_packet. send(Pid, Msg) -> - diameter_stats:incr({diameter_codec:msg_id(Msg), send}), diameter_peer:send(Pid, Msg). %% handle_request/3 +%% +%% Incoming CER or DPR. -handle_request(Type, #diameter_packet{} = Pkt, #state{dictionary = D} = S) -> - ?LOG(recv, Type), - send_answer(Type, diameter_codec:decode(D, Pkt), S). +handle_request(Name, + #diameter_packet{header = H} = Pkt, + #state{dictionary = Dict0} = S) -> + ?LOG(recv, Name), + incr(recv, H, Dict0), + send_answer(Name, diameter_codec:decode(Dict0, Pkt), S). %% send_answer/3 send_answer(Type, ReqPkt, #state{transport = TPid, dictionary = Dict} = S) -> - incr_error(recv, ReqPkt), + incr_error(recv, ReqPkt, Dict), #diameter_packet{header = H, transport_data = TD} @@ -660,6 +676,7 @@ send_answer(Type, ReqPkt, #state{transport = TPid, dictionary = Dict} = S) -> AnsPkt = diameter_codec:encode(Dict, Pkt), + incr(send, AnsPkt, Dict), incr_rc(send, AnsPkt, Dict), send(TPid, AnsPkt), ?LOG(send, ans(Type)), @@ -862,13 +879,12 @@ recv_CER(CER, #state{service = Svc, dictionary = Dict}) -> %% handle_CEA/2 -handle_CEA(#diameter_packet{bin = Bin} +handle_CEA(#diameter_packet{header = H} = Pkt, #state{dictionary = Dict0, service = #diameter_service{capabilities = LCaps}} - = S) - when is_binary(Bin) -> - ?LOG(recv, 'CEA'), + = S) -> + incr(recv, H, Dict0), #diameter_packet{} = DPkt @@ -909,7 +925,7 @@ handle_CEA(#diameter_packet{bin = Bin} %% capabilities exchange could send DIAMETER_LIMITED_SUCCESS = 2002, %% even if this isn't required by RFC 3588. -result_code({{'Result-Code', N}, _}) -> +result_code({'Result-Code', N}) -> N; result_code(_) -> undefined. diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 7c680f96b5..5fac61f416 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -32,7 +32,8 @@ -export([receive_message/4]). %% towards diameter_peer_fsm and diameter_watchdog --export([incr_error/3, +-export([incr/4, + incr_error/4, incr_rc/4]). %% towards diameter_service @@ -115,38 +116,52 @@ peer_down(TPid) -> failover(TPid). %% --------------------------------------------------------------------------- -%% incr_error/3 +%% incr/4 %% --------------------------------------------------------------------------- -%% A decoded message with errors. -incr_error(Dir, #diameter_packet{header = H, errors = [_|_]}, TPid) -> - incr_error(Dir, H, TPid); +incr(Dir, #diameter_packet{header = H}, TPid, Dict) -> + incr(Dir, H, TPid, Dict); -%% An encoded message with errors and an identifiable header ... -incr_error(Dir, {_, _, #diameter_header{} = H}, TPid) -> - incr_error(Dir, H, TPid); +incr(Dir, #diameter_header{} = H, TPid, Dict) -> + incr(TPid, {msg_id(H, Dict), Dir}). -%% ... or not. -incr_error(Dir, {_,_}, TPid) -> - incr(TPid, {unknown, Dir, error}); +%% --------------------------------------------------------------------------- +%% incr_error/4 +%% --------------------------------------------------------------------------- -incr_error(Dir, #diameter_header{} = H, TPid) -> - incr_error(Dir, diameter_codec:msg_id(H), TPid); +%% Decoded message without errors. +incr_error(recv, #diameter_packet{errors = []}, _, _) -> + ok; -incr_error(Dir, {_,_,_} = Id, TPid) -> - incr(TPid, {Id, Dir, error}); +incr_error(recv = D, #diameter_packet{header = H}, TPid, Dict) -> + incr_error(D, H, TPid, Dict); -incr_error(_, _, _) -> - false. +%% Encoded message with errors and an identifiable header ... +incr_error(send = D, {_, _, #diameter_header{} = H}, TPid, Dict) -> + incr_error(D, H, TPid, Dict); +%% ... or not. +incr_error(send = D, {_,_}, TPid, _) -> + incr_error(D, unknown, TPid); + +incr_error(Dir, #diameter_header{} = H, TPid, Dict) -> + incr_error(Dir, msg_id(H, Dict), TPid); + +incr_error(Dir, Id, TPid, _) -> + incr_error(Dir, Id, TPid). + +incr_error(Dir, Id, TPid) -> + incr(TPid, {Id, Dir, error}). + %% --------------------------------------------------------------------------- %% incr_rc/4 %% --------------------------------------------------------------------------- --spec incr_rc(send|recv, #diameter_packet{}, TPid, Dict0) +-spec incr_rc(send|recv, Pkt, TPid, Dict0) -> {Counter, non_neg_integer()} | Reason - when TPid :: pid(), + when Pkt :: #diameter_packet{}, + TPid :: pid(), Dict0 :: module(), Counter :: {'Result-Code', integer()} | {'Experimental-Result', integer(), integer()}, @@ -264,8 +279,9 @@ recv_R({#diameter_app{id = Id, dictionary = Dict} = App, Caps}, Pkt0, Dict0, RecvData) -> + incr(recv, Pkt0, TPid, Dict), Pkt = errors(Id, diameter_codec:decode(Id, Dict, Pkt0)), - incr_error(recv, Pkt, TPid), + incr_error(recv, Pkt, TPid, Dict), {Caps, Pkt, App, recv_R(App, TPid, Dict0, Caps, RecvData, Pkt)}; %% Note that the decode is different depending on whether or not Id is %% ?APP_ID_RELAY. @@ -656,6 +672,7 @@ reply(Msg, Dict, TPid, Dict0, Fs, ReqPkt) -> TPid, reset(make_answer_packet(Msg, ReqPkt), Dict, Dict0), Fs), + incr(send, Pkt, TPid, Dict), incr_rc(send, Pkt, Dict, TPid, Dict0), %% count outgoing send(TPid, Pkt). @@ -1040,10 +1057,10 @@ incr_rc(Dir, Pkt, Dict, TPid, Dict0) -> errors = Es} = Pkt, - Id = diameter_codec:msg_id(Hdr), + Id = msg_id(Hdr, Dict), %% Count incoming decode errors. - recv /= Dir orelse [] == Es orelse incr_error(Dir, Id, TPid), + recv /= Dir orelse [] == Es orelse incr_error(Dir, Id, TPid, Dict), %% Exit on a missing result code. T = rc_counter(Dict, Msg), @@ -1054,7 +1071,15 @@ incr_rc(Dir, Pkt, Dict, TPid, Dict0) -> is_result(RC, E, Dict0) orelse ?LOGX(invalid_error_bit, {Dict, Dir, Hdr, RC}), - incr(TPid, {Id, Dir, Ctr}). + incr(TPid, {Id, Dir, Ctr}), + Ctr. + +%% Only count on known keeps so as not to be vulnerable to attack: +%% there are 2^32 (application ids) * 2^24 (command codes) * 2 (R-bits) +%% = 2^57 Ids for an attacker to choose from. +msg_id(Hdr, Dict) -> + {_ApplId, Code, R} = Id = diameter_codec:msg_id(Hdr), + choose('' == Dict:msg_name(Code, 0 == R), unknown, Id). %% No E-bit: can't be 3xxx. is_result(RC, false, _Dict0) -> @@ -1072,8 +1097,8 @@ is_result(RC, true, _) -> %% incr/2 -incr(TPid, {_, _, T} = Counter) -> - {T, diameter_stats:incr(Counter, TPid, 1)}. +incr(TPid, Counter) -> + diameter_stats:incr(Counter, TPid, 1). %% rc_counter/2 @@ -1423,6 +1448,8 @@ handle_answer(SvcName, %% want to examine the answer? handle_A(Pkt, SvcName, Dict, Dict0, App, #request{transport = TPid} = Req) -> + incr(recv, Pkt, TPid, Dict), + try incr_rc(recv, Pkt, Dict, TPid, Dict0) %% count incoming of @@ -1547,7 +1574,7 @@ encode(Dict, TPid, #diameter_packet{bin = undefined} = Pkt) -> diameter_codec:encode(Dict, Pkt) catch exit: {diameter_codec, encode, T} = Reason -> - incr_error(send, T, TPid), + incr_error(send, T, TPid, Dict), exit(Reason) end; diff --git a/lib/diameter/src/base/diameter_watchdog.erl b/lib/diameter/src/base/diameter_watchdog.erl index 5e27778432..4616c675ff 100644 --- a/lib/diameter/src/base/diameter_watchdog.erl +++ b/lib/diameter/src/base/diameter_watchdog.erl @@ -471,8 +471,7 @@ encode(dwr = M, Dict0, Mask) -> hop_by_hop_id = Seq}, Pkt = #diameter_packet{header = Hdr, msg = Msg}, - #diameter_packet{bin = Bin} = diameter_codec:encode(Dict0, Pkt), - Bin; + diameter_codec:encode(Dict0, Pkt); encode(dwa, Dict0, #diameter_packet{header = H, transport_data = TD} = ReqPkt) -> @@ -541,10 +540,14 @@ send_watchdog(#watchdog{pending = false, dictionary = Dict0, sequence = Mask} = S) -> - send(TPid, {send, encode(dwr, Dict0, Mask)}), + #diameter_packet{bin = Bin} = EPkt = encode(dwr, Dict0, Mask), + diameter_traffic:incr(send, EPkt, TPid, Dict0), + send(TPid, {send, Bin}), ?LOG(send, 'DWR'), S#watchdog{pending = true}. +%% Dont' count encode errors since we don't expect any on DWR/DWA. + %% recv/3 recv(Name, Pkt, S) -> @@ -563,8 +566,10 @@ rcv('DWR', Pkt, #watchdog{transport = TPid, dictionary = Dict0}) -> ?LOG(recv, 'DWR'), DPkt = diameter_codec:decode(Dict0, Pkt), - diameter_traffic:incr_error(recv, DPkt, TPid), + diameter_traffic:incr(recv, DPkt, TPid, Dict0), + diameter_traffic:incr_error(recv, DPkt, TPid, Dict0), EPkt = encode(dwa, Dict0, Pkt), + diameter_traffic:incr(send, EPkt, TPid, Dict0), diameter_traffic:incr_rc(send, EPkt, TPid, Dict0), send(TPid, {send, EPkt}), @@ -573,6 +578,7 @@ rcv('DWR', Pkt, #watchdog{transport = TPid, rcv('DWA', Pkt, #watchdog{transport = TPid, dictionary = Dict0}) -> ?LOG(recv, 'DWA'), + diameter_traffic:incr(recv, Pkt, TPid, Dict0), diameter_traffic:incr_rc(recv, diameter_codec:decode(Dict0, Pkt), TPid, -- cgit v1.2.3 From 6ff5384c118a3382823a99ccbb5b82d68c7a8e74 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 24 May 2014 07:39:31 +0200 Subject: Make example code quiet The output could make it impossible to use the shell. Counters returned diameter:service_info/2 can be used to check for expected happenings. --- lib/diameter/examples/code/client.erl | 3 ++- lib/diameter/examples/code/client_cb.erl | 14 +------------- lib/diameter/examples/code/redirect_cb.erl | 8 +++----- lib/diameter/examples/code/relay_cb.erl | 8 +++----- lib/diameter/examples/code/server_cb.erl | 8 +++----- 5 files changed, 12 insertions(+), 29 deletions(-) diff --git a/lib/diameter/examples/code/client.erl b/lib/diameter/examples/code/client.erl index bfe71b0e56..6606ac254d 100644 --- a/lib/diameter/examples/code/client.erl +++ b/lib/diameter/examples/code/client.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2012. All Rights Reserved. +%% Copyright Ericsson AB 2010-2014. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -72,6 +72,7 @@ {'Product-Name', "Client"}, {'Auth-Application-Id', [?DIAMETER_APP_ID_COMMON]}, {application, [{alias, ?APP_ALIAS}, + {answer_errors, discard}, {dictionary, ?DIAMETER_DICT_COMMON}, {module, ?CALLBACK_MOD}]}]). diff --git a/lib/diameter/examples/code/client_cb.erl b/lib/diameter/examples/code/client_cb.erl index ee3dcb2fec..843cdd9262 100644 --- a/lib/diameter/examples/code/client_cb.erl +++ b/lib/diameter/examples/code/client_cb.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2012. All Rights Reserved. +%% Copyright Ericsson AB 2010-2014. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -77,23 +77,11 @@ prepare_retransmit(Packet, SvcName, Peer) -> %% handle_answer/4 -%% Since client.erl has detached the call when using the list -%% encoding and not otherwise, output to the terminal in the -%% the former case, return in the latter. - -handle_answer(#diameter_packet{msg = Msg}, Request, _SvcName, _Peer) - when is_list(Request) -> - io:format("answer: ~p~n", [Msg]); - handle_answer(#diameter_packet{msg = Msg}, _Request, _SvcName, _Peer) -> {ok, Msg}. %% handle_error/4 -handle_error(Reason, Request, _SvcName, _Peer) - when is_list(Request) -> - io:format("error: ~p~n", [Reason]); - handle_error(Reason, _Request, _SvcName, _Peer) -> {error, Reason}. diff --git a/lib/diameter/examples/code/redirect_cb.erl b/lib/diameter/examples/code/redirect_cb.erl index 69836774a1..8d98b0d2df 100644 --- a/lib/diameter/examples/code/redirect_cb.erl +++ b/lib/diameter/examples/code/redirect_cb.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2013. All Rights Reserved. +%% Copyright Ericsson AB 2010-2014. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -34,12 +34,10 @@ -define(UNEXPECTED, erlang:error({unexpected, ?MODULE, ?LINE})). -peer_up(_SvcName, {PeerRef, _}, State) -> - io:format("up: ~p~n", [PeerRef]), +peer_up(_SvcName, _Peer, State) -> State. -peer_down(_SvcName, {PeerRef, _}, State) -> - io:format("down: ~p~n", [PeerRef]), +peer_down(_SvcName, _Peer, State) -> State. pick_peer(_, _, _SvcName, _State) -> diff --git a/lib/diameter/examples/code/relay_cb.erl b/lib/diameter/examples/code/relay_cb.erl index 9f9cd8d5ae..68798014e6 100644 --- a/lib/diameter/examples/code/relay_cb.erl +++ b/lib/diameter/examples/code/relay_cb.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2012. All Rights Reserved. +%% Copyright Ericsson AB 2010-2014. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -32,12 +32,10 @@ handle_error/5, handle_request/3]). -peer_up(_SvcName, {PeerRef, _}, State) -> - io:format("up: ~p~n", [PeerRef]), +peer_up(_SvcName, _Peer, State) -> State. -peer_down(_SvcName, {PeerRef, _}, State) -> - io:format("down: ~p~n", [PeerRef]), +peer_down(_SvcName, _Peer, State) -> State. %% Returning 'relay' from handle_request causes diameter to resend the diff --git a/lib/diameter/examples/code/server_cb.erl b/lib/diameter/examples/code/server_cb.erl index 0f6eb32ed6..68901099e3 100644 --- a/lib/diameter/examples/code/server_cb.erl +++ b/lib/diameter/examples/code/server_cb.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2012. All Rights Reserved. +%% Copyright Ericsson AB 2010-2014. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -38,12 +38,10 @@ -define(UNEXPECTED, erlang:error({unexpected, ?MODULE, ?LINE})). -peer_up(_SvcName, {PeerRef, _}, State) -> - io:format("up: ~p~n", [PeerRef]), +peer_up(_SvcName, _Peer, State) -> State. -peer_down(_SvcName, {PeerRef, _}, State) -> - io:format("down: ~p~n", [PeerRef]), +peer_down(_SvcName, _Peer, State) -> State. pick_peer(_, _, _SvcName, _State) -> -- cgit v1.2.3 From 11e3a1b7f21fbb4419043b36ea9cf8c2d53aa7b5 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 24 May 2014 10:56:22 +0200 Subject: Make example server answer unsupported requests with 3001 As it should. The previous discard (surely) pre-dated being able to return {answer_message, 3001} from a handle_request callback. --- lib/diameter/examples/code/server_cb.erl | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/diameter/examples/code/server_cb.erl b/lib/diameter/examples/code/server_cb.erl index 68901099e3..76d922c112 100644 --- a/lib/diameter/examples/code/server_cb.erl +++ b/lib/diameter/examples/code/server_cb.erl @@ -88,16 +88,15 @@ handle_request(#diameter_packet{msg = Req}, _SvcName, {_, Caps}) {reply, Ans}; -%% Should really reply to other base messages that we don't support -%% but simply discard them instead. -handle_request(#diameter_packet{}, _SvcName, {_,_}) -> - discard. +%% Answer that any other message is unsupported. +handle_request(#diameter_packet{}, _SvcName, _) -> + {answer_message, 3001}. %% DIAMETER_COMMAND_UNSUPPORTED %% --------------------------------------------------------------------------- %% Answer using the record or list encoding depending on %% Re-Auth-Request-Type. This is just as an example. You would -%% typically just choose one, and this has nothing to do with the how +%% typically just choose one, and this has nothing to do with how %% client.erl sends. answer(0, Id, OH, OR) -> -- cgit v1.2.3 From 2d5e6f4d438db1a480e44c539155e334c11851bc Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 24 May 2014 12:13:04 +0200 Subject: Simplify example server In particular, remove the unnecessary list-or-record answer. --- lib/diameter/examples/code/server_cb.erl | 38 ++++++++++++-------------------- 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/lib/diameter/examples/code/server_cb.erl b/lib/diameter/examples/code/server_cb.erl index 76d922c112..9d8d395d06 100644 --- a/lib/diameter/examples/code/server_cb.erl +++ b/lib/diameter/examples/code/server_cb.erl @@ -66,10 +66,13 @@ handle_request(#diameter_packet{msg = Req, errors = []}, _SvcName, {_, Caps}) origin_realm = {OR,_}} = Caps, #diameter_base_RAR{'Session-Id' = Id, - 'Re-Auth-Request-Type' = RT} + 'Re-Auth-Request-Type' = Type} = Req, - {reply, answer(RT, Id, OH, OR)}; + {reply, #diameter_base_RAA{'Result-Code' = rc(Type), + 'Origin-Host' = OH, + 'Origin-Realm' = OR, + 'Session-Id' = Id}}; %% ... or one that wasn't. 3xxx errors are answered by diameter itself %% but these are 5xxx errors for which we must contruct a reply. @@ -82,31 +85,18 @@ handle_request(#diameter_packet{msg = Req}, _SvcName, {_, Caps}) #diameter_base_RAR{'Session-Id' = Id} = Req, - Ans = #diameter_base_RAA{'Origin-Host' = OH, - 'Origin-Realm' = OR, - 'Session-Id' = Id}, - - {reply, Ans}; + {reply, #diameter_base_RAA{'Origin-Host' = OH, + 'Origin-Realm' = OR, + 'Session-Id' = Id}}; %% Answer that any other message is unsupported. handle_request(#diameter_packet{}, _SvcName, _) -> {answer_message, 3001}. %% DIAMETER_COMMAND_UNSUPPORTED -%% --------------------------------------------------------------------------- - -%% Answer using the record or list encoding depending on -%% Re-Auth-Request-Type. This is just as an example. You would -%% typically just choose one, and this has nothing to do with how -%% client.erl sends. - -answer(0, Id, OH, OR) -> - #diameter_base_RAA{'Result-Code' = 2001, %% DIAMETER_SUCCESS - 'Origin-Host' = OH, - 'Origin-Realm' = OR, - 'Session-Id' = Id}; +%% Map Re-Auth-Request-Type to Result-Code just for the purpose of +%% generating different answers. -answer(_, Id, OH, OR) -> - ['RAA', {'Result-Code', 5012}, %% DIAMETER_UNABLE_TO_COMPLY - {'Origin-Host', OH}, - {'Origin-Realm', OR}, - {'Session-Id', Id}]. +rc(0) -> + 2001; %% DIAMETER_SUCCESS +rc(_) -> + 5012. %% DIAMETER_UNABLE_TO_COMPLY -- cgit v1.2.3 From da356f2e79b2cf5e767cb11cc643beab086e5263 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 26 May 2014 16:55:06 +0200 Subject: Add testcases that send unknown AVPs with a bad AVP Length In particular, a length that points past the end of the message. This goes undetected there is some other problem with the AVP (eg. M-bit), which is a problem we're about to fix. --- lib/diameter/test/diameter_traffic_SUITE.erl | 52 ++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index a97c54fc04..89592f02ef 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2013. All Rights Reserved. +%% Copyright Ericsson AB 2010-2014. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -43,7 +43,9 @@ send_protocol_error/1, send_arbitrary/1, send_unknown/1, + send_unknown_short/1, send_unknown_mandatory/1, + send_unknown_short_mandatory/1, send_noreply/1, send_unsupported/1, send_unsupported_app/1, @@ -266,7 +268,9 @@ tc() -> send_protocol_error, send_arbitrary, send_unknown, + send_unknown_short, send_unknown_mandatory, + send_unknown_short_mandatory, send_noreply, send_unsupported, send_unsupported_app, @@ -447,6 +451,24 @@ send_unknown(Config) -> data = <<17>>}]} = lists:last(Avps). +%% Ditto, and point the AVP length past the end of the message. Expect +%% 5014. +send_unknown_short(Config) -> + send_unknown_short(Config, false, ?INVALID_AVP_LENGTH). + +send_unknown_short(Config, M, RC) -> + Req = ['ASR', {'AVP', [#diameter_avp{code = 999, + is_mandatory = M, + data = <<17>>}]}], + ['ASA', _SessionId, {'Result-Code', RC} | Avps] + = call(Config, Req), + [#'diameter_base_Failed-AVP'{'AVP' = As}] + = proplists:get_value('Failed-AVP', Avps), + [#diameter_avp{code = 999, + is_mandatory = M, + data = <<17, _/binary>>}] %% extra bits from padding + = As. + %% Ditto but set the M flag. send_unknown_mandatory(Config) -> Req = ['ASR', {'AVP', [#diameter_avp{code = 999, @@ -461,6 +483,11 @@ send_unknown_mandatory(Config) -> data = <<17>>}] = As. +%% Ditto, and point the AVP length past the end of the message. Expect +%% 5014 instead of 5001. +send_unknown_short_mandatory(Config) -> + send_unknown_short(Config, true, ?INVALID_AVP_LENGTH). + %% Send an STR that the server ignores. send_noreply(Config) -> Req = ['STR', {'Termination-Cause', ?BAD_ANSWER}], @@ -835,6 +862,26 @@ log(#diameter_packet{bin = Bin} = P, T) %% prepare/4 +prepare(Pkt, Caps, N, #group{client_dict0 = Dict0} = Group) + when N == send_unknown_short_mandatory; + N == send_unknown_short -> + Req = prepare(Pkt, Caps, Group), + + #diameter_packet{header = #diameter_header{length = L}, + bin = Bin} + = E + = diameter_codec:encode(Dict0, Pkt#diameter_packet{msg = Req}), + + %% Find the unknown AVP data at the end of the message and alter + %% its length header. + + {Padding, [17|_]} = lists:splitwith(fun(C) -> C == 0 end, + lists:reverse(binary_to_list(Bin))), + + Offset = L - length(Padding) - 4, + <> = Bin, + E#diameter_packet{bin = <>}; + prepare(Pkt, Caps, N, #group{client_dict0 = Dict0} = Group) when N == send_long_avp_length; N == send_short_avp_length; @@ -997,7 +1044,8 @@ answer(Rec, [_|_], N) N == send_short_avp_length; N == send_zero_avp_length; N == send_invalid_avp_length; - N == send_invalid_reject -> + N == send_invalid_reject; + N == send_unknown_short_mandatory -> Rec; answer(Rec, [], _) -> Rec. -- cgit v1.2.3 From 58a74f2b6b6bd604d60fa8dca347cdd4ad2e4a3b Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 23 May 2014 18:28:19 +0200 Subject: Fix handling of AVP length errors (5014) in unknown AVPs Commit 4ce2d3a6 added the insertion of a single bit into binary AVP data to induce an encode error in the case of a header length that pointed past the available bytes: a 5014 = DIAMETER_INVALID_AVP_LENGTH error. Commit 838856b fixed this for stringish Diameter types, but both commits neglected the case in which the offending AVP isn't known to the dictionary in question. Unless the AVP was regarded as erroneous for other reasons (eg. an M-bit resulting in 5001) it would be happily be packed into an 'AVP' field. If it was regarded as an error, the record could be passed back to diameter_codec:pack_avp/1, and if the record contained header data then there was no clause to deal with the unpleasantry. Deal with it by having the dictionary module strip the extra bit and flag the AVP as 5014, and by having diameter_codec handle any extra bit coming from an dictionary compiled against an old diameter_gen. An old dictionary won't detect 5014 however, so dictionaries should be recompiled. Change most of the guards in diameter_codec from is_bitstring/1 to is_binary/1. What's being passed to the decode functions are binaries received other the network. The only case in which a non-binary bitstring is when we've placed an extra bit there ourselves. (Modulo someone doing something they shouldn't.) --- lib/diameter/include/diameter_gen.hrl | 17 +++++++ lib/diameter/src/base/diameter_codec.erl | 77 ++++++++++++++++---------------- 2 files changed, 55 insertions(+), 39 deletions(-) diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index 319ad5a783..ebc10b8918 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -412,6 +412,23 @@ pack_avp(_, Arity, Avp, Acc) -> %% pack_AVP/3 +%% Length failure was induced because of a header/payload length +%% mismatch. The AVP Length is reset to match the received data if +%% this AVP is encoded in an answer message, since the length is +%% computed. +%% +%% Data is a truncated header if command_code = undefined, otherwise +%% payload bytes. The former is padded to the length of a header if +%% the AVP reaches an outgoing encode in diameter_codec. +%% +%% RFC 6733 says that an AVP returned with 5014 can contain a minimal +%% payload for the AVP's type, but in this case we don't know the +%% type. + +pack_AVP(_, #diameter_avp{data = <<0:1, Data/binary>>} = Avp, Acc) -> + {Rec, Failed} = Acc, + {Rec, [{5014, Avp#diameter_avp{data = Data}} | Failed]}; + pack_AVP(Name, #diameter_avp{is_mandatory = M} = Avp, Acc) -> case pack_arity(Name, M) of 0 -> diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index 84139d7f5e..0ca4a84d21 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -237,7 +237,7 @@ rec2msg(Mod, Rec) -> %% Unsuccessfully decoded AVPs will be placed in #diameter_packet.errors. --spec decode(module(), #diameter_packet{} | bitstring()) +-spec decode(module(), #diameter_packet{} | binary()) -> #diameter_packet{}. decode(Mod, Pkt) -> @@ -271,7 +271,7 @@ decode(_, Mod, #diameter_packet{header = Hdr} = Pkt) -> decode_avps(MsgName, Mod, Pkt, collect_avps(Pkt)); decode(Id, Mod, Bin) - when is_bitstring(Bin) -> + when is_binary(Bin) -> decode(Id, Mod, #diameter_packet{header = decode_header(Bin), bin = Bin}). decode_avps(MsgName, Mod, Pkt, {E, Avps}) -> @@ -298,7 +298,7 @@ decode_avps(MsgName, Mod, Pkt, Avps) -> %% ... or not %%% # decode_header/1 %%% --------------------------------------------------------------------------- --spec decode_header(bitstring()) +-spec decode_header(binary()) -> #diameter_header{} | false. @@ -309,7 +309,7 @@ decode_header(<>) -> + _/binary>>) -> <> = CmdFlags, %% 3588 (ch 3) says that reserved bits MUST be set to 0 and ignored @@ -422,7 +422,7 @@ msg_id(#diameter_header{application_id = A, is_request = R}) -> {A, C, if R -> 1; true -> 0 end}; -msg_id(<<_:32, Rbit:1, _:7, CmdCode:24, ApplId:32, _/bitstring>>) -> +msg_id(<<_:32, Rbit:1, _:7, CmdCode:24, ApplId:32, _/binary>>) -> {ApplId, CmdCode, Rbit}. %%% --------------------------------------------------------------------------- @@ -433,17 +433,18 @@ msg_id(<<_:32, Rbit:1, _:7, CmdCode:24, ApplId:32, _/bitstring>>) -> %% order in the binary. Note also that grouped avp's aren't unraveled, %% only those at the top level. --spec collect_avps(#diameter_packet{} | bitstring()) +-spec collect_avps(#diameter_packet{} | binary()) -> [Avp] | {Error, [Avp]} when Avp :: #diameter_avp{}, Error :: {5014, #diameter_avp{}}. collect_avps(#diameter_packet{bin = Bin}) -> - <<_:20/binary, Avps/bitstring>> = Bin, + <<_:20/binary, Avps/binary>> = Bin, collect_avps(Avps); -collect_avps(Bin) -> +collect_avps(Bin) + when is_binary(Bin) -> collect_avps(Bin, 0, []). collect_avps(<<>>, _, Acc) -> @@ -473,7 +474,9 @@ collect_avps(Bin, N, Acc) -> split_avp(Bin) -> {Code, V, M, P, Len, HdrLen} = split_head(Bin), - {Data, B} = split_data(Bin, HdrLen, Len - HdrLen), + + <<_:HdrLen/binary, Rest/binary>> = Bin, + {Data, B} = split_data(Rest, Len - HdrLen), {B, #diameter_avp{code = Code, vendor_id = V, @@ -483,17 +486,15 @@ split_avp(Bin) -> %% split_head/1 -split_head(<>) -> +split_head(<>) -> {Code, V, M, P, Len, 12}; -split_head(<>) -> +split_head(<>) -> {Code, undefined, M, P, Len, 8}; -%% Header is truncated: pack_avp/1 will pad to the minimum header -%% length. -split_head(B) - when is_bitstring(B) -> - ?THROW({5014, #diameter_avp{data = B}}). +%% Header is truncated. +split_head(Bin) -> + ?THROW({5014, #diameter_avp{data = Bin}}). %% 3588: %% @@ -528,34 +529,27 @@ split_head(B) %% split_data/3 -split_data(Bin, HdrLen, Len) - when 0 =< Len -> - split_data(Bin, HdrLen, Len, (4 - (Len rem 4)) rem 4); - -split_data(_, _, _) -> - invalid_avp_length(). +split_data(Bin, Len) -> + Pad = (4 - (Len rem 4)) rem 4, -%% split_data/4 + %% Len might be negative here, but that ensures the failure of the + %% binary match. -split_data(Bin, HdrLen, Len, Pad) -> case Bin of - <<_:HdrLen/binary, Data:Len/binary, _:Pad/binary, Rest/bitstring>> -> + <> -> {Data, Rest}; _ -> - invalid_avp_length() + %% Header length points past the end of the message. As + %% stated in the 6733 text above, it's sufficient to + %% return a zero-filled minimal payload if this is a + %% request. Do this (in cases that we know the type) by + %% inducing a decode failure and letting the dictionary's + %% decode (in diameter_gen) deal with it. Here we don't + %% know type. If the type isn't known, then the decode + %% just strips the extra bit. + {<<0:1, Bin/binary>>, <<>>} end. -%% invalid_avp_length/0 -%% -%% AVP Length doesn't mesh with payload. Induce a decode error by -%% returning a payload that no valid Diameter type can have. This is -%% so that a known AVP will result in 5014 error with a zero'd -%% payload. Here we simply don't know how to construct this payload. -%% (Yes, this solution is an afterthought.) - -invalid_avp_length() -> - {<<0:1>>, <<>>}. - %%% --------------------------------------------------------------------------- %%% # pack_avp/1 %%% --------------------------------------------------------------------------- @@ -587,17 +581,22 @@ pack_avp(#diameter_avp{data = {Dict, Name, Value}} = A) -> {Name, Type} = Dict:avp_name(Code, Vid), pack_avp(A#diameter_avp{data = {Hdr, {Type, Value}}}); +%% ... with a truncated header ... pack_avp(#diameter_avp{code = undefined, data = B}) - when is_bitstring(B) -> + when is_binary(B) -> %% Reset the AVP Length of an AVP Header resulting from a 5014 %% error. The RFC doesn't explicitly say to do this but the %% receiver can't correctly extract this and following AVP's %% without a correct length. On the downside, the header doesn't %% reveal if the received header has been padded. Pad = 8*header_length(B) - bit_size(B), - Len = size(<> = <>), + Len = size(<> = <>), <>; +%% ... from a dictionary compiled against old code (diameter_gen) ... +pack_avp(#diameter_avp{data = <<0:1, B/binary>>} = A) -> + pack_avp(A#diameter_avp{data = B}); + %% ... or as an iolist. pack_avp(#diameter_avp{code = Code, vendor_id = V, -- cgit v1.2.3 From 31a7c0b20dbcfdeee946064e610bd51b95be269c Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 26 May 2014 17:47:26 +0200 Subject: Add a testcase that expects a decoded value in Failed-AVP This isn't currently the case, but soon will be. --- lib/diameter/test/diameter_traffic_SUITE.erl | 35 +++++++++++++++++++++------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 89592f02ef..4b67372016 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -56,7 +56,8 @@ send_zero_avp_length/1, send_invalid_avp_length/1, send_invalid_reject/1, - send_unrecognized_mandatory/1, + send_unexpected_mandatory_decode/1, + send_unexpected_mandatory/1, send_long/1, send_nopeer/1, send_noapp/1, @@ -281,7 +282,8 @@ tc() -> send_zero_avp_length, send_invalid_avp_length, send_invalid_reject, - send_unrecognized_mandatory, + send_unexpected_mandatory_decode, + send_unexpected_mandatory, send_long, send_nopeer, send_noapp, @@ -488,6 +490,22 @@ send_unknown_mandatory(Config) -> send_unknown_short_mandatory(Config) -> send_unknown_short(Config, true, ?INVALID_AVP_LENGTH). +%% Send an ACR containing an unexpected mandatory Session-Timeout. +%% Expect 5001, and check that the value in Failed-AVP was decoded. +send_unexpected_mandatory_decode(Config) -> + Req = ['ASR', {'AVP', [#diameter_avp{code = 27, %% Session-Timeout + is_mandatory = true, + data = <<12:32>>}]}], + ['ASA', _SessionId, {'Result-Code', ?AVP_UNSUPPORTED} | Avps] + = call(Config, Req), + [#'diameter_base_Failed-AVP'{'AVP' = As}] + = proplists:get_value('Failed-AVP', Avps), + [#diameter_avp{code = 27, + is_mandatory = true, + value = 12, + data = <<12:32>>}] + = As. + %% Send an STR that the server ignores. send_noreply(Config) -> Req = ['STR', {'Termination-Cause', ?BAD_ANSWER}], @@ -554,9 +572,9 @@ send_invalid_reject(Config) -> ?answer_message(?TOO_BUSY) = call(Config, Req). -%% Send an STR containing a known AVP, but one that's not allowed and -%% sets the M-bit. -send_unrecognized_mandatory(Config) -> +%% Send an STR containing a known AVP, but one that's not expected and +%% that sets the M-bit. +send_unexpected_mandatory(Config) -> Req = ['STR', {'Termination-Cause', ?LOGOUT}], ['STA', _SessionId, {'Result-Code', ?AVP_UNSUPPORTED} | _] @@ -923,8 +941,8 @@ prepare(Pkt, Caps, N, #group{client_dict0 = Dict0} = Group) <> = H0, %% assert E#diameter_packet{bin = <>}; -prepare(Pkt, Caps, send_unrecognized_mandatory, #group{client_dict0 = Dict0} - = Group) -> +prepare(Pkt, Caps, send_unexpected_mandatory, #group{client_dict0 = Dict0} + = Group) -> Req = prepare(Pkt, Caps, Group), #diameter_packet{bin = <>} = E @@ -1045,7 +1063,8 @@ answer(Rec, [_|_], N) N == send_zero_avp_length; N == send_invalid_avp_length; N == send_invalid_reject; - N == send_unknown_short_mandatory -> + N == send_unknown_short_mandatory; + N == send_unexpected_mandatory_decode -> Rec; answer(Rec, [], _) -> Rec. -- cgit v1.2.3 From c2c00fdd4de1b8883e47ec1b5b048659ef033302 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 23 May 2014 00:24:41 +0200 Subject: Do best-effort decode of Failed-AVP Commit 4ce2d3a6 (diameter-1.4.2, OTP-11007) disabled the decode of values in Failed-AVP components since any error caused the decode of Failed-AVP itself to fail. This is less than useful since (1) we should be able to decode it given that we've sent it (modulo mangling on the way to the peer and back), and (2) it's not unheard of to examine Failed-AVP to see what the peer objected to. This commits adds a best-effort decode: decode if possible, otherwise not, using the same abuse of the process dictionary as commit bbdb027c. --- lib/diameter/include/diameter_gen.hrl | 102 ++++++++++++++++++++----------- lib/diameter/src/base/diameter_codec.erl | 3 +- 2 files changed, 69 insertions(+), 36 deletions(-) diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index ebc10b8918..7e91ce375f 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -30,6 +30,10 @@ %% error or not. See is_strict/0. -define(STRICT_KEY, strict). +%% Key that says whether or not we should do a best-effort decode +%% within Failed-AVP. +-define(FAILED_KEY, failed). + -type parent_name() :: atom(). %% parent = Message or AVP -type parent_record() :: tuple(). %% -type avp_name() :: atom(). @@ -286,15 +290,7 @@ decode(Name, 'AVP', Avp, Acc) -> %% d/3 -%% Don't try to decode the value of a Failed-AVP component since it -%% probably won't. Note that matching on 'Failed-AVP' assumes that -%% this is the RFC AVP, with code 279. Strictly, this doesn't need to -%% be the case, so we're assuming no one defines another Failed-AVP. -d('Failed-AVP' = Name, Avp, Acc) -> - decode_AVP(Name, Avp, Acc); - -%% Or try to decode. -d(Name, Avp, {Avps, Acc}) -> +d(Name, Avp, Acc) -> #diameter_avp{name = AvpName, data = Data, type = Type, @@ -307,53 +303,81 @@ d(Name, Avp, {Avps, Acc}) -> %% value around through the entire decode. The solution here is %% simple in comparison, both to implement and to understand. - Reset = relax(Type, M), + Strict = relax(Type, M), + %% Use the process dictionary again to keep track of whether we're + %% decoding within Failed-AVP and should ignore decode errors + %% altogether. + + Failed = relax(Name), %% Not AvpName or else a failed Failed-AVP + %% decode is packed into 'AVP'. try avp(decode, Data, AvpName) of V -> + {Avps, T} = Acc, {H, A} = ungroup(V, Avp), - {[H | Avps], pack_avp(Name, A, Acc)} + {[H | Avps], pack_avp(Name, A, T)} catch error: Reason -> - %% Failures here won't be visible since they're a "normal" - %% occurrence if the peer sends a faulty AVP that we need to - %% respond sensibly to. Log the occurence for traceability, - %% but the peer will also receive info in the resulting - %% answer-message. - Stack = diameter_lib:get_stacktrace(), - diameter_lib:log(decode_error, - ?MODULE, - ?LINE, - {Reason, AvpName, Stack}), - - {Rec, Failed} = Acc, - {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}} + d(undefined == Failed orelse is_failed(), Reason, Name, Avp, Acc) after - relax(Reset) + reset(?STRICT_KEY, Strict), + reset(?FAILED_KEY, Failed) end. +%% Ignore a decode error within Failed-AVP ... +d(true, _, Name, Avp, Acc) -> + decode_AVP(Name, Avp, Acc); + +%% ... or not. Failures here won't be visible since they're a "normal" +%% occurrence if the peer sends a faulty AVP that we need to respond +%% sensibly to. Log the occurence for traceability, but the peer will +%% also receive info in the resulting answer message. +d(false, Reason, Name, Avp, {Avps, Acc}) -> + Stack = diameter_lib:get_stacktrace(), + diameter_lib:log(decode_error, + ?MODULE, + ?LINE, + {Reason, Name, Avp#diameter_avp.name, Stack}), + {Rec, Failed} = Acc, + {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}}. + %% Set false in the process dictionary as soon as we see a Grouped AVP %% that doesn't set the M-bit, so that is_strict() can say whether or %% not to ignore the M-bit on an encapsulated AVP. relax('Grouped', M) -> - V = getr(?STRICT_KEY), - if V == undefined andalso not M -> + case getr(?STRICT_KEY) of + undefined when not M -> putr(?STRICT_KEY, M); - true -> + _ -> false end; relax(_, _) -> false. -%% Reset strictness. -relax(undefined) -> - eraser(?STRICT_KEY); -relax(false) -> - ok. - is_strict() -> false /= getr(?STRICT_KEY). +%% Set true in the process dictionary as soon as we see Failed-AVP. +%% Matching on 'Failed-AVP' assumes that this is the RFC AVP. +%% Strictly, this doesn't need to be the case. +relax('Failed-AVP') -> + case getr(?FAILED_KEY) of + undefined -> + putr(?FAILED_KEY, true); + true = Yes -> + Yes + end; +relax(_) -> + is_failed(). + +is_failed() -> + true == getr(?FAILED_KEY). + +reset(Key, undefined) -> + eraser(Key); +reset(_, _) -> + ok. + %% decode_AVP/3 %% %% Don't know this AVP: see if it can be packed in an 'AVP' field @@ -441,7 +465,15 @@ pack_AVP(Name, #diameter_avp{is_mandatory = M} = Avp, Acc) -> %% Give Failed-AVP special treatment since it'll contain any %% unrecognized mandatory AVP's. pack_arity(Name, M) -> - case Name /= 'Failed-AVP' andalso M andalso is_strict() of + NF = Name /= 'Failed-AVP' andalso not is_failed(), + %% Not testing just Name /= 'Failed-AVP' means we're changing the + %% packing of AVPs nested within Failed-AVP, but the point of + %% ignoring errors within Failed-AVP is to decode as much as + %% possible, and failing because a mandatory AVP couldn't be + %% packed into a dedicated field defeats that point. Note that we + %% can't just test not is_failed() since this will be 'true' when + %% packing an unknown AVP directly within Failed-AVP. + case NF andalso M andalso is_strict() of true -> 0; false -> diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index 0ca4a84d21..06a4f5de64 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -593,7 +593,8 @@ pack_avp(#diameter_avp{code = undefined, data = B}) Len = size(<> = <>), <>; -%% ... from a dictionary compiled against old code (diameter_gen) ... +%% ... from a dictionary compiled against old code in diameter_gen ... +%% ... when ignoring errors in Failed-AVP ... pack_avp(#diameter_avp{data = <<0:1, B/binary>>} = A) -> pack_avp(A#diameter_avp{data = B}); -- cgit v1.2.3