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

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'}}]