Merge branch 'anders/diameter/Failed-AVP/OTP-11936' into maint

* anders/diameter/Failed-AVP/OTP-11936:
  Do best-effort decode of Failed-AVP
  Add a testcase that expects a decoded value in Failed-AVP

3 files changed, 96 insertions, 44 deletions

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index ebc10b8918..7e91ce375f 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -30,6 +30,10 @@
 %% error or not. See is_strict/0.
 -define(STRICT_KEY, strict).
 
+%% Key that says whether or not we should do a best-effort decode
+%% within Failed-AVP.
+-define(FAILED_KEY, failed).
+
 -type parent_name()   :: atom().  %% parent = Message or AVP
 -type parent_record() :: tuple(). %%
 -type avp_name()   :: atom().
@@ -286,15 +290,7 @@ decode(Name, 'AVP', Avp, Acc) ->
 
 %% d/3
 
-%% Don't try to decode the value of a Failed-AVP component since it
-%% probably won't. Note that matching on 'Failed-AVP' assumes that
-%% this is the RFC AVP, with code 279. Strictly, this doesn't need to
-%% be the case, so we're assuming no one defines another Failed-AVP.
-d('Failed-AVP' = Name, Avp, Acc) ->
-    decode_AVP(Name, Avp, Acc);
-
-%% Or try to decode.
-d(Name, Avp, {Avps, Acc}) ->
+d(Name, Avp, Acc) ->
     #diameter_avp{name = AvpName,
                   data = Data,
                   type = Type,
@@ -307,53 +303,81 @@ d(Name, Avp, {Avps, Acc}) ->
     %% value around through the entire decode. The solution here is
     %% simple in comparison, both to implement and to understand.
 
-    Reset = relax(Type, M),
+    Strict = relax(Type, M),
 
+    %% Use the process dictionary again to keep track of whether we're
+    %% decoding within Failed-AVP and should ignore decode errors
+    %% altogether.
+
+    Failed = relax(Name),  %% Not AvpName or else a failed Failed-AVP
+                           %% decode is packed into 'AVP'.
     try avp(decode, Data, AvpName) of
         V ->
+            {Avps, T} = Acc,
             {H, A} = ungroup(V, Avp),
-            {[H | Avps], pack_avp(Name, A, Acc)}
+            {[H | Avps], pack_avp(Name, A, T)}
     catch
         error: Reason ->
-            %% Failures here won't be visible since they're a "normal"
-            %% occurrence if the peer sends a faulty AVP that we need to
-            %% respond sensibly to. Log the occurence for traceability,
-            %% but the peer will also receive info in the resulting
-            %% answer-message.
-            Stack = diameter_lib:get_stacktrace(),
-            diameter_lib:log(decode_error,
-                             ?MODULE,
-                             ?LINE,
-                             {Reason, AvpName, Stack}),
-
-            {Rec, Failed} = Acc,
-            {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}}
+            d(undefined == Failed orelse is_failed(), Reason, Name, Avp, Acc)
     after
-        relax(Reset)
+        reset(?STRICT_KEY, Strict),
+        reset(?FAILED_KEY, Failed)
     end.
 
+%% Ignore a decode error within Failed-AVP ...
+d(true, _, Name, Avp, Acc) ->
+    decode_AVP(Name, Avp, Acc);
+
+%% ... or not. Failures here won't be visible since they're a "normal"
+%% occurrence if the peer sends a faulty AVP that we need to respond
+%% sensibly to. Log the occurence for traceability, but the peer will
+%% also receive info in the resulting answer message.
+d(false, Reason, Name, Avp, {Avps, Acc}) ->
+    Stack = diameter_lib:get_stacktrace(),
+    diameter_lib:log(decode_error,
+                     ?MODULE,
+                     ?LINE,
+                     {Reason, Name, Avp#diameter_avp.name, Stack}),
+    {Rec, Failed} = Acc,
+    {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}}.
+
 %% Set false in the process dictionary as soon as we see a Grouped AVP
 %% that doesn't set the M-bit, so that is_strict() can say whether or
 %% not to ignore the M-bit on an encapsulated AVP.
 relax('Grouped', M) ->
-    V = getr(?STRICT_KEY),
-    if V == undefined andalso not M ->
+    case getr(?STRICT_KEY) of
+        undefined when not M ->
             putr(?STRICT_KEY, M);
-       true ->
+        _ ->
             false
     end;
 relax(_, _) ->
     false.
 
-%% Reset strictness.
-relax(undefined) ->
-    eraser(?STRICT_KEY);
-relax(false) ->
-    ok.
-
 is_strict() ->
     false /= getr(?STRICT_KEY).
 
+%% Set true in the process dictionary as soon as we see Failed-AVP.
+%% Matching on 'Failed-AVP' assumes that this is the RFC AVP.
+%% Strictly, this doesn't need to be the case.
+relax('Failed-AVP') ->
+    case getr(?FAILED_KEY) of
+        undefined ->
+            putr(?FAILED_KEY, true);
+        true = Yes ->
+            Yes
+    end;
+relax(_) ->
+    is_failed().
+    
+is_failed() ->
+    true == getr(?FAILED_KEY).
+
+reset(Key, undefined) ->
+    eraser(Key);
+reset(_, _) ->
+    ok.
+
 %% decode_AVP/3
 %%
 %% Don't know this AVP: see if it can be packed in an 'AVP' field
@@ -441,7 +465,15 @@ pack_AVP(Name, #diameter_avp{is_mandatory = M} = Avp, Acc) ->
 %% Give Failed-AVP special treatment since it'll contain any
 %% unrecognized mandatory AVP's.
 pack_arity(Name, M) ->
-    case Name /= 'Failed-AVP' andalso M andalso is_strict() of
+    NF = Name /= 'Failed-AVP' andalso not 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.
+    case NF andalso M andalso is_strict() of
         true ->
             0;
         false ->
diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl
index 0ca4a84d21..06a4f5de64 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -593,7 +593,8 @@ pack_avp(#diameter_avp{code = undefined, data = B})
     Len = size(<<H:5/binary, _:24, T/binary>> = <<B/binary, 0:Pad>>),
     <<H/binary, Len:24, T/binary>>;
 
-%% ... from a dictionary compiled against old code (diameter_gen) ...
+%% ... from a dictionary compiled against old code in diameter_gen ...
+%% ... when ignoring errors in Failed-AVP ...
 pack_avp(#diameter_avp{data = <<0:1, B/binary>>} = A) ->
     pack_avp(A#diameter_avp{data = B});
 
diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl
index 89592f02ef..4b67372016 100644
--- a/lib/diameter/test/diameter_traffic_SUITE.erl
+++ b/lib/diameter/test/diameter_traffic_SUITE.erl
@@ -56,7 +56,8 @@
          send_zero_avp_length/1,
          send_invalid_avp_length/1,
          send_invalid_reject/1,
-         send_unrecognized_mandatory/1,
+         send_unexpected_mandatory_decode/1,
+         send_unexpected_mandatory/1,
          send_long/1,
          send_nopeer/1,
          send_noapp/1,
@@ -281,7 +282,8 @@ tc() ->
      send_zero_avp_length,
      send_invalid_avp_length,
      send_invalid_reject,
-     send_unrecognized_mandatory,
+     send_unexpected_mandatory_decode,
+     send_unexpected_mandatory,
      send_long,
      send_nopeer,
      send_noapp,
@@ -488,6 +490,22 @@ 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.
+%% 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
+                                         is_mandatory = true,
+                                         data = <<12:32>>}]}],
+    ['ASA', _SessionId, {'Result-Code', ?AVP_UNSUPPORTED} | Avps]
+        = call(Config, Req),
+    [#'diameter_base_Failed-AVP'{'AVP' = As}]
+        = proplists:get_value('Failed-AVP', Avps),
+    [#diameter_avp{code = 27,
+                   is_mandatory = true,
+                   value = 12,
+                   data = <<12:32>>}]
+        = As.
+
 %% Send an STR that the server ignores.
 send_noreply(Config) ->
     Req = ['STR', {'Termination-Cause', ?BAD_ANSWER}],
@@ -554,9 +572,9 @@ send_invalid_reject(Config) ->
     ?answer_message(?TOO_BUSY)
         = call(Config, Req).
 
-%% Send an STR containing a known AVP, but one that's not allowed and
-%% sets the M-bit.
-send_unrecognized_mandatory(Config) ->
+%% Send an STR containing a known AVP, but one that's not expected and
+%% that sets the M-bit.
+send_unexpected_mandatory(Config) ->
     Req = ['STR', {'Termination-Cause', ?LOGOUT}],
 
     ['STA', _SessionId, {'Result-Code', ?AVP_UNSUPPORTED} | _]
@@ -923,8 +941,8 @@ prepare(Pkt, Caps, N, #group{client_dict0 = Dict0} = Group)
     <<V, L:24, H/binary>> = H0,  %% assert
     E#diameter_packet{bin = <<V, (L+4):24, H/binary, 16:24, 0:32, T/binary>>};
 
-prepare(Pkt, Caps, send_unrecognized_mandatory, #group{client_dict0 = Dict0}
-                                                = Group) ->
+prepare(Pkt, Caps, send_unexpected_mandatory, #group{client_dict0 = Dict0}
+                                              = Group) ->
     Req = prepare(Pkt, Caps, Group),
     #diameter_packet{bin = <<V, Len:24, T/binary>>}
         = E
@@ -1045,7 +1063,8 @@ answer(Rec, [_|_], N)
        N == send_zero_avp_length;
        N == send_invalid_avp_length;
        N == send_invalid_reject;
-       N == send_unknown_short_mandatory ->
+       N == send_unknown_short_mandatory;
+       N == send_unexpected_mandatory_decode ->
     Rec;
 answer(Rec, [], _) ->
     Rec.