aboutsummaryrefslogtreecommitdiffstats
path: root/lib
diff options
context:
space:
mode:
authorAnders Svensson <[email protected]>2015-06-22 00:39:48 +0200
committerAnders Svensson <[email protected]>2015-06-22 00:39:48 +0200
commit5fff4543e890229527c2b8875fbc96aa88c87ad9 (patch)
treed884f1f39ffd44ac3aade7888a6d40db693d9341 /lib
parent2bf100436c480c030200a655c82ec12fe55cdca4 (diff)
parent7f4f9583bb1245c27ca58d88fe6862498a2df1f2 (diff)
downloadotp-5fff4543e890229527c2b8875fbc96aa88c87ad9.tar.gz
otp-5fff4543e890229527c2b8875fbc96aa88c87ad9.tar.bz2
otp-5fff4543e890229527c2b8875fbc96aa88c87ad9.zip
Merge branch 'anders/diameter/grouped_errors/OTP-12721'
* anders/diameter/grouped_errors/OTP-12721: Fix decode of Grouped AVPs containing errors Simplify logic Simplify logic
Diffstat (limited to 'lib')
-rw-r--r--lib/diameter/include/diameter_gen.hrl92
-rw-r--r--lib/diameter/src/base/diameter_codec.erl3
-rw-r--r--lib/diameter/test/diameter_traffic_SUITE.erl57
3 files changed, 125 insertions, 27 deletions
diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index e8ffe7f92c..fb321054bd 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -333,16 +333,10 @@ d(Name, Avp, Acc) ->
{H, A} = ungroup(V, Avp),
{[H | Avps], pack_avp(Name, A, T)}
catch
- throw: {?TAG, {grouped, RC, ComponentAvps}} ->
- {Avps, {Rec, Errors}} = Acc,
- A = trim(Avp),
- {[[A | trim(ComponentAvps)] | Avps], {Rec, [{RC, A} | Errors]}};
+ throw: {?TAG, {grouped, Error, ComponentAvps}} ->
+ g(is_failed(), Error, Name, trim(Avp), Acc, ComponentAvps);
error: Reason ->
- d(undefined == Failed orelse is_failed(),
- Reason,
- Name,
- trim(Avp),
- Acc)
+ d(is_failed(), Reason, Name, trim(Avp), Acc)
after
reset(?STRICT_KEY, Strict),
reset(?FAILED_KEY, Failed)
@@ -380,6 +374,27 @@ dict(true) ->
dict(_) ->
?MODULE.
+%% g/5
+
+%% Ignore decode errors within Failed-AVP (best-effort) ...
+g(true, [_Error | Rec], Name, Avp, Acc, _ComponentAvps) ->
+ decode_AVP(Name, Avp#diameter_avp{value = Rec}, Acc);
+g(true, _Error, Name, Avp, Acc, _ComponentAvps) ->
+ decode_AVP(Name, Avp, Acc);
+
+%% ... or not.
+g(false, [Error | _Rec], _Name, Avp, Acc, ComponentAvps) ->
+ g(Error, Avp, Acc, ComponentAvps);
+g(false, Error, _Name, Avp, Acc, ComponentAvps) ->
+ g(Error, Avp, Acc, ComponentAvps).
+
+%% g/4
+
+g({RC, ErrorData}, Avp, Acc, ComponentAvps) ->
+ {Avps, {Rec, Errors}} = Acc,
+ E = Avp#diameter_avp{data = [ErrorData]},
+ {[[Avp | trim(ComponentAvps)] | Avps], {Rec, [{RC, E} | Errors]}}.
+
%% d/5
%% Ignore a decode error within Failed-AVP ...
@@ -424,14 +439,26 @@ is_strict() ->
%% Strictly, this doesn't need to be the case.
relax('Failed-AVP') ->
- is_failed() orelse putr(?FAILED_KEY, true);
+ putr(?FAILED_KEY, true);
relax(_) ->
is_failed().
-
+
+%% is_failed/0
+%%
+%% Is the AVP currently being decoded nested within Failed-AVP? Note
+%% that this is only true when Failed-AVP is the parent. In
+%% particular, it's not true when Failed-AVP itself is being decoded
+%% (unless nested).
+
is_failed() ->
true == getr(?FAILED_KEY).
+%% is_failed/1
+
+is_failed(Name) ->
+ 'Failed-AVP' == Name orelse is_failed().
+
%% reset/2
reset(Key, undefined) ->
@@ -451,8 +478,8 @@ decode_AVP(Name, Avp, {Avps, Acc}) ->
%% diameter_types will raise an error of this form to communicate
%% DIAMETER_INVALID_AVP_LENGTH (5014). A module specified to a
-%% @custom_types tag in a spec file can also raise an error of this
-%% form.
+%% @custom_types tag in a dictionary file can also raise an error of
+%% this form.
rc({'DIAMETER', 5014 = RC, _}, #diameter_avp{name = AvpName} = Avp) ->
{RC, Avp#diameter_avp{data = empty_value(AvpName)}};
@@ -528,17 +555,16 @@ pack_AVP(Name, #diameter_avp{is_mandatory = M, name = AvpName} = Avp, Acc) ->
%% allow for Failed-AVP in an answer-message.
pack_arity(Name, AvpName, M) ->
- IsFailed = Name == 'Failed-AVP' orelse 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.
+ %% packed into a dedicated field defeats that point. Note
+ %% is_failed/1 since is_failed/0 will return false when packing
+ %% 'AVP' within Failed-AVP.
- pack_arity(IsFailed
+ pack_arity(is_failed(Name)
orelse {Name, AvpName} == {'answer-message', 'Failed-AVP'}
orelse not M
orelse not is_strict(),
@@ -610,9 +636,12 @@ value(_, Avp) ->
-> binary()
| no_return().
-%% Length error induced by diameter_codec:collect_avps/1.
+%% Length error induced by diameter_codec:collect_avps/1: the AVP
+%% length in the header was too short (insufficient for the extracted
+%% header) or too long (past the end of the message). An empty payload
+%% is sufficient according to the RFC text for 5014.
grouped_avp(decode, _Name, <<0:1, _/binary>>) ->
- throw({?TAG, {grouped, 5014, []}});
+ throw({?TAG, {grouped, {5014, []}, []}});
grouped_avp(decode, Name, Data) ->
grouped_decode(Name, diameter_codec:collect_avps(Data));
@@ -626,13 +655,28 @@ grouped_avp(encode, Name, Data) ->
%% decoded value, also returning the list of component diameter_avp
%% records.
+%% Length error in trailing component AVP.
grouped_decode(_Name, {Error, Acc}) ->
- {RC, Avp} = Error,
- throw({?TAG, {grouped, RC, [Avp | Acc]}});
-
+ {5014, Avp} = Error,
+ throw({?TAG, {grouped, Error, [Avp | Acc]}});
+
+%% 7.5. Failed-AVP AVP
+
+%% In the case where the offending AVP is embedded within a Grouped AVP,
+%% the Failed-AVP MAY contain the grouped AVP, which in turn contains
+%% the single offending AVP. The same method MAY be employed if the
+%% grouped AVP itself is embedded in yet another grouped AVP and so on.
+%% In this case, the Failed-AVP MAY contain the grouped AVP hierarchy up
+%% to the single offending AVP. This enables the recipient to detect
+%% the location of the offending AVP when embedded in a group.
+
+%% An error in decoding a component AVP throws the first fauly
+%% component, which the catch in d/3 wraps in the Grouped AVP in
+%% question. A partially decoded record is only used when ignoring
+%% errors in Failed-AVP.
grouped_decode(Name, ComponentAvps) ->
{Rec, Avps, Es} = decode_avps(Name, ComponentAvps),
- [] == Es orelse throw({?TAG, {grouped, 5004, Avps}}), %% decode failure
+ [] == Es orelse throw({?TAG, {grouped, [{_,_} = hd(Es) | Rec], Avps}}),
{Rec, Avps}.
%% ---------------------------------------------------------------------------
diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl
index bf2fe8e7ca..810be03f5e 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -590,6 +590,7 @@ split_head(<<Code:32, 0:1, M:1, P:1, _:5, Len:24, _/binary>>) ->
%% Header is truncated.
split_head(Bin) ->
?THROW({5014, #diameter_avp{data = Bin}}).
+%% Note that pack_avp/1 will pad this at encode if sent in a Failed-AVP.
%% 3588:
%%
@@ -619,7 +620,7 @@ split_head(Bin) ->
%% AVP header with zero up to the minimum AVP header length.
%%
%% The underlined clause must be in error since (1) a header less than
-%% the minimum value mean we don't know the identity of the AVP and
+%% the minimum value mean we might not know the identity of the AVP and
%% (2) the last sentence covers this case.
%% split_data/3
diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl
index 17faf30a9b..e26a8946b7 100644
--- a/lib/diameter/test/diameter_traffic_SUITE.erl
+++ b/lib/diameter/test/diameter_traffic_SUITE.erl
@@ -48,6 +48,7 @@
send_unknown_mandatory/1,
send_unknown_short_mandatory/1,
send_noreply/1,
+ send_grouped_error/1,
send_unsupported/1,
send_unsupported_app/1,
send_error_bit/1,
@@ -329,6 +330,7 @@ tc() ->
send_unknown_mandatory,
send_unknown_short_mandatory,
send_noreply,
+ send_grouped_error,
send_unsupported,
send_unsupported_app,
send_error_bit,
@@ -573,7 +575,7 @@ 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.
+%% Send an ASR 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
@@ -589,6 +591,25 @@ send_unexpected_mandatory_decode(Config) ->
data = <<12:32>>}]
= As.
+%% Send an containing a faulty Grouped AVP (empty Proxy-Host in
+%% Proxy-Info) and expect that only the faulty AVP is sent in
+%% Failed-AVP. The encoded values of Proxy-Host and Proxy-State are
+%% swapped in prepare_request since an empty Proxy-Host is an encode
+%% error.
+send_grouped_error(Config) ->
+ Req = ['ASR', {'Proxy-Info', [[{'Proxy-Host', "abcd"},
+ {'Proxy-State', ""}]]}],
+ ['ASA', {'Session-Id', _}, {'Result-Code', ?INVALID_AVP_LENGTH} | Avps]
+ = call(Config, Req),
+ [#'diameter_base_Failed-AVP'{'AVP' = As}]
+ = proplists:get_value('Failed-AVP', Avps),
+ [#diameter_avp{name = 'Proxy-Info',
+ value = #'diameter_base_Proxy-Info'
+ {'Proxy-Host' = Empty,
+ 'Proxy-State' = undefined}}]
+ = As,
+ <<0>> = iolist_to_binary(Empty).
+
%% Send an STR that the server ignores.
send_noreply(Config) ->
Req = ['STR', {'Termination-Cause', ?BAD_ANSWER}],
@@ -1069,6 +1090,38 @@ prepare(Pkt, Caps, send_unexpected_mandatory, #group{client_dict0 = Dict0}
Avp = <<Code:32, Flags, 8:24>>,
E#diameter_packet{bin = <<V, (Len+8):24, T/binary, Avp/binary>>};
+prepare(Pkt, Caps, send_grouped_error, #group{client_dict0 = Dict0}
+ = Group) ->
+ Req = prepare(Pkt, Caps, Group),
+ #diameter_packet{bin = Bin}
+ = E
+ = diameter_codec:encode(Dict0, Pkt#diameter_packet{msg = Req}),
+ {Code, Flags, undefined} = Dict0:avp_header('Proxy-Info'),
+ %% Find Proxy-Info by looking for its header.
+ Pattern = <<Code:32, Flags, 28:24>>,
+ {Offset, 8} = binary:match(Bin, Pattern),
+
+ %% Extract and swap Proxy-Host/State payloads.
+
+ <<H:Offset/binary,
+ PI:8/binary,
+ PH:5/binary,
+ 12:24,
+ Payload:4/binary,
+ PS:5/binary,
+ 8:24,
+ T/binary>>
+ = Bin,
+
+ E#diameter_packet{bin = <<H/binary,
+ PI/binary,
+ PH/binary,
+ 8:24,
+ PS:5/binary,
+ 12:24,
+ Payload/binary,
+ T/binary>>};
+
prepare(Pkt, Caps, send_unsupported, #group{client_dict0 = Dict0} = Group) ->
Req = prepare(Pkt, Caps, Group),
#diameter_packet{bin = <<H:5/binary, _CmdCode:3/binary, T/binary>>}
@@ -1175,7 +1228,7 @@ 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.
+%% Missing Result-Code and inappropriate Experimental-Result-Code.
answer(Rec, Es, send_experimental_result) ->
[{5004, #diameter_avp{name = 'Experimental-Result'}},
{5005, #diameter_avp{name = 'Result-Code'}}]