From a1df50b38826370b84ec557de401bc1b09d56de7 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 2 May 2015 10:14:58 +0200 Subject: Don't confuse Result-Code and Experimental-Result Decode of an answer message not setting the E-bit, and containing Experiment-Result but not Result-Code, identified Result-Code as the erroneous when Erroneous-Result-Code was 3xxx. Here's an example (from trace) of a the errors field after decode: [{5004, {diameter_avp,undefined,undefined,false,false,undefined,'Result-Code', 3001,undefined,undefined}}], The diameter_avp was just constructed from the AVP name and decoded result, without regard for which result code AVP contained the value. Fix by extracting the AVP from the incoming message. --- lib/diameter/src/base/diameter_traffic.erl | 158 +++++++++++++++------------ lib/diameter/test/diameter_traffic_SUITE.erl | 60 +++++++++- 2 files changed, 147 insertions(+), 71 deletions(-) (limited to 'lib') diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 538ebeeeba..9a9f7a3197 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -980,8 +980,8 @@ answer_message(OH, OR, RC, Dict0, #diameter_packet{avps = Avps, session_id(Code, Vid, Dict0, Avps) when is_list(Avps) -> try - {value, #diameter_avp{data = D}} = find_avp(Code, Vid, Avps), - [{'Session-Id', [Dict0:avp(decode, D, 'Session-Id')]}] + #diameter_avp{data = Bin} = find_avp(Code, Vid, Avps), + [{'Session-Id', [Dict0:avp(decode, Bin, 'Session-Id')]}] catch error: _ -> [] @@ -998,26 +998,17 @@ failed_avp(_, [] = No) -> %% find_avp/3 -find_avp(Code, Vid, Avps) - when is_integer(Code), (undefined == Vid orelse is_integer(Vid)) -> - find(fun(A) -> is_avp(Code, Vid, A) end, Avps). +%% Grouped ... +find_avp(Code, VId, [[#diameter_avp{code = Code, vendor_id = VId} | _] = As + | _]) -> + As; -%% The final argument here could be a list of AVP's, depending on the case, -%% but we're only searching at the top level. -is_avp(Code, Vid, #diameter_avp{code = Code, vendor_id = Vid}) -> - true; -is_avp(_, _, _) -> - false. +%% ... or not. +find_avp(Code, VId, [#diameter_avp{code = Code, vendor_id = VId} = A | _]) -> + A; -find(_, []) -> - false; -find(Pred, [H|T]) -> - case Pred(H) of - true -> - {value, H}; - false -> - find(Pred, T) - end. +find_avp(Code, VId, [_ | Avps]) -> + find_avp(Code, VId, Avps). %% 7. Error Handling %% @@ -1086,7 +1077,6 @@ incr_result(_, #diameter_packet{msg = undefined = No}, _, _) -> incr_result(Dir, Pkt, TPid, {Dict, AppDict, Dict0}) -> #diameter_packet{header = #diameter_header{is_error = E} = Hdr, - msg = Msg, errors = Es} = Pkt, @@ -1096,13 +1086,13 @@ incr_result(Dir, Pkt, TPid, {Dict, AppDict, Dict0}) -> recv /= Dir orelse [] == Es orelse incr_error(Dir, Id, TPid, AppDict), %% Exit on a missing result code. - T = rc_counter(Dict, Msg), + T = rc_counter(Dict, Dir, Pkt), T == false andalso ?LOGX(no_result_code, {Dict, Dir, Hdr}), - {Ctr, RC} = T, + {Ctr, RC, Avp} = T, %% Or on an inappropriate value. is_result(RC, E, Dict0) - orelse ?LOGX(invalid_error_bit, {Dict, Dir, Hdr, RC}), + orelse ?LOGX(invalid_error_bit, {Dict, Dir, Hdr, Avp}), incr(TPid, {Id, Dir, Ctr}), Ctr. @@ -1148,7 +1138,7 @@ is_result(RC, true, _) -> incr(TPid, Counter) -> diameter_stats:incr(Counter, TPid, 1). -%% rc_counter/2 +%% rc_counter/3 %% RFC 3588, 7.6: %% @@ -1156,39 +1146,45 @@ incr(TPid, Counter) -> %% applications MUST include either one Result-Code AVP or one %% Experimental-Result AVP. -rc_counter(Dict, Msg) -> - rcc(Dict, Msg, int(get_avp_value(Dict, 'Result-Code', Msg))). +rc_counter(Dict, recv, #diameter_packet{header = H, avps = As}) -> + rc_counter(Dict, [H|As]); -rcc(Dict, Msg, undefined) -> - rcc(get_avp_value(Dict, 'Experimental-Result', Msg)); +rc_counter(Dict, _, #diameter_packet{msg = Msg}) -> + rc_counter(Dict, Msg). + +rc_counter(Dict, Msg) -> + rcc(get_result(Dict, Msg)). -rcc(_, _, N) +rcc(#diameter_avp{name = 'Result-Code' = Name, value = N} = A) when is_integer(N) -> - {{'Result-Code', N}, N}. + {{Name, N}, N, A}; -%% Outgoing answers may be in any of the forms messages can be sent -%% in. Incoming messages will be records. We're assuming here that the -%% arity of the result code AVP's is 0 or 1. +rcc(#diameter_avp{name = 'Result-Code' = Name, value = [N|_]} = A) + when is_integer(N) -> + {{Name, N}, N, A}; -rcc([{_,_,N} = T | _]) +rcc(#diameter_avp{name = 'Experimental-Result', value = {_,_,N} = T} = A) when is_integer(N) -> - {T,N}; -rcc({_,_,N} = T) + {T, N, A}; + +rcc(#diameter_avp{name = 'Experimental-Result', value = [{_,_,N} = T|_]} = A) when is_integer(N) -> - {T,N}; + {T, N, A}; + rcc(_) -> false. -%% Extract the first good looking integer. There's no guarantee -%% that what we're looking for has arity 1. -int([N|_]) - when is_integer(N) -> - N; -int(N) - when is_integer(N) -> - N; -int(_) -> - undefined. +%% get_result/2 + +get_result(Dict, Msg) -> + try + [throw(A) || N <- ['Result-Code', 'Experimental-Result'], + #diameter_avp{} = A <- [get_avp(Dict, N, Msg)]] + of + [] -> false + catch + #diameter_avp{} = A -> A + end. x(T) -> exit(T). @@ -1528,10 +1524,10 @@ handle_A(Pkt, SvcName, Dict, Dict0, App, #request{transport = TPid} = Req) -> %% a missing AVP. If both are optional in the dictionary %% then this isn't a decode error: just continue on. answer(Pkt, SvcName, App, Req); - exit: {invalid_error_bit, {_, _, _, RC}} -> + exit: {invalid_error_bit, {_, _, _, Avp}} -> #diameter_packet{errors = Es} = Pkt, - E = {5004, #diameter_avp{name = 'Result-Code', value = RC}}, + E = {5004, Avp}, answer(Pkt#diameter_packet{errors = [E|Es]}, SvcName, App, Req) end. @@ -1868,7 +1864,7 @@ str([]) -> str(T) -> T. -%% get_avp_value/3 +%% get_avp/3 %% %% Find an AVP in a message of one of three forms: %% @@ -1885,47 +1881,71 @@ str(T) -> %% look for are in the common dictionary. This is required since the %% relay dictionary doesn't inherit the common dictionary (which maybe %% it should). -get_avp_value(?RELAY, Name, Msg) -> - get_avp_value(?BASE, Name, Msg); +get_avp(?RELAY, Name, Msg) -> + get_avp(?BASE, Name, Msg); -%% Message sent as a header/avps list, probably a relay case but not -%% necessarily. -get_avp_value(Dict, Name, [#diameter_header{} | Avps]) -> +%% Message as a header/avps list. +get_avp(Dict, Name, [#diameter_header{} | Avps]) -> try {Code, _, VId} = Dict:avp_header(Name), - [A|_] = lists:dropwhile(fun(#diameter_avp{code = C, vendor_id = V}) -> - C /= Code orelse V /= VId - end, - Avps), - avp_decode(Dict, Name, A) + find_avp(Code, VId, Avps) + of + A -> + avp_decode(Dict, Name, ungroup(A)) catch error: _ -> undefined end; %% Outgoing message as a name/values list. -get_avp_value(_, Name, [_MsgName | Avps]) -> +get_avp(_, Name, [_MsgName | Avps]) -> case lists:keyfind(Name, 1, Avps) of {_, V} -> - V; + #diameter_avp{name = Name, value = V}; _ -> undefined end; %% Message is typically a record but not necessarily. -get_avp_value(Dict, Name, Rec) -> +get_avp(Dict, Name, Rec) -> try - Dict:'#get-'(Name, Rec) + #diameter_avp{name = Name, value = Dict:'#get-'(Name, Rec)} catch error:_ -> undefined end. +%% get_avp_value/3 + +get_avp_value(Dict, Name, Msg) -> + case get_avp(Dict, Name, Msg) of + #diameter_avp{value = V} -> + V; + undefined = No -> + No + end. + +%% ungroup/1 + +ungroup([Avp|_]) -> + Avp; +ungroup(Avp) -> + Avp. + +%% avp_decode/3 + avp_decode(Dict, Name, #diameter_avp{value = undefined, - data = Bin}) -> - Dict:avp(decode, Bin, Name); -avp_decode(_, _, #diameter_avp{value = V}) -> - V. + data = Bin} + = Avp) -> + try Dict:avp(decode, Bin, Name) of + V -> + Avp#diameter_avp{value = V} + catch + error:_ -> + Avp + end; +avp_decode(_, _, #diameter_avp{} = Avp) -> + Avp. cb(#diameter_app{module = [_|_] = M}, F, A) -> eval(M, F, A); diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 7dd9f39f85..9e1d5e1435 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -41,6 +41,7 @@ send_eval/1, send_bad_answer/1, send_protocol_error/1, + send_experimental_result/1, send_arbitrary/1, send_unknown/1, send_unknown_short/1, @@ -301,6 +302,7 @@ tc() -> send_eval, send_bad_answer, send_protocol_error, + send_experimental_result, send_arbitrary, send_unknown, send_unknown_short, @@ -480,6 +482,14 @@ send_protocol_error(Config) -> ?answer_message(?TOO_BUSY) = call(Config, Req). +%% Send a 3xxx Experimental-Result in an answer not setting the E-bit +%% and missing a Result-Code. +send_experimental_result(Config) -> + Req = ['ACR', {'Accounting-Record-Type', ?EVENT_RECORD}, + {'Accounting-Record-Number', 5}], + ['ACA', _SessionId | _] + = call(Config, Req). + %% Send an ASR with an arbitrary non-mandatory AVP and expect success %% and the same AVP in the reply. send_arbitrary(Config) -> @@ -1144,6 +1154,13 @@ answer(Pkt, Req, _Peer, Name, #group{client_dict0 = Dict0}) -> [R | Vs] = Dict:'#get-'(answer(Ans, Es, Name)), [Dict:rec2msg(R) | Vs]. +%% Missing Result-Codec and inapproriate Experimental-Result-Code. +answer(Rec, Es, send_experimental_result) -> + [{5004, #diameter_avp{name = 'Experimental-Result'}}, + {5005, #diameter_avp{name = 'Result-Code'}}] + = Es, + Rec; + %% An inappropriate E-bit results in a decode error ... answer(Rec, Es, send_bad_answer) -> [{5004, #diameter_avp{name = 'Result-Code'}} | _] = Es, @@ -1175,7 +1192,9 @@ handle_error(Reason, _Req, [$C|_], _Peer, _, _Time) -> %% Note that diameter will set Result-Code and Failed-AVPs if %% #diameter_packet.errors is non-null. -handle_request(#diameter_packet{header = H, msg = M}, _, {_Ref, Caps}) -> +handle_request(#diameter_packet{header = H, msg = M, avps = As}, + _, + {_Ref, Caps}) -> #diameter_header{end_to_end_id = EI, hop_by_hop_id = HI} = H, @@ -1183,10 +1202,12 @@ handle_request(#diameter_packet{header = H, msg = M}, _, {_Ref, Caps}) -> V = EI bsr B, %% assert V = HI bsr B, %% #diameter_caps{origin_state_id = {_,[Id]}} = Caps, - answer(origin(Id), request(M, Caps)). + answer(origin(Id), request(M, [H|As], Caps)). answer(T, {Tag, Action, Post}) -> {Tag, answer(T, Action), Post}; +answer(_, {reply, [#diameter_header{} | _]} = T) -> + T; answer({A,C}, {reply, Ans}) -> answer(C, {reply, msg(Ans, A, diameter_gen_base_rfc3588)}); answer(pkt, {reply, Ans}) @@ -1195,6 +1216,41 @@ answer(pkt, {reply, Ans}) answer(_, T) -> T. +%% request/3 + +%% send_experimental_result +request(#diameter_base_accounting_ACR{'Accounting-Record-Number' = 5}, + [Hdr | Avps], + #diameter_caps{origin_host = {OH, _}, + origin_realm = {OR, _}}) -> + [H,R|T] = [A || N <- ['Origin-Host', + 'Origin-Realm', + 'Session-Id', + 'Accounting-Record-Type', + 'Accounting-Record-Number'], + #diameter_avp{} = A + <- [lists:keyfind(N, #diameter_avp.name, Avps)]], + Ans = [Hdr#diameter_header{is_request = false}, + H#diameter_avp{data = OH}, + R#diameter_avp{data = OR}, + #diameter_avp{name = 'Experimental-Result', + code = 297, + need_encryption = false, + data = [#diameter_avp{data = {?DIAMETER_DICT_COMMON, + 'Vendor-Id', + 123}}, + #diameter_avp{data + = {?DIAMETER_DICT_COMMON, + 'Experimental-Result-Code', + 3987}}]} + | T], + {reply, Ans}; + +request(Msg, _Avps, Caps) -> + request(Msg, Caps). + +%% request/2 + %% send_nok request(#diameter_base_accounting_ACR{'Accounting-Record-Number' = 0}, _) -> -- cgit v1.2.3