Merge branch 'anders/diameter/grouped_errors/OTP-12930' into maint-17

* anders/diameter/grouped_errors/OTP-12930:
  Fix decode of Grouped AVPs containing errors
  Simplify logic
  Simplify logic

1 files changed, 68 insertions, 24 deletions

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index ebe9e6363d..ac2126cdc5 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -350,16 +350,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)
@@ -397,6 +391,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 ...
@@ -441,14 +456,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) ->
@@ -468,8 +495,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)}};
 
@@ -545,17 +572,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(),
@@ -630,9 +656,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));
@@ -646,13 +675,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}.
 
 %% ---------------------------------------------------------------------------