Merge branch 'anders/diameter/avp_decode/OTP-11007' into maint

* anders/diameter/avp_decode/OTP-11007:
  Detect all 5005 (MISSING_AVP) errors and don't reverse errors
  Adapt Failed-AVP setting to RFC 6733
  Add spec to diameter_codec
  Add spec to diameter_gen
  Fix recognition of 5014 (INVALID_AVP_LENGTH) errors
  Ensure setting Failed-AVP is appropriate
  Correct AVP Length error testcases

6 files changed, 360 insertions, 242 deletions

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index 03aa557c2e..2126a5623d 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -25,11 +25,23 @@
 
 -define(THROW(T), throw({?MODULE, T})).
 
-%%% ---------------------------------------------------------------------------
-%%% # encode_avps/3
-%%%
-%%% Returns: binary()
-%%% ---------------------------------------------------------------------------
+-type parent_name()   :: atom().  %% parent = Message or AVP
+-type parent_record() :: tuple(). %%
+-type avp_name()   :: atom().
+-type avp_record() :: tuple().
+-type avp_values() :: [{avp_name(), term()}].
+
+-type non_grouped_avp() :: #diameter_avp{}.
+-type grouped_avp() :: nonempty_improper_list(#diameter_avp{}, [avp()]).
+-type avp() :: non_grouped_avp() | grouped_avp().
+
+%% ---------------------------------------------------------------------------
+%% # encode_avps/2
+%% ---------------------------------------------------------------------------
+
+-spec encode_avps(parent_name(), parent_record() | avp_values())
+   -> binary()
+    | no_return().
 
 encode_avps(Name, Vals)
   when is_list(Vals) ->
@@ -129,42 +141,26 @@ pack_AVP(Name, #diameter_avp{name = AvpName,
         orelse ?THROW([known_avp_as_AVP, Name, AvpName, Data]),
     e(AvpName, [Data]).
 
-%%% ---------------------------------------------------------------------------
-%%% # decode_avps/2
-%%%
-%%% Returns: {Rec, Avps, Failed}
-%%%
-%%%          Rec  = decoded message record
-%%%          Avps = list of Avp
-%%%          Failed = list of {ResultCode, #diameter_avp{}}
-%%%
-%%%          Avp = #diameter_avp{}    if type is not Grouped
-%%%              | list of Avp        where first element has type Grouped
-%%%                                   and following elements are its component
-%%%                                   AVP's.
-%%% ---------------------------------------------------------------------------
+%% ---------------------------------------------------------------------------
+%% # decode_avps/2
+%% ---------------------------------------------------------------------------
+
+-spec decode_avps(parent_name(), [#diameter_avp{}])
+   -> {parent_record(), [avp()], Failed}
+ when Failed :: [{5000..5999, #diameter_avp{}}].
 
 decode_avps(Name, Recs) ->
-    d_rc(Name, lists:foldl(fun(T,A) -> decode(Name, T, A) end,
-                           {[], {newrec(Name), []}},
-                           Recs)).
+    {Avps, {Rec, Failed}}
+        = lists:foldl(fun(T,A) -> decode(Name, T, A) end,
+                      {[], {newrec(Name), []}},
+                      Recs),
+    {Rec, Avps, Failed ++ missing(Rec, Name)}.
+%% Append 5005 errors so that a 5014 for the same AVP will take
+%% precedence in a Result-Code/Failed-AVP setting.
 
 newrec(Name) ->
     '#new-'(name2rec(Name)).
 
-%% No errors so far: keep looking.
-d_rc(Name, {Avps, {Rec, [] = Failed}}) ->
-    try
-        true = have_required_avps(Rec, Name),
-        {Rec, Avps, Failed}
-    catch
-        throw: {?MODULE, {AvpName, Reason}} ->
-            diameter_lib:log({decode, error},
-                             ?MODULE,
-                             ?LINE,
-                             {AvpName, Reason, Rec}),
-            {Rec, Avps, [{5005, empty_avp(AvpName)}]}
-    end;
 %% 3588:
 %%
 %%   DIAMETER_MISSING_AVP               5005
@@ -175,9 +171,17 @@ d_rc(Name, {Avps, {Rec, [] = Failed}}) ->
 %%      Vendor-Id if applicable.  The value field of the missing AVP
 %%      should be of correct minimum length and contain zeroes.
 
-%% Or not. Only need to report the first error so look no further.
-d_rc(_, {Avps, {Rec, Failed}}) ->
-    {Rec, Avps, lists:reverse(Failed)}.
+missing(Rec, Name) ->
+    [{5005, empty_avp(F)} || F <- '#info-'(element(1, Rec), fields),
+                             A <- [avp_arity(Name, F)],
+                             false <- [have_arity(A, '#get-'(F, Rec))]].
+
+%% Maximum arities have already been checked in building the record.
+
+have_arity({Min, _}, L) ->
+    Min =< length(L);
+have_arity(N, V) ->
+    N /= 1 orelse V /= undefined.
 
 %% empty_avp/1
 
@@ -192,25 +196,6 @@ empty_avp(Name) ->
                   data = empty_value(Name),
                   type = Type}.
 
-%% have_required_avps/2
-
-have_required_avps(Rec, Name) ->
-    lists:foreach(fun(F) -> hra(Name, F, Rec) end,
-                  '#info-'(element(1, Rec), fields)),
-    true.
-
-hra(Name, AvpName, Rec) ->
-    Arity = avp_arity(Name, AvpName),
-    hra(Arity, '#get-'(AvpName, Rec))
-        orelse ?THROW({AvpName, {insufficient_arity, Arity}}).
-
-%% Maximum arities have already been checked in building the record.
-
-hra({Min, _}, L) ->
-    Min =< length(L);
-hra(N, V) ->
-    N /= 1 orelse V /= undefined.
-
 %% 3588, ch 7:
 %%
 %%   The Result-Code AVP describes the error that the Diameter node
@@ -227,23 +212,20 @@ decode(Name, #diameter_avp{code = Code, vendor_id = Vid} = Avp, Acc) ->
 
 %% decode/4
 
-%% 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.
-decode(Name, 'AVP', #diameter_avp{is_mandatory = M} = Avp, {Avps, Acc}) ->
-    {[Avp | Avps], if M, Name /= 'Failed-AVP' ->
-                           unknown(Avp, Acc);
-                      true ->
-                           pack_AVP(Name, Avp, Acc)
-                   end};
-%% Note that the type field is 'undefined' in this case.
-
-%% Or try to decode.
 decode(Name, {AvpName, Type}, Avp, Acc) ->
-    d(Name, Avp#diameter_avp{name = AvpName, type = Type}, Acc).
+    d(Name, Avp#diameter_avp{name = AvpName, type = Type}, Acc);
+
+decode(Name, 'AVP', Avp, Acc) ->
+    decode_AVP(Name, Avp, Acc).
 
 %% d/3
 
+%% Don't try to decode the value of a Failed-AVP component since it
+%% probably won't.
+d('Failed-AVP' = Name, Avp, Acc) ->
+    decode_AVP(Name, Avp, Acc);
+
+%% Or try to decode.
 d(Name, Avp, {Avps, Acc}) ->
     #diameter_avp{name = AvpName,
                   data = Data}
@@ -265,17 +247,32 @@ d(Name, Avp, {Avps, Acc}) ->
                              ?LINE,
                              {Reason, Avp, erlang:get_stacktrace()}),
             {Rec, Failed} = Acc,
-            {[Avp|Avps], {Rec, [{rc(Reason), Avp} | Failed]}}
+            {[Avp|Avps], {Rec, [rc(Reason, Avp) | Failed]}}
     end.
 
+%% 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}.
+
 %% rc/1
 
 %% 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.
-rc({'DIAMETER', RC, _}) ->
-    RC;
+rc({'DIAMETER', 5014 = RC, _}, #diameter_avp{name = AvpName} = Avp) ->
+    {RC, Avp#diameter_avp{data = empty_value(AvpName)}};
 
 %% 3588:
 %%
@@ -283,20 +280,13 @@ rc({'DIAMETER', RC, _}) ->
 %%      The request contained an AVP with an invalid value in its data
 %%      portion.  A Diameter message indicating this error MUST include
 %%      the offending AVPs within a Failed-AVP AVP.
-rc(_) ->
-    5004.
+rc(_, Avp) ->
+    {5004, Avp}.
 
 %% ungroup/2
-%%
-%% Returns: {Avp, Dec}
-%%
-%%          Avp = #diameter_avp{}    if type is not Grouped
-%%              | list of Avp        where first element has type Grouped
-%%                                   and following elements are its component
-%%                                   AVP's.
-%%              = as for decode_avps/2
-%%
-%%          Dec = #diameter_avp{}, either Avp or its head in the list case.
+
+-spec ungroup(term(), #diameter_avp{})
+   -> {avp(), #diameter_avp{}}.
 
 %% The decoded value in the Grouped case is as returned by grouped_avp/3:
 %% a record and a list of component AVP's.
@@ -386,23 +376,29 @@ value('AVP', Avp) ->
 value(_, Avp) ->
     Avp#diameter_avp.value.
 
-%%% ---------------------------------------------------------------------------
-%%% # grouped_avp/3
-%%% ---------------------------------------------------------------------------
+%% ---------------------------------------------------------------------------
+%% # grouped_avp/3
+%% ---------------------------------------------------------------------------
+
+-spec grouped_avp(decode, avp_name(), binary())
+   -> {avp_record(), [avp()]};
+                 (encode, avp_name(), avp_record() | avp_values())
+   -> binary()
+    | no_return().
 
 grouped_avp(decode, Name, Data) ->
     {Rec, Avps, []} = decode_avps(Name, diameter_codec:collect_avps(Data)),
     {Rec, Avps};
-%% Note that a failed match here will result in 5004. Note that this is
-%% the only AVP type that doesn't just return the decoded value, also
-%% returning the list of component #diameter_avp{}'s.
+%% A failed match here will result in 5004. Note that this is the only
+%% AVP type that doesn't just return the decoded record, also
+%% returning the list of component AVP's.
 
 grouped_avp(encode, Name, Data) ->
     encode_avps(Name, Data).
 
-%%% ---------------------------------------------------------------------------
-%%% # empty_group/1
-%%% ---------------------------------------------------------------------------
+%% ---------------------------------------------------------------------------
+%% # empty_group/1
+%% ---------------------------------------------------------------------------
 
 empty_group(Name) ->
     list_to_binary(empty_body(Name)).
@@ -423,9 +419,9 @@ z(Name) ->
     Bin = diameter_codec:pack_avp(avp_header(Name), empty_value(Name)),
     << <<0>> || <<_>> <= Bin >>.
 
-%%% ---------------------------------------------------------------------------
-%%% # empty/1
-%%% ---------------------------------------------------------------------------
+%% ---------------------------------------------------------------------------
+%% # empty/1
+%% ---------------------------------------------------------------------------
 
 empty(AvpName) ->
     avp(encode, zero, AvpName).
diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl
index 09b99b2cae..6c0e73de36 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -38,6 +38,10 @@
 
 -define(MASK(N,I), ((I) band (1 bsl (N)))).
 
+-type u32() :: 0..16#FFFFFFFF.
+-type u24() :: 0..16#FFFFFF.
+-type u1()  :: 0..1.
+
 %%     0                   1                   2                   3
 %%     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
 %%    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
@@ -55,9 +59,13 @@
 %%    +-+-+-+-+-+-+-+-+-+-+-+-+-
 
 %%% ---------------------------------------------------------------------------
-%%% # encode/[2-4]
+%%% # encode/2
 %%% ---------------------------------------------------------------------------
 
+-spec encode(module(), Msg :: term())
+   -> #diameter_packet{}
+    | no_return().
+
 encode(Mod, #diameter_packet{} = Pkt) ->
     try
         e(Mod, Pkt)
@@ -217,6 +225,9 @@ rec2msg(Mod, Rec) ->
 
 %% Unsuccessfully decoded AVPs will be placed in #diameter_packet.errors.
 
+-spec decode(module(), #diameter_packet{} | bitstring())
+   -> #diameter_packet{}.
+
 decode(Mod, Pkt) ->
     decode(Mod:id(), Mod, Pkt).
 
@@ -225,9 +236,9 @@ decode(Mod, Pkt) ->
 %% question.
 decode(?APP_ID_RELAY, _, #diameter_packet{} = Pkt) ->
     case collect_avps(Pkt) of
-        {Bs, As} ->
+        {E, As} ->
             Pkt#diameter_packet{avps = As,
-                                errors = [Bs]};
+                                errors = [E]};
         As ->
             Pkt#diameter_packet{avps = As}
     end;
@@ -251,12 +262,12 @@ decode(Id, Mod, Bin)
   when is_bitstring(Bin) ->
     decode(Id, Mod, #diameter_packet{header = decode_header(Bin), bin = Bin}).
 
-decode_avps(MsgName, Mod, Pkt, {Bs, Avps}) ->  %% invalid avp bits ...
+decode_avps(MsgName, Mod, Pkt, {E, Avps}) ->
     ?LOG(invalid, Pkt#diameter_packet.bin),
     #diameter_packet{errors = Failed}
         = P
         = decode_avps(MsgName, Mod, Pkt, Avps),
-    P#diameter_packet{errors = [Bs | Failed]};
+    P#diameter_packet{errors = [E | Failed]};
 
 decode_avps('', Mod, Pkt, Avps) ->  %% unknown message ...
     ?LOG(unknown, {Mod, Pkt#diameter_packet.header}),
@@ -275,6 +286,10 @@ decode_avps(MsgName, Mod, Pkt, Avps) ->  %% ... or not
 %%% # decode_header/1
 %%% ---------------------------------------------------------------------------
 
+-spec decode_header(bitstring())
+   -> #diameter_header{}
+    | false.
+
 decode_header(<<Version:8,
                 MsgLength:24,
                 CmdFlags:1/binary,
@@ -324,6 +339,13 @@ decode_header(_) ->
 %% wraparound counter. The 8-bit counter is incremented each time the
 %% system is restarted.
 
+-spec sequence_numbers(#diameter_packet{}
+                       | #diameter_header{}
+                       | binary()
+                       | Seq)
+   -> Seq
+ when Seq :: {HopByHopId :: u32(), EndToEndId :: u32()}.
+
 sequence_numbers({_,_} = T) ->
     T;
 
@@ -345,6 +367,9 @@ sequence_numbers(<<_:12/binary, H:32, E:32, _/binary>>) ->
 %%% # hop_by_hop_id/2
 %%% ---------------------------------------------------------------------------
 
+-spec hop_by_hop_id(u32(), binary())
+   -> binary().
+
 hop_by_hop_id(Id, <<H:12/binary, _:32, T/binary>>) ->
     <<H/binary, Id:32, T/binary>>.
 
@@ -352,6 +377,10 @@ hop_by_hop_id(Id, <<H:12/binary, _:32, T/binary>>) ->
 %%% # msg_name/2
 %%% ---------------------------------------------------------------------------
 
+-spec msg_name(module(), #diameter_header{})
+   -> atom()
+    | {ApplicationId :: u32(), CommandCode :: u24(), Rbit :: u1()}.
+
 msg_name(Dict0, #diameter_header{application_id = ?APP_ID_COMMON,
                                  cmd_code = C,
                                  is_request = R}) ->
@@ -367,6 +396,9 @@ msg_name(_, Hdr) ->
 %%% # msg_id/1
 %%% ---------------------------------------------------------------------------
 
+-spec msg_id(#diameter_packet{} | #diameter_header{})
+   -> {ApplicationId :: u32(), CommandCode :: u24(), Rbit :: u1()}.
+
 msg_id(#diameter_packet{msg = [#diameter_header{} = Hdr | _]}) ->
     msg_id(Hdr);
 
@@ -389,6 +421,12 @@ msg_id(<<_:32, Rbit:1, _:7, CmdCode:24, ApplId:32, _/bitstring>>) ->
 %% order in the binary. Note also that grouped avp's aren't unraveled,
 %% only those at the top level.
 
+-spec collect_avps(#diameter_packet{} | bitstring())
+   -> [Avp]
+    | {Error, [Avp]}
+ when Avp   :: #diameter_avp{},
+      Error :: {5014, #diameter_avp{}}.
+
 collect_avps(#diameter_packet{bin = Bin}) ->
     <<_:20/binary, Avps/bitstring>> = Bin,
     collect_avps(Avps);
@@ -403,8 +441,8 @@ collect_avps(Bin, N, Acc) ->
         {Rest, AVP} ->
             collect_avps(Rest, N+1, [AVP#diameter_avp{index = N} | Acc])
     catch
-        ?FAILURE(_) ->
-            {Bin, Acc}
+        ?FAILURE(Error) ->
+            {Error, Acc}
     end.
 
 %%     0                   1                   2                   3
@@ -422,44 +460,87 @@ collect_avps(Bin, N, Acc) ->
 %% split_avp/1
 
 split_avp(Bin) ->
-    8 =< size(Bin) orelse ?THROW(truncated_header),
+    {Code, V, M, P, Len, HdrLen} = split_head(Bin),
+    {Data, B} = split_data(Bin, HdrLen, Len - HdrLen),
+
+    {B, #diameter_avp{code = Code,
+                      vendor_id = V,
+                      is_mandatory = 1 == M,
+                      need_encryption = 1 == P,
+                      data = Data}}.
+
+%% split_head/1
+
+split_head(<<Code:32, 1:1, M:1, P:1, _:5, Len:24, V:32, _/bitstring>>) ->
+    {Code, V, M, P, Len, 12};
+
+split_head(<<Code:32, 0:1, M:1, P:1, _:5, Len:24, _/bitstring>>) ->
+    {Code, undefined, M, P, Len, 8};
 
-    <<Code:32, Flags:1/binary, Length:24, Rest/bitstring>>
-        = Bin,
+split_head(Bin) ->
+    ?THROW({5014, #diameter_avp{data = Bin}}).
 
-    8 =< Length orelse ?THROW(invalid_avp_length),
+%% 3588:
+%%
+%%   DIAMETER_INVALID_AVP_LENGTH        5014
+%%      The request contained an AVP with an invalid length.  A Diameter
+%%      message indicating this error MUST include the offending AVPs
+%%      within a Failed-AVP AVP.
 
-    DataSize = Length - 8,        % size(Code+Flags+Length) = 8 octets
-    PadSize = (4 - (DataSize rem 4)) rem 4,
+%% 6733:
+%%
+%%    DIAMETER_INVALID_AVP_LENGTH 5014
+%%
+%%       The request contained an AVP with an invalid length.  A Diameter
+%%       message indicating this error MUST include the offending AVPs
+%%       within a Failed-AVP AVP.  In cases where the erroneous AVP length
+%%       value exceeds the message length or is less than the minimum AVP
+%%                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+%%       header length, it is sufficient to include the offending AVP
+%%       ^^^^^^^^^^^^^
+%%       header and a zero filled payload of the minimum required length
+%%       for the payloads data type.  If the AVP is a Grouped AVP, the
+%%       Grouped AVP header with an empty payload would be sufficient to
+%%       indicate the offending AVP.  In the case where the offending AVP
+%%       header cannot be fully decoded when the AVP length is less than
+%%       the minimum AVP header length, it is sufficient to include an
+%%       offending AVP header that is formulated by padding the incomplete
+%%       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
+%% (2) the last sentence covers this case.
 
-    DataSize + PadSize =< size(Rest)
-        orelse ?THROW(truncated_data),
+%% split_data/3
 
-    <<Data:DataSize/binary, _:PadSize/binary, R/bitstring>>
-        = Rest,
-    <<Vbit:1, Mbit:1, Pbit:1, _Reserved:5>>
-        = Flags,
+split_data(Bin, HdrLen, Len)
+  when 0 =< Len ->
+    split_data(Bin, HdrLen, Len, (4 - (Len rem 4)) rem 4);
 
-    0 == Vbit orelse 4 =< size(Data)
-        orelse ?THROW(truncated_vendor_id),
+split_data(_, _, _) ->
+    invalid_avp_length().
 
-    {Vid, D} = vid(Vbit, Data),
-    {R, #diameter_avp{code = Code,
-                      vendor_id = Vid,
-                      is_mandatory = 1 == Mbit,
-                      need_encryption = 1 == Pbit,
-                      data = D}}.
+%% split_data/4
 
-%% The RFC is a little misleading when stating that OctetString is
-%% padded to a 32-bit boundary while other types align naturally. All
-%% other types are already multiples of 32 bits so there's no need to
-%% distinguish between types here. Any invalid lengths will result in
-%% decode error in diameter_types.
+split_data(Bin, HdrLen, Len, Pad) ->
+    <<_:HdrLen/binary, T/bitstring>> = Bin,
+    case T of
+        <<Data:Len/binary, _:Pad/binary, Rest/bitstring>> ->
+            {Data, Rest};
+        _ ->
+            invalid_avp_length()
+    end.
+
+%% invalid_avp_length/0
+%%
+%% AVP Length doesn't mesh with payload. Induce a decode error by
+%% returning a payload that no valid Diameter type can have. This is
+%% so that a known AVP will result in 5014 error with a zero'd
+%% payload. Here we simply don't know how to construct this payload.
+%% (Yes, this solution is an afterthought.)
 
-vid(1, <<Vid:32, Data/bitstring>>) ->
-    {Vid, Data};
-vid(0, Data) ->
-    {undefined, Data}.
+invalid_avp_length() ->
+    {<<0:1>>, <<>>}.
 
 %%% ---------------------------------------------------------------------------
 %%% # pack_avp/1
@@ -474,20 +555,35 @@ vid(0, Data) ->
 pack_avp(#diameter_avp{data = [#diameter_avp{} | _] = Avps} = A) ->
     pack_avp(A#diameter_avp{data = encode_avps(Avps)});
 
-%% ... data as a type/value tuple, possibly with header data, ...
+%% ... data as a type/value tuple ...
 pack_avp(#diameter_avp{data = {Type, Value}} = A)
   when is_atom(Type) ->
     pack_avp(A#diameter_avp{data = diameter_types:Type(encode, Value)});
+
+%% ... with a header in various forms ...
 pack_avp(#diameter_avp{data = {{_,_,_} = T, {Type, Value}}}) ->
     pack_avp(T, iolist_to_binary(diameter_types:Type(encode, Value)));
+
 pack_avp(#diameter_avp{data = {{_,_,_} = T, Bin}})
   when is_binary(Bin) ->
     pack_avp(T, Bin);
+
 pack_avp(#diameter_avp{data = {Dict, Name, Value}} = A) ->
     {Code, _Flags, Vid} = Hdr = Dict:avp_header(Name),
     {Name, Type} = Dict:avp_name(Code, Vid),
     pack_avp(A#diameter_avp{data = {Hdr, {Type, Value}}});
 
+pack_avp(#diameter_avp{code = undefined, data = Bin})
+  when is_binary(Bin) ->
+    %% Reset the AVP Length of an AVP Header resulting from a 5014
+    %% error. The RFC doesn't explicitly say to do this but the
+    %% receiver can't correctly extract this and following AVP's
+    %% without a correct length. On the downside, the header doesn't
+    %% reveal if the received header has been padded.
+    Pad = 8*header_length(Bin) - bit_size(Bin),
+    Len = size(<<H:5/binary, _:24, T/binary>> = <<Bin/bitstring, 0:Pad>>),
+    <<H/binary, Len:24, T/binary>>;
+
 %% ... or as an iolist.
 pack_avp(#diameter_avp{code = Code,
                        vendor_id = V,
@@ -499,6 +595,11 @@ pack_avp(#diameter_avp{code = Code,
                                             {P, 2#00100000}]),
     pack_avp({Code, Flags, V}, iolist_to_binary(Data)).
 
+header_length(<<_:32, 1:1, _/bitstring>>) ->
+    12;
+header_length(_) ->
+    8.
+
 flag_avp({true, B}, F) ->
     F bor B;
 flag_avp({false, _}, F) ->
diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl
index 25b902e3f2..820d37535a 100644
--- a/lib/diameter/src/base/diameter_traffic.erl
+++ b/lib/diameter/src/base/diameter_traffic.erl
@@ -226,10 +226,10 @@ recv_R(false = No, _, _, _, _) ->  %% transport has gone down
 
 collect_avps(Pkt) ->
     case diameter_codec:collect_avps(Pkt) of
-        {_Bs, As} ->
-            As;
-        As ->
-            As
+        {_Error, Avps} ->
+            Avps;
+        Avps ->
+            Avps
     end.
 
 %% recv_R/6
@@ -300,7 +300,7 @@ errors(_, #diameter_packet{header = #diameter_header{version = V},
 %%      AVP's definition.
 
 errors(_, #diameter_packet{errors = [Bs | Es]} = Pkt)
-  when is_bitstring(Bs) ->
+  when is_bitstring(Bs) ->  %% from old code
     Pkt#diameter_packet{errors = [3009 | Es]};
 
 %%   DIAMETER_COMMAND_UNSUPPORTED       3001
@@ -595,71 +595,87 @@ reply([Msg], Dict, TPid, Dict0, Fs, ReqPkt)
        is_tuple(Msg) ->
     reply(Msg, Dict, TPid, Dict0, Fs, ReqPkt#diameter_packet{errors = []});
 
-%% No errors or a diameter_header/avp list.
 reply(Msg, Dict, TPid, Dict0, Fs, ReqPkt) ->
-    Pkt = encode(Dict, reset(make_answer_packet(Msg, ReqPkt), Dict), Fs),
+    Pkt = encode(Dict,
+                 reset(make_answer_packet(Msg, ReqPkt), Dict, Dict0),
+                 Fs),
     incr(send, Pkt, Dict, TPid, Dict0),  %% count outgoing result codes
     send(TPid, Pkt).
 
-%% reset/2
+%% reset/3
 
 %% Header/avps list: send as is.
-reset(#diameter_packet{msg = [#diameter_header{} | _]} = Pkt, _) ->
+reset(#diameter_packet{msg = [#diameter_header{} | _]} = Pkt, _, _) ->
     Pkt;
 
 %% No errors to set or errors explicitly ignored.
-reset(#diameter_packet{errors = Es} = Pkt, _)
+reset(#diameter_packet{errors = Es} = Pkt, _, _)
   when Es == [];
        Es == false ->
     Pkt;
 
 %% Otherwise possibly set Result-Code and/or Failed-AVP.
-reset(#diameter_packet{msg = Msg, errors = Es} = Pkt, Dict) ->
-    Pkt#diameter_packet{msg = reset(Msg, Dict, Es)}.
-
-%% reset/3
-
-reset(Msg, Dict, Es)
-  when is_list(Es) ->
-    {E3, E5, Fs} = partition(Es),
-    FailedAVP = failed_avp(Msg, lists:reverse(Fs), Dict),
-    reset(set(Msg, FailedAVP, Dict),
-          Dict,
-          choose(is_answer_message(Msg, Dict), E3, E5));
-
-reset(Msg, Dict, N)
-  when is_integer(N) ->
-    ResultCode = rc(Msg, {'Result-Code', N}, Dict),
-    set(Msg, ResultCode, Dict);
-
-reset(Msg, _, _) ->
-    Msg.
-
-partition(Es) ->
-    lists:foldl(fun pacc/2, {false, false, []}, Es).
+reset(#diameter_packet{msg = Msg, errors = Es} = Pkt, Dict, Dict0) ->
+    {RC, Failed} = select_error(Msg, Es, Dict0),
+    Pkt#diameter_packet{msg = reset(Msg, Dict, RC, Failed)}.
 
-%% Note that the errors list can contain not only integer() and
-%% {integer(), #diameter_avp{}} but also #diameter_avp{}. The latter
-%% isn't something that's returned by decode but can be set in a reply
-%% for encode.
+%% select_error/3
+%%
+%% Extract the first appropriate RC or {RC, #diameter_avp{}}
+%% pair from an errors list, and accumulate all #diameter_avp{}.
+%%
+%% RFC 6733:
+%%
+%%  7.5.  Failed-AVP AVP
+%%
+%%   The Failed-AVP AVP (AVP Code 279) is of type Grouped and provides
+%%   debugging information in cases where a request is rejected or not
+%%   fully processed due to erroneous information in a specific AVP.  The
+%%   value of the Result-Code AVP will provide information on the reason
+%%   for the Failed-AVP AVP.  A Diameter answer message SHOULD contain an
+%%   instance of the Failed-AVP AVP that corresponds to the error
+%%   indicated by the Result-Code AVP.  For practical purposes, this
+%%   Failed-AVP would typically refer to the first AVP processing error
+%%   that a Diameter node encounters.
+
+select_error(Msg, Es, Dict0) ->
+    {RC, Avps} = lists:foldl(fun(T,A) -> select(T, A, Dict0) end,
+                             {is_answer_message(Msg, Dict0), []},
+                             Es),
+    {RC, lists:reverse(Avps)}.
+
+%% Only integer() and {integer(), #diameter_avp{}} are the result of
+%% decode. #diameter_avp{} can only be set in a reply for encode.
+
+select(#diameter_avp{} = A, {RC, As}, _) ->
+    {RC, [A|As]};
+
+select(_, {RC, _} = Acc, _)
+  when is_integer(RC) ->
+    Acc;
 
-pacc({RC, #diameter_avp{} = A}, {E3, E5, Acc})
+select({RC, #diameter_avp{} = A}, {IsAns, As} = Acc, Dict0)
   when is_integer(RC) ->
-    pacc(RC, {E3, E5, [A|Acc]});
+    case is_result(RC, IsAns, Dict0) of
+        true  -> {RC, [A|As]};
+        false -> Acc
+    end;
 
-pacc(#diameter_avp{} = A, {E3, E5, Acc}) ->
-    {E3, E5, [A|Acc]};
+select(RC, {IsAns, As} = Acc, Dict0)
+  when is_boolean(IsAns), is_integer(RC) ->
+    case is_result(RC, IsAns, Dict0) of
+        true  -> {RC, As};
+        false -> Acc
+    end.
 
-pacc(RC, {false, E5, Acc})
-  when 3 == RC div 1000 ->
-    {RC, E5, Acc};
+%% reset/4
 
-pacc(RC, {E3, false, Acc})
-  when 5 == RC div 1000 ->
-    {E3, RC, Acc};
+reset(Msg, Dict, RC, Avps) ->
+    FailedAVP = failed_avp(Msg, Avps, Dict),
+    ResultCode = rc(Msg, RC, Dict),
+    set(set(Msg, FailedAVP, Dict), ResultCode, Dict).
 
-pacc(_, Acc) ->
-    Acc.
+%% eval_packet/2
 
 eval_packet(Pkt, Fs) ->
     lists:foreach(fun(F) -> diameter_lib:eval([F,Pkt]) end, Fs).
@@ -725,29 +741,34 @@ set(Rec, Avps, Dict) ->
 %% the arity is 1 or {0,1}. In other cases (which probably shouldn't
 %% exist in practise) we can't know what's appropriate.
 
-rc([MsgName | _], {'Result-Code' = K, RC} = T, Dict) ->
-    case Dict:avp_arity(MsgName, 'Result-Code') of
-        1     -> [T];
+rc(_, B, _)
+  when is_boolean(B) ->
+    [];
+
+rc([MsgName | _], RC, Dict) ->
+    K = 'Result-Code',
+    case Dict:avp_arity(MsgName, K) of
+        1     -> [{K, RC}];
         {0,1} -> [{K, [RC]}];
         _     -> []
     end;
 
-rc(Rec, T, Dict) ->
-    rc([Dict:rec2msg(element(1, Rec))], T, Dict).
+rc(Rec, RC, Dict) ->
+    rc([Dict:rec2msg(element(1, Rec))], RC, Dict).
 
 %% failed_avp/3
 
 failed_avp(_, [] = No, _) ->
     No;
 
-failed_avp(Rec, Failed, Dict) ->
-    [fa(Rec, [{'AVP', Failed}], Dict)].
+failed_avp(Rec, Avps, Dict) ->
+    [failed(Rec, [{'AVP', Avps}], Dict)].
 
 %% Reply as name and tuple list ...
-fa([MsgName | Values], FailedAvp, Dict) ->
-    R = Dict:msg2rec(MsgName),
+failed([MsgName | Values], FailedAvp, Dict) ->
+    RecName = Dict:msg2rec(MsgName),
     try
-        Dict:'#info-'(R, {index, 'Failed-AVP'}),
+        Dict:'#info-'(RecName, {index, 'Failed-AVP'}),
         {'Failed-AVP', [FailedAvp]}
     catch
         error: _ ->
@@ -758,8 +779,10 @@ fa([MsgName | Values], FailedAvp, Dict) ->
     end;
 
 %% ... or record.
-fa(Rec, FailedAvp, Dict) ->
+failed(Rec, FailedAvp, Dict) ->
     try
+        RecName = element(1, Rec),  
+        Dict:'#info-'(RecName, {index, 'Failed-AVP'}),
         {'Failed-AVP', [FailedAvp]}
     catch
         error: _ ->
diff --git a/lib/diameter/test/diameter_3xxx_SUITE.erl b/lib/diameter/test/diameter_3xxx_SUITE.erl
index 89c78d8b57..0ec0d5020f 100644
--- a/lib/diameter/test/diameter_3xxx_SUITE.erl
+++ b/lib/diameter/test/diameter_3xxx_SUITE.erl
@@ -40,7 +40,7 @@
          send_unknown_application/1,
          send_unknown_command/1,
          send_ok/1,
-         send_invalid_avp_bits/1,
+         send_invalid_hdr_bits/1,
          send_missing_avp/1,
          send_ignore_missing_avp/1,
          send_double_error/1,
@@ -136,7 +136,7 @@ tc() ->
     [send_unknown_application,
      send_unknown_command,
      send_ok,
-     send_invalid_avp_bits,
+     send_invalid_hdr_bits,
      send_missing_avp,
      send_ignore_missing_avp,
      send_double_error,
@@ -216,27 +216,26 @@ send_ok([_,_]) ->
 send_ok(Config) ->
     send_ok(?group(Config)).
 
-%% send_invalid_avp_bits/1
+%% send_invalid_hdr_bits/1
 %%
-%% Send a request with an incorrect length on the optional
-%% Origin-State-Id that a callback ignores.
+%% Send a request with an incorrect E-bit that a callback ignores.
 
 %% Callback answers.
-send_invalid_avp_bits([callback, _]) ->
+send_invalid_hdr_bits([callback, _]) ->
     #diameter_base_STA{'Result-Code' = 2001,  %% SUCCESS
                        'Failed-AVP' = [],
                        'AVP' = []}
         = call();
 
 %% diameter answers.
-send_invalid_avp_bits([_,_]) ->
-    #'diameter_base_answer-message'{'Result-Code' = 3009, %% INVALID_AVP_BITS
+send_invalid_hdr_bits([_,_]) ->
+    #'diameter_base_answer-message'{'Result-Code' = 3008, %% INVALID_HDR_BITS
                                     'Failed-AVP' = [],
                                     'AVP' = []}
         = call();
 
-send_invalid_avp_bits(Config) ->
-    send_invalid_avp_bits(?group(Config)).
+send_invalid_hdr_bits(Config) ->
+    send_invalid_hdr_bits(?group(Config)).
 
 %% send_missing_avp/1
 %%
@@ -282,8 +281,7 @@ send_ignore_missing_avp(Config) ->
 
 %% send_double_error/1
 %%
-%% Send a request with both an incorrect length on the optional
-%% Origin-State-Id and a missing AVP.
+%% Send a request with both an invalid E-bit and a missing AVP.
 
 %% Callback answers with STA.
 send_double_error([callback, _]) ->
@@ -294,8 +292,8 @@ send_double_error([callback, _]) ->
 
 %% diameter answers with answer-message.
 send_double_error([_,_]) ->
-    #'diameter_base_answer-message'{'Result-Code' = 3009, %% INVALID_AVP_BITS
-                                    'Failed-AVP' = [_],
+    #'diameter_base_answer-message'{'Result-Code' = 3008, %% INVALID_HDR_BITS
+                                    'Failed-AVP' = [],
                                     'AVP' = []}
         = call();
 
@@ -392,20 +390,16 @@ prepare(Pkt, Caps, T)
        T == send_5xxx ->
     sta(Pkt, Caps);
 
-prepare(Pkt0, Caps, send_invalid_avp_bits) ->
-    Req0 = sta(Pkt0, Caps),
-    %% Append an Origin-State-Id with an incorrect AVP Length in order
-    %% to force 3009.
-    Req = Req0#diameter_base_STR{'Origin-State-Id' = [7]},
-    #diameter_packet{bin = Bin}
+prepare(Pkt0, Caps, send_invalid_hdr_bits) ->
+    Req = sta(Pkt0, Caps),
+    %% Set the E-bit to force 3008.
+    #diameter_packet{bin = <<H:34, 0:1, T/bitstring>>}
         = Pkt
         = diameter_codec:encode(?DICT, Pkt0#diameter_packet{msg = Req}),
-    Offset = size(Bin) - 12 + 5,
-    <<H:Offset/binary, Len:24, T/binary>> = Bin,
-    Pkt#diameter_packet{bin = <<H/binary, (Len + 2):24, T/binary>>};
+    Pkt#diameter_packet{bin = <<H:34, 1:1, T/bitstring>>};
 
 prepare(Pkt0, Caps, send_double_error) ->
-    dehost(prepare(Pkt0, Caps, send_invalid_avp_bits));
+    dehost(prepare(Pkt0, Caps, send_invalid_hdr_bits));
 
 prepare(Pkt, Caps, T)
   when T == send_missing_avp;
@@ -480,9 +474,7 @@ request(send_3xxx, _Req, _Caps) ->
 request(send_5xxx, _Req, _Caps) ->
     {answer_message, 5999};
 
-request(send_invalid_avp_bits, Req, Caps) ->
-    #diameter_base_STR{'Origin-State-Id' = []}
-        = Req,
+request(send_invalid_hdr_bits, Req, Caps) ->
     %% Default errors field but a non-answer-message and only 3xxx
     %% errors detected means diameter sets neither Result-Code nor
     %% Failed-AVP.
diff --git a/lib/diameter/test/diameter_codec_SUITE_data/diameter_test_unknown.erl b/lib/diameter/test/diameter_codec_SUITE_data/diameter_test_unknown.erl
index bce3d78a37..49f2158b1a 100644
--- a/lib/diameter/test/diameter_codec_SUITE_data/diameter_test_unknown.erl
+++ b/lib/diameter/test/diameter_codec_SUITE_data/diameter_test_unknown.erl
@@ -71,6 +71,6 @@ dec('AR', #diameter_packet
 dec('BR', #diameter_packet
            {msg = #recv_BR{'Origin-Host'  = ?HOST,
                            'Origin-Realm' = ?REALM},
-            errors = [{5008, ?NOT_MANDATORY_YYY},
-                      {5001, ?MANDATORY_XXX}]}) ->
+            errors = [{5001, ?MANDATORY_XXX},
+                      {5008, ?NOT_MANDATORY_YYY}]}) ->
     ok.
diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl
index 3878d6717a..f222e4a3ab 100644
--- a/lib/diameter/test/diameter_traffic_SUITE.erl
+++ b/lib/diameter/test/diameter_traffic_SUITE.erl
@@ -487,31 +487,31 @@ send_unsupported_version(Config) ->
 
 %% Send a request containing an AVP length > data size.
 send_long_avp_length(Config) ->
-    Req = ['STR', {'Termination-Cause', ?LOGOUT}],
-
-    ?answer_message(?INVALID_AVP_BITS)
-        = call(Config, Req).
+    send_invalid_avp_length(Config).
 
 %% Send a request containing an AVP length < data size.
 send_short_avp_length(Config) ->
-    Req = ['STR', {'Termination-Cause', ?LOGOUT}],
+    send_invalid_avp_length(Config).
 
-    ['STA', _SessionId, {'Result-Code', ?INVALID_AVP_LENGTH} | _]
-        = call(Config, Req).
+%% Send a request containing an AVP whose advertised length is < 8.
+send_zero_avp_length(Config) ->
+    send_invalid_avp_length(Config).
 
 %% Send a request containing an AVP length that doesn't match the
 %% AVP's type.
 send_invalid_avp_length(Config) ->
     Req = ['STR', {'Termination-Cause', ?LOGOUT}],
 
-    ['STA', _SessionId, {'Result-Code', ?INVALID_AVP_LENGTH} | _]
-        = call(Config, Req).
-
-%% Send a request containing an AVP whose advertised length is < 8.
-send_zero_avp_length(Config) ->
-    Req = ['STR', {'Termination-Cause', ?LOGOUT}],
-
-    ?answer_message(?INVALID_AVP_BITS)
+    ['STA', _SessionId,
+            {'Result-Code', ?INVALID_AVP_LENGTH},
+            _OriginHost,
+            _OriginRealm,
+            _UserName,
+            _Class,
+            _ErrorMessage,
+            _ErrorReportingHost,
+            {'Failed-AVP', [#'diameter_base_Failed-AVP'{'AVP' = [_]}]}
+          | _]
         = call(Config, Req).
 
 %% Send a request containing 5xxx errors that the server rejects with
@@ -828,19 +828,25 @@ prepare(Pkt, Caps, N, #group{client_dict0 = Dict0} = Group)
        N == send_zero_avp_length ->
     Req = prepare(Pkt, Caps, Group),
     %% Second last AVP in our STR is Auth-Application-Id of type
-    %% Unsigned32: set AVP Length to a value other than 12.
+    %% Unsigned32: set AVP Length to a value other than 12 and place
+    %% it last in the message (so as not to mess with Termination-Cause).
     #diameter_packet{header = #diameter_header{length = L},
                      bin = B}
         = E
         = diameter_codec:encode(Dict0, Pkt#diameter_packet{msg = Req}),
-    Offset = L - 7 - 12,  %% to AVP Length
-    <<H:Offset/binary, 12:24/integer, T:16/binary>> = B,  %% assert
+    Offset = L - 24,  %% to Auth-Application-Id
+    <<H:Offset/binary,
+      Hdr:5/binary, 12:24/integer, Data:4/binary,
+      T:12/binary>>
+        = B,
     AL = case N of
              send_long_avp_length  -> 13;
              send_short_avp_length -> 11;
              send_zero_avp_length  -> 0
          end,
-    E#diameter_packet{bin = <<H/binary, AL:24/integer, T/binary>>};
+    E#diameter_packet{bin = <<H/binary,
+                              T/binary,
+                              Hdr/binary, AL:24/integer, Data/binary>>};
 
 prepare(Pkt, Caps, N, #group{client_dict0 = Dict0} = Group)
   when N == send_invalid_avp_length;