Fix recognition of 5001 on mandatory AVP's

An AVP setting the M-bit was not regarded as erroneous if it was defined
in the dictionary in question and its container (message or Grouped AVP)
had an 'AVP' field. It's now regarded as a 5001 error (AVP_UNSUPPORTED),
as in the case that the AVP is not defined.

2 files changed, 42 insertions, 21 deletions

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index 2126a5623d..55aae3a243 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -221,7 +221,9 @@ decode(Name, 'AVP', Avp, Acc) ->
 %% d/3
 
 %% Don't try to decode the value of a Failed-AVP component since it
-%% probably won't.
+%% 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);
 
@@ -253,17 +255,10 @@ d(Name, Avp, {Avps, Acc}) ->
 %% decode_AVP/3
 %%
 %% Don't know this AVP: see if it can be packed in an 'AVP' field
-%% undecoded, unless it's mandatory. Need to give Failed-AVP special
-%% treatment since it'll contain any unrecognized mandatory AVP's.
-%% Note that the type field is 'undefined' in this case.
-
-decode_AVP(Name, #diameter_avp{is_mandatory = M} = Avp, {Avps, Acc}) ->
-    {[Avp | Avps], if Name == 'Failed-AVP';
-                      not M ->
-                           pack_AVP(Name, Avp, Acc);
-                      true ->
-                           unknown(Avp, Acc)
-                   end}.
+%% undecoded. Note that the type field is 'undefined' in this case.
+
+decode_AVP(Name, Avp, {Avps, Acc}) ->
+    {[Avp | Avps], pack_AVP(Name, Avp, Acc)}.
 
 %% rc/1
 
@@ -315,10 +310,18 @@ pack_avp(_, Arity, Avp, Acc) ->
 
 %% pack_AVP/3
 
-pack_AVP(Name, Avp, Acc) ->
+%% Give Failed-AVP special treatment since it'll contain any
+%% unrecognized mandatory AVP's.
+pack_AVP(Name, #diameter_avp{is_mandatory = true} = Avp, Acc)
+  when Name /= 'Failed-AVP' ->
+    {Rec, Failed} = Acc,
+    {Rec, [{5001, Avp} | Failed]};
+
+pack_AVP(Name, #diameter_avp{is_mandatory = M} = Avp, Acc) ->
     case avp_arity(Name, 'AVP') of
         0 ->
-            unknown(Avp, Acc);
+            {Rec, Failed} = Acc,
+            {Rec, [{if M -> 5001; true -> 5008 end, Avp} | Failed]};
         Arity ->
             pack(Arity, 'AVP', Avp, Acc)
     end.
@@ -335,9 +338,6 @@ pack_AVP(Name, Avp, Acc) ->
 %%      A message was received with an AVP that MUST NOT be present.  The
 %%      Failed-AVP AVP MUST be included and contain a copy of the
 %%      offending AVP.
-%%
-unknown(#diameter_avp{is_mandatory = B} = Avp, {Rec, Failed}) ->
-    {Rec, [{if B -> 5001; true -> 5008 end, Avp} | Failed]}.
 
 %% pack/4
 
diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl
index 1d5627af5e..42cb4b6844 100644
--- a/lib/diameter/test/diameter_traffic_SUITE.erl
+++ b/lib/diameter/test/diameter_traffic_SUITE.erl
@@ -54,6 +54,7 @@
          send_zero_avp_length/1,
          send_invalid_avp_length/1,
          send_invalid_reject/1,
+         send_unrecognized_mandatory/1,
          send_long/1,
          send_nopeer/1,
          send_noapp/1,
@@ -275,6 +276,7 @@ tc() ->
      send_zero_avp_length,
      send_invalid_avp_length,
      send_invalid_reject,
+     send_unrecognized_mandatory,
      send_long,
      send_nopeer,
      send_noapp,
@@ -420,13 +422,14 @@ send_protocol_error(Config) ->
     ?answer_message(?TOO_BUSY)
         = call(Config, Req).
 
-%% Send an ASR with an arbitrary AVP and expect success and the same
-%% AVP in the reply.
+%% Send an ASR with an arbitrary non-mandatory AVP and expect success
+%% and the same AVP in the reply.
 send_arbitrary(Config) ->
-    Req = ['ASR', {'AVP', [#diameter_avp{name = 'Class', value = "XXX"}]}],
+    Req = ['ASR', {'AVP', [#diameter_avp{name = 'Product-Name',
+                                         value = "XXX"}]}],
     ['ASA', _SessionId, {'Result-Code', ?SUCCESS} | Avps]
         = call(Config, Req),
-    {'AVP', [#diameter_avp{name = 'Class',
+    {'AVP', [#diameter_avp{name = 'Product-Name',
                            value = "XXX"}]}
         = lists:last(Avps).
 
@@ -522,6 +525,14 @@ 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) ->
+    Req = ['STR', {'Termination-Cause', ?LOGOUT}],
+
+    ['STA', _SessionId, {'Result-Code', ?AVP_UNSUPPORTED} | _]
+        = call(Config, Req).
+
 %% Send something long that will be fragmented by TCP.
 send_long(Config) ->
     Req = ['STR', {'Termination-Cause', ?LOGOUT},
@@ -868,6 +879,16 @@ prepare(Pkt, Caps, N, #group{client_dict0 = Dict0} = Group)
                               0:32/integer,
                               T/binary>>};
 
+prepare(Pkt, Caps, send_unrecognized_mandatory, #group{client_dict0 = Dict0}
+                                                = Group) ->
+    Req = prepare(Pkt, Caps, Group),
+    #diameter_packet{bin = <<V, Len:24, T/binary>>}
+        = E
+        = diameter_codec:encode(Dict0, Pkt#diameter_packet{msg = Req}),
+    {Code, Flags, undefined} = Dict0:avp_header('Proxy-State'),
+    Avp = <<Code:32, Flags, 8:24>>,
+    E#diameter_packet{bin = <<V, (Len+8):24, T/binary, Avp/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>>}