From 60ddb0dc9e5c1e28aa01548b01310744fd9ede15 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sun, 15 Feb 2015 14:40:41 +0100 Subject: Fix process dictionary manipulation during message decode Decode can span multiple codec modules, so written entries cannot be tagged on ?MODULE. --- lib/diameter/include/diameter_gen.hrl | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) (limited to 'lib/diameter') diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index bc25f7d472..49287b3f70 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2014. All Rights Reserved. +%% Copyright Ericsson AB 2010-2015. 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,6 +25,9 @@ -define(THROW(T), throw({?MODULE, T})). +%% Tag common to generated dictionaries. +-define(TAG, diameter_gen). + %% Key to a value in the process dictionary that determines whether or %% not an unrecognized AVP setting the M-bit should be regarded as an %% error or not. See is_strict/0. @@ -48,13 +51,20 @@ %% dictionary. putr(K,V) -> - put({?MODULE, K}, V). + put({?TAG, K}, V). getr(K) -> - get({?MODULE, K}). + case get({?TAG, K}) of + undefined -> + V = erase({?MODULE, K}), %% written in old code + V == undefined orelse putr(K,V), + V; + V -> + V + end. eraser(K) -> - erase({?MODULE, K}). + erase({?TAG, K}). %% --------------------------------------------------------------------------- %% # encode_avps/2 -- cgit v1.2.3 From ebe60da137dafe659d9577e1c6411bcc1ea431af Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 16 Feb 2015 09:11:50 +0100 Subject: Don't discard component diameter_avp list on Grouped AVP decode error The AVPs of an incoming Diameter message diameter_codec:decode/2,3 are decoded into a diameter_packet record in two ways: as a message-specific record in the 'msg' field and as a deep list of diameter_avp records in the 'avps' field. The record decode came first; the diameter_avp decode came later to support the Diameter relay application, but can also be convenient for non-relay applications. The diameter_avp representation can be used with outgoing messages, but what exactly is supported for isn't clearly documented. In the diameter_avp list representation, it's AVPs of type Grouped that lead to nesting: instead of a diameter_avp record, a Grouped AVP is represented by a diameter_avp list whose head is the Grouped AVP itself, and whose tail is the list of component AVPs. The diameter_avp decode was broken in the case of decode errors: the Grouped AVP was represented as a bare diameter_avp, and the component records were lost. The decode now produces the intended list. Note that component AVPs that could not be decoded will have 'undefined' in their data field. --- lib/diameter/include/diameter_gen.hrl | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'lib/diameter') diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index 49287b3f70..8916286275 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -323,12 +323,19 @@ d(Name, Avp, Acc) -> %% decode is packed into 'AVP'. Mod = dict(Failed), %% Dictionary to decode in. + %% On decode, a Grouped AVP is represented as a #diameter_avp{} + %% list with AVP as head and component AVPs as tail. On encode, + %% data can be a list of component AVPs. + try Mod:avp(decode, Data, AvpName) of V -> {Avps, T} = Acc, {H, A} = ungroup(V, Avp), {[H | Avps], pack_avp(Name, A, T)} catch + throw: {?TAG, {grouped, RC, ComponentAvps}} -> + {Avps, {Rec, Failed}} = Acc, + {[[Avp | ComponentAvps] | Avps], {Rec, [{RC, Avp} | Failed]}}; error: Reason -> d(undefined == Failed orelse is_failed(), Reason, @@ -599,11 +606,11 @@ value(_, Avp) -> | no_return(). grouped_avp(decode, Name, Data) -> - {Rec, Avps, []} = decode_avps(Name, diameter_codec:collect_avps(Data)), + {Rec, Avps, Es} = decode_avps(Name, diameter_codec:collect_avps(Data)), + [] == Es orelse throw({?TAG, {grouped, 5004, Avps}}), %% decode failure {Rec, Avps}; -%% A failed match here will result in 5004. Note that this is the only -%% AVP type that doesn't just return the decoded record, also -%% returning the list of component AVP's. +%% Note that this is the only AVP type that doesn't just return the +%% decoded record, also returning the list of component AVP's. grouped_avp(encode, Name, Data) -> encode_avps(Name, Data). -- cgit v1.2.3 From a63a93e6f16368fb82d24b8efd8f8ff9aabf8013 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 16 Feb 2015 11:32:53 +0100 Subject: Fix handling of length errors on Grouped AVPs The decode of a Grouped AVP ignored the case that extracting component AVPs with diameter_codec:collect_avps/1 returned a tuple, in the case of a truncated AVP header. --- lib/diameter/include/diameter_gen.hrl | 36 +++++++++++++++++++++++++------- lib/diameter/src/base/diameter_codec.erl | 16 +++++++------- lib/diameter/src/base/diameter_types.erl | 2 +- 3 files changed, 37 insertions(+), 17 deletions(-) (limited to 'lib/diameter') diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index 8916286275..8272904856 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -334,8 +334,9 @@ d(Name, Avp, Acc) -> {[H | Avps], pack_avp(Name, A, T)} catch throw: {?TAG, {grouped, RC, ComponentAvps}} -> - {Avps, {Rec, Failed}} = Acc, - {[[Avp | ComponentAvps] | Avps], {Rec, [{RC, Avp} | Failed]}}; + {Avps, {Rec, Errors}} = Acc, + A = trim(Avp), + {[[A | trim(ComponentAvps)] | Avps], {Rec, [{RC, A} | Errors]}}; error: Reason -> d(undefined == Failed orelse is_failed(), Reason, @@ -355,6 +356,10 @@ d(Name, Avp, Acc) -> trim(#diameter_avp{data = <<0:1, Bin/binary>>} = Avp) -> Avp#diameter_avp{data = Bin}; +trim(Avps) + when is_list(Avps) -> + lists:map(fun trim/1, Avps); + trim(Avp) -> Avp. @@ -599,22 +604,37 @@ value(_, Avp) -> %% # grouped_avp/3 %% --------------------------------------------------------------------------- --spec grouped_avp(decode, avp_name(), binary()) +-spec grouped_avp(decode, avp_name(), bitstring()) -> {avp_record(), [avp()]}; (encode, avp_name(), avp_record() | avp_values()) -> binary() | no_return(). +%% Length error induced by diameter_codec:collect_avps/1. +grouped_avp(decode, _Name, <<0:1, _/binary>>) -> + throw({?TAG, {grouped, 5014, []}}); + grouped_avp(decode, Name, Data) -> - {Rec, Avps, Es} = decode_avps(Name, diameter_codec:collect_avps(Data)), - [] == Es orelse throw({?TAG, {grouped, 5004, Avps}}), %% decode failure - {Rec, Avps}; -%% Note that this is the only AVP type that doesn't just return the -%% decoded record, also returning the list of component AVP's. + grouped_decode(Name, diameter_codec:collect_avps(Data)); grouped_avp(encode, Name, Data) -> encode_avps(Name, Data). +%% grouped_decode/2 +%% +%% Note that Grouped is the only AVP type that doesn't just return a +%% decoded value, also returning the list of component diameter_avp +%% records. + +grouped_decode(_Name, {Error, Acc}) -> + {RC, Avp} = Error, + throw({?TAG, {grouped, RC, [Avp | Acc]}}); + +grouped_decode(Name, ComponentAvps) -> + {Rec, Avps, Es} = decode_avps(Name, ComponentAvps), + [] == Es orelse throw({?TAG, {grouped, 5004, Avps}}), %% decode failure + {Rec, Avps}. + %% --------------------------------------------------------------------------- %% # empty_group/1 %% --------------------------------------------------------------------------- diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index a2b04bfd63..dd9039d7dc 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -561,14 +561,14 @@ split_data(Bin, Len) -> <> -> {Data, Rest}; _ -> - %% 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. + %% Header length points past the end of the message, or + %% doesn't span the header. 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. diff --git a/lib/diameter/src/base/diameter_types.erl b/lib/diameter/src/base/diameter_types.erl index ca3338be5f..442d90c98b 100644 --- a/lib/diameter/src/base/diameter_types.erl +++ b/lib/diameter/src/base/diameter_types.erl @@ -75,7 +75,7 @@ %% message indicating this error MUST include the offending AVPs %% within a Failed-AVP AVP. %% --define(INVALID_LENGTH(Bin), erlang:error({'DIAMETER', 5014, Bin})). +-define(INVALID_LENGTH(Bitstr), erlang:error({'DIAMETER', 5014, Bitstr})). %% ------------------------------------------------------------------------- %% 3588, 4.2. Basic AVP Data Formats -- cgit v1.2.3 From 545d06c58377938877f1a29a4a078c12644be676 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Tue, 17 Feb 2015 15:42:41 +0100 Subject: Add testcases for diameter_avp decode --- lib/diameter/test/diameter_codec_SUITE.erl | 188 ++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 3 deletions(-) (limited to 'lib/diameter') diff --git a/lib/diameter/test/diameter_codec_SUITE.erl b/lib/diameter/test/diameter_codec_SUITE.erl index cd8ca41f66..64ea90554d 100644 --- a/lib/diameter/test/diameter_codec_SUITE.erl +++ b/lib/diameter/test/diameter_codec_SUITE.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2013. All Rights Reserved. +%% Copyright Ericsson AB 2010-2015. 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 @@ -29,6 +29,9 @@ -export([suite/0, all/0, + groups/0, + init_per_group/2, + end_per_group/2, init_per_testcase/2, end_per_testcase/2]). @@ -36,9 +39,13 @@ -export([base/1, gen/1, lib/1, - unknown/1]). + unknown/1, + success/1, + grouped_error/1, + failed_error/1]). -include("diameter_ct.hrl"). +-include("diameter.hrl"). -define(L, atom_to_list). @@ -48,7 +55,19 @@ suite() -> [{timetrap, {seconds, 10}}]. all() -> - [base, gen, lib, unknown]. + [base, gen, lib, unknown, {group, recode}]. + +groups() -> + [{recode, [], [success, + grouped_error, + failed_error]}]. + +init_per_group(recode, Config) -> + ok = diameter:start(), + Config. + +end_per_group(_, _) -> + ok = diameter:stop(). init_per_testcase(gen, Config) -> [{application, ?APP, App}] = diameter_util:consult(?APP, app), @@ -98,3 +117,166 @@ compile(File) -> compile(File, Opts) -> compile:file(File, [return | Opts]). + +%% =========================================================================== + +%% Ensure a Grouped AVP is represented by a list in the avps field. +success(_) -> + Avps = [{295, <<1:32>>}, %% Termination-Cause + {284, [{280, "Proxy-Host"}, %% Proxy-Info + {33, "Proxy-State"}, %% + {295, <<2:32>>}]}], %% Termination-Cause + #diameter_packet{avps = [#diameter_avp{code = 295, + value = 1, + data = <<1:32>>}, + [#diameter_avp{code = 284}, + #diameter_avp{code = 280}, + #diameter_avp{code = 33}, + #diameter_avp{code = 295, + value = 2, + data = <<2:32>>}]], + errors = []} + = str(recode(str(Avps))). + +%% =========================================================================== + +%% Ensure a Grouped AVP is represented by a list in the avps field +%% even in the case of a decode error on a component AVP. +grouped_error(_) -> + Avps = [{295, <<1:32>>}, %% Termination-Cause + {284, [{295, <<0:32>>}, %% Proxy-Info, Termination-Cause + {280, "Proxy-Host"}, + {33, "Proxy-State"}]}], + #diameter_packet{avps = [#diameter_avp{code = 295, + value = 1, + data = <<1:32>>}, + [#diameter_avp{code = 284}, + #diameter_avp{code = 295, + value = undefined, + data = <<0:32>>}, + #diameter_avp{code = 280}, + #diameter_avp{code = 33}]], + errors = [{5004, #diameter_avp{code = 284}}]} + = str(recode(str(Avps))). + +%% =========================================================================== + +%% Ensure that a failed decode in Failed-AVP is acceptable, and that +%% the component AVPs are decoded if possible. +failed_error(_) -> + Avps = [{279, [{295, <<0:32>>}, %% Failed-AVP, Termination-Cause + {258, <<1:32>>}, %% Auth-Application-Id + {284, [{280, "Proxy-Host"}, %% Proxy-Info + {33, "Proxy-State"}, + {295, <<0:32>>}, %% Termination-Cause, invalid + {258, <<2:32>>}]}]}], %% Auth-Application-Id + #diameter_packet{avps = [[#diameter_avp{code = 279}, + #diameter_avp{code = 295, + value = undefined, + data = <<0:32>>}, + #diameter_avp{code = 258, + value = 1, + data = <<1:32>>}, + [#diameter_avp{code = 284}, + #diameter_avp{code = 280}, + #diameter_avp{code = 33}, + #diameter_avp{code = 295, + value = undefined}, + #diameter_avp{code = 258, + value = 2, + data = <<2:32>>}]]], + errors = []} + = sta(recode(sta(Avps))). + +%% =========================================================================== + +%% str/1 + +str(#diameter_packet{avps = [#diameter_avp{code = 263}, + #diameter_avp{code = 264}, + #diameter_avp{code = 296}, + #diameter_avp{code = 283}, + #diameter_avp{code = 258, + value = 0} + | T]} + = Pkt) -> + Pkt#diameter_packet{avps = T}; + +str(Avps) -> + OH = "diameter.erlang.org", + OR = "erlang.org", + DR = "example.com", + Sid = "diameter.erlang.org;123;456", + + [#diameter_header{version = 1, + cmd_code = 275, %% STR + is_request = true, + application_id = 0, + hop_by_hop_id = 17, + end_to_end_id = 42, + is_proxiable = false, + is_error = false, + is_retransmitted = false} + | avp([{263, Sid}, %% Session-Id + {264, OH}, %% Origin-Host + {296, OR}, %% Origin-Realm + {283, DR}, %% Destination-Realm + {258, <<0:32>>}] %% Auth-Application-Id + ++ Avps)]. + +%% sta/1 + +sta(#diameter_packet{avps = [#diameter_avp{code = 263}, + #diameter_avp{code = 268}, + #diameter_avp{code = 264}, + #diameter_avp{code = 296}, + #diameter_avp{code = 278, + value = 4} + | T]} + = Pkt) -> + Pkt#diameter_packet{avps = T}; + +sta(Avps) -> + OH = "diameter.erlang.org", + OR = "erlang.org", + Sid = "diameter.erlang.org;123;456", + + [#diameter_header{version = 1, + cmd_code = 275, %% STA + is_request = false, + application_id = 0, + hop_by_hop_id = 17, + end_to_end_id = 42, + is_proxiable = false, + is_error = false, + is_retransmitted = false} + | avp([{263, Sid}, %% Session-Id + {268, <<2002:32>>}, %% Result-Code + {264, OH}, %% Origin-Host + {296, OR}, %% Origin-Realm + {278, <<4:32>>}] %% Origin-State-Id + ++ Avps)]. + +avp({Code, Data}) -> + #diameter_avp{code = Code, + data = avp(Data)}; + +avp(#diameter_avp{} = A) -> + A; + +avp([{_,_} | _] = Avps) -> + lists:map(fun avp/1, Avps); + +avp(V) -> + V. + +%% recode/1 + +recode(Msg) -> + recode(Msg, diameter_gen_base_rfc6733). + +recode(#diameter_packet{} = Pkt, Dict) -> + diameter_codec:decode(Dict, diameter_codec:encode(Dict, Pkt)); + +recode(Msg, Dict) -> + recode(#diameter_packet{msg = Msg}, Dict). -- cgit v1.2.3 From c74b593a185bde920497ef6d25c92c896949d07c Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Wed, 18 Feb 2015 11:11:24 +0100 Subject: Allow encode of decoded diameter_avp list The decode of an incoming request in a non-relay application results in a deep list of diameter_avp records. Encoding such a list resulted in a function_clause error in diameter_codec:pack_avp/1, which expected a flat list. The list is only flat in the relay case, or in the absence of AVPs of type Grouped. This is also related to code that exists but isn't documented. It's documented that a diameter_app(3) handle_request callback can return {relay, Opts} to relay a request received in the relay application. What's not documented is that it can also return {proxy|resend, Opts} in a non-relay application, but this leads to encode failure when there are Grouped AVPs. This shouldn't be interpreted as meaning that proxy|resend are now supported: they aren't. The two extra terms are a historical relic that should probably be removed. Neither are generally usable since, for example, a proxy agent may want to modify a request before resending it. A specific handle_request return is not needed to implement a proxy agent. Even {relay, Opts} isn't strictly necessary. --- lib/diameter/src/base/diameter_codec.erl | 2 ++ 1 file changed, 2 insertions(+) (limited to 'lib/diameter') diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index dd9039d7dc..9bc8178230 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -582,6 +582,8 @@ split_data(Bin, Len) -> %% dictionary doesn't know about specific AVP's. %% Grouped AVP whose components need packing ... +pack_avp([#diameter_avp{} = A | Avps]) -> + pack_avp(A#diameter_avp{data = Avps}); pack_avp(#diameter_avp{data = [#diameter_avp{} | _] = Avps} = A) -> pack_avp(A#diameter_avp{data = encode_avps(Avps)}); -- cgit v1.2.3