Do best-effort decode of Failed-AVP

Commit 4ce2d3a6 (diameter-1.4.2, OTP-11007) disabled the decode of
values in Failed-AVP components since any error caused the decode of
Failed-AVP itself to fail. This is less than useful since (1) we should
be able to decode it given that we've sent it (modulo mangling on the
way to the peer and back), and (2) it's not unheard of to examine
Failed-AVP to see what the peer objected to.

This commits adds a best-effort decode: decode if possible, otherwise
not, using the same abuse of the process dictionary as commit bbdb027c.

2 files changed, 69 insertions, 36 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});