Merge branch 'anders/diameter/grouped_decode/OTP-12475' into maint

* anders/diameter/grouped_decode/OTP-12475:
  Allow encode of decoded diameter_avp list
  Add testcases for diameter_avp decode
  Fix handling of length errors on Grouped AVPs
  Don't discard component diameter_avp list on Grouped AVP decode error
  Fix process dictionary manipulation during message decode

4 files changed, 243 insertions, 22 deletions

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index bc25f7d472..8272904856 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2010-2014. All Rights Reserved.
+%% Copyright Ericsson AB 2010-2015. All Rights Reserved.
 %%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
@@ -25,6 +25,9 @@
 
 -define(THROW(T), throw({?MODULE, T})).
 
+%% Tag common to generated dictionaries.
+-define(TAG, diameter_gen).
+
 %% Key to a value in the process dictionary that determines whether or
 %% not an unrecognized AVP setting the M-bit should be regarded as an
 %% error or not. See is_strict/0.
@@ -48,13 +51,20 @@
 %% dictionary.
 
 putr(K,V) ->
-    put({?MODULE, K}, V).
+    put({?TAG, K}, V).
 
 getr(K) ->
-    get({?MODULE, K}).
+    case get({?TAG, K}) of
+        undefined ->
+            V = erase({?MODULE, K}),  %% written in old code
+            V == undefined orelse putr(K,V),
+            V;
+        V ->
+            V
+    end.
 
 eraser(K) ->
-    erase({?MODULE, K}).
+    erase({?TAG, K}).
 
 %% ---------------------------------------------------------------------------
 %% # encode_avps/2
@@ -313,12 +323,20 @@ d(Name, Avp, Acc) ->
                            %% decode is packed into 'AVP'.
     Mod = dict(Failed),    %% Dictionary to decode in.
 
+    %% On decode, a Grouped AVP is represented as a #diameter_avp{}
+    %% list with AVP as head and component AVPs as tail. On encode,
+    %% data can be a list of component AVPs.
+
     try Mod:avp(decode, Data, AvpName) of
         V ->
             {Avps, T} = 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]}};
         error: Reason ->
             d(undefined == Failed orelse is_failed(),
               Reason,
@@ -338,6 +356,10 @@ d(Name, Avp, Acc) ->
 trim(#diameter_avp{data = <<0:1, Bin/binary>>} = Avp) ->
     Avp#diameter_avp{data = Bin};
 
+trim(Avps)
+  when is_list(Avps) ->
+    lists:map(fun trim/1, Avps);
+
 trim(Avp) ->
     Avp.
 
@@ -582,22 +604,37 @@ value(_, Avp) ->
 %% # grouped_avp/3
 %% ---------------------------------------------------------------------------
 
--spec grouped_avp(decode, avp_name(), binary())
+-spec grouped_avp(decode, avp_name(), bitstring())
    -> {avp_record(), [avp()]};
                  (encode, avp_name(), avp_record() | avp_values())
    -> binary()
     | no_return().
 
+%% Length error induced by diameter_codec:collect_avps/1.
+grouped_avp(decode, _Name, <<0:1, _/binary>>) ->
+    throw({?TAG, {grouped, 5014, []}});
+
 grouped_avp(decode, Name, Data) ->
-    {Rec, Avps, []} = decode_avps(Name, diameter_codec:collect_avps(Data)),
-    {Rec, Avps};
-%% 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_decode(Name, diameter_codec:collect_avps(Data));
 
 grouped_avp(encode, Name, Data) ->
     encode_avps(Name, Data).
 
+%% grouped_decode/2
+%%
+%% Note that Grouped is the only AVP type that doesn't just return a
+%% decoded value, also returning the list of component diameter_avp
+%% records.
+
+grouped_decode(_Name, {Error, Acc}) ->
+    {RC, Avp} = Error,
+    throw({?TAG, {grouped, RC, [Avp | Acc]}});
+
+grouped_decode(Name, ComponentAvps) ->
+    {Rec, Avps, Es} = decode_avps(Name, ComponentAvps),
+    [] == Es orelse throw({?TAG, {grouped, 5004, Avps}}), %% decode failure
+    {Rec, Avps}.
+
 %% ---------------------------------------------------------------------------
 %% # empty_group/1
 %% ---------------------------------------------------------------------------
diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl
index f9bc44cf61..b4ecb63961 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -564,14 +564,14 @@ split_data(Bin, Len) ->
         <<Data:Len/binary, _:Pad/binary, Rest/binary>> ->
             {Data, Rest};
         _ ->
-            %% Header length points past the end of the message. As
-            %% stated in the 6733 text above, it's sufficient to
-            %% return a zero-filled minimal payload if this is a
-            %% request. Do this (in cases that we know the type) by
-            %% inducing a decode failure and letting the dictionary's
-            %% decode (in diameter_gen) deal with it. Here we don't
-            %% know type. If the type isn't known, then the decode
-            %% just strips the extra bit.
+            %% Header length points past the end of the message, or
+            %% doesn't span the header. As stated in the 6733 text
+            %% above, it's sufficient to return a zero-filled minimal
+            %% payload if this is a request. Do this (in cases that we
+            %% know the type) by inducing a decode failure and letting
+            %% the dictionary's decode (in diameter_gen) deal with it.
+            %% Here we don't know type. If the type isn't known, then
+            %% the decode just strips the extra bit.
             {<<0:1, Bin/binary>>, <<>>}
     end.
 
@@ -585,6 +585,8 @@ split_data(Bin, Len) ->
 %% dictionary doesn't know about specific AVP's.
 
 %% Grouped AVP whose components need packing ...
+pack_avp([#diameter_avp{} = A | Avps]) ->
+    pack_avp(A#diameter_avp{data = Avps});
 pack_avp(#diameter_avp{data = [#diameter_avp{} | _] = Avps} = A) ->
     pack_avp(A#diameter_avp{data = encode_avps(Avps)});
 
diff --git a/lib/diameter/src/base/diameter_types.erl b/lib/diameter/src/base/diameter_types.erl
index ca3338be5f..442d90c98b 100644
--- a/lib/diameter/src/base/diameter_types.erl
+++ b/lib/diameter/src/base/diameter_types.erl
@@ -75,7 +75,7 @@
 %%      message indicating this error MUST include the offending AVPs
 %%      within a Failed-AVP AVP.
 %%
--define(INVALID_LENGTH(Bin), erlang:error({'DIAMETER', 5014, Bin})).
+-define(INVALID_LENGTH(Bitstr), erlang:error({'DIAMETER', 5014, Bitstr})).
 
 %% -------------------------------------------------------------------------
 %% 3588, 4.2.  Basic AVP Data Formats
diff --git a/lib/diameter/test/diameter_codec_SUITE.erl b/lib/diameter/test/diameter_codec_SUITE.erl
index cd8ca41f66..64ea90554d 100644
--- a/lib/diameter/test/diameter_codec_SUITE.erl
+++ b/lib/diameter/test/diameter_codec_SUITE.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2010-2013. All Rights Reserved.
+%% Copyright Ericsson AB 2010-2015. All Rights Reserved.
 %%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
@@ -29,6 +29,9 @@
 
 -export([suite/0,
          all/0,
+         groups/0,
+         init_per_group/2,
+         end_per_group/2,
          init_per_testcase/2,
          end_per_testcase/2]).
 
@@ -36,9 +39,13 @@
 -export([base/1,
          gen/1,
          lib/1,
-         unknown/1]).
+         unknown/1,
+         success/1,
+         grouped_error/1,
+         failed_error/1]).
 
 -include("diameter_ct.hrl").
+-include("diameter.hrl").
 
 -define(L, atom_to_list).
 
@@ -48,7 +55,19 @@ suite() ->
     [{timetrap, {seconds, 10}}].
 
 all() ->
-    [base, gen, lib, unknown].
+    [base, gen, lib, unknown, {group, recode}].
+
+groups() ->
+    [{recode, [], [success,
+                   grouped_error,
+                   failed_error]}].
+
+init_per_group(recode, Config) ->
+    ok = diameter:start(),
+    Config.
+
+end_per_group(_, _) ->
+    ok =  diameter:stop().
 
 init_per_testcase(gen, Config) ->
     [{application, ?APP, App}] = diameter_util:consult(?APP, app),
@@ -98,3 +117,166 @@ compile(File) ->
 
 compile(File, Opts) ->
     compile:file(File, [return | Opts]).
+
+%% ===========================================================================
+
+%% Ensure a Grouped AVP is represented by a list in the avps field.
+success(_) ->
+    Avps = [{295, <<1:32>>},  %% Termination-Cause
+            {284, [{280, "Proxy-Host"}, %% Proxy-Info
+                   {33, "Proxy-State"}, %%
+                   {295, <<2:32>>}]}],  %% Termination-Cause
+    #diameter_packet{avps = [#diameter_avp{code = 295,
+                                           value = 1,
+                                           data = <<1:32>>},
+                             [#diameter_avp{code = 284},
+                              #diameter_avp{code = 280},
+                              #diameter_avp{code = 33},
+                              #diameter_avp{code = 295,
+                                            value = 2,
+                                            data = <<2:32>>}]],
+                     errors = []}
+        = str(recode(str(Avps))).
+
+%% ===========================================================================
+
+%% Ensure a Grouped AVP is represented by a list in the avps field
+%% even in the case of a decode error on a component AVP.
+grouped_error(_) ->
+    Avps = [{295, <<1:32>>},  %% Termination-Cause
+            {284, [{295, <<0:32>>},      %% Proxy-Info, Termination-Cause
+                   {280, "Proxy-Host"},
+                   {33, "Proxy-State"}]}],
+    #diameter_packet{avps = [#diameter_avp{code = 295,
+                                           value = 1,
+                                           data = <<1:32>>},
+                             [#diameter_avp{code = 284},
+                              #diameter_avp{code = 295,
+                                            value = undefined,
+                                            data = <<0:32>>},
+                              #diameter_avp{code = 280},
+                              #diameter_avp{code = 33}]],
+                     errors = [{5004, #diameter_avp{code = 284}}]}
+        = str(recode(str(Avps))).
+
+%% ===========================================================================
+
+%% Ensure that a failed decode in Failed-AVP is acceptable, and that
+%% the component AVPs are decoded if possible.
+failed_error(_) ->
+    Avps = [{279, [{295, <<0:32>>},    %% Failed-AVP, Termination-Cause
+                   {258, <<1:32>>},             %% Auth-Application-Id
+                   {284, [{280, "Proxy-Host"},  %% Proxy-Info
+                          {33, "Proxy-State"},
+                          {295, <<0:32>>},      %% Termination-Cause, invalid
+                          {258, <<2:32>>}]}]}], %% Auth-Application-Id
+    #diameter_packet{avps = [[#diameter_avp{code = 279},
+                              #diameter_avp{code = 295,
+                                            value = undefined,
+                                            data = <<0:32>>},
+                              #diameter_avp{code = 258,
+                                            value = 1,
+                                            data = <<1:32>>},
+                              [#diameter_avp{code = 284},
+                               #diameter_avp{code = 280},
+                               #diameter_avp{code = 33},
+                               #diameter_avp{code = 295,
+                                             value = undefined},
+                               #diameter_avp{code = 258,
+                                             value = 2,
+                                             data = <<2:32>>}]]],
+                     errors = []}
+        = sta(recode(sta(Avps))).
+
+%% ===========================================================================
+
+%% str/1
+
+str(#diameter_packet{avps = [#diameter_avp{code = 263},
+                             #diameter_avp{code = 264},
+                             #diameter_avp{code = 296},
+                             #diameter_avp{code = 283},
+                             #diameter_avp{code = 258,
+                                           value = 0}
+                             | T]}
+    = Pkt) ->
+    Pkt#diameter_packet{avps = T};
+
+str(Avps) ->
+    OH = "diameter.erlang.org",
+    OR = "erlang.org",
+    DR = "example.com",
+    Sid = "diameter.erlang.org;123;456",
+
+    [#diameter_header{version = 1,
+                      cmd_code = 275,  %% STR
+                      is_request = true,
+                      application_id = 0,
+                      hop_by_hop_id = 17,
+                      end_to_end_id = 42,
+                      is_proxiable = false,
+                      is_error = false,
+                      is_retransmitted = false}
+     | avp([{263, Sid},  %% Session-Id
+            {264, OH},   %% Origin-Host
+            {296, OR},   %% Origin-Realm
+            {283, DR},   %% Destination-Realm
+            {258, <<0:32>>}]  %% Auth-Application-Id
+           ++ Avps)].
+
+%% sta/1
+
+sta(#diameter_packet{avps = [#diameter_avp{code = 263},
+                             #diameter_avp{code = 268},
+                             #diameter_avp{code = 264},
+                             #diameter_avp{code = 296},
+                             #diameter_avp{code = 278,
+                                           value = 4}
+                             | T]}
+    = Pkt) ->
+    Pkt#diameter_packet{avps = T};
+
+sta(Avps) ->
+    OH = "diameter.erlang.org",
+    OR = "erlang.org",
+    Sid = "diameter.erlang.org;123;456",
+
+    [#diameter_header{version = 1,
+                      cmd_code = 275,  %% STA
+                      is_request = false,
+                      application_id = 0,
+                      hop_by_hop_id = 17,
+                      end_to_end_id = 42,
+                      is_proxiable = false,
+                      is_error = false,
+                      is_retransmitted = false}
+     | avp([{263, Sid},         %% Session-Id
+            {268, <<2002:32>>}, %% Result-Code
+            {264, OH},          %% Origin-Host
+            {296, OR},          %% Origin-Realm
+            {278, <<4:32>>}]    %% Origin-State-Id
+           ++ Avps)].
+
+avp({Code, Data}) ->
+    #diameter_avp{code = Code,
+                  data = avp(Data)};
+
+avp(#diameter_avp{} = A) ->
+    A;
+
+avp([{_,_} | _] = Avps) ->
+    lists:map(fun avp/1, Avps);
+
+avp(V) ->
+    V.
+
+%% recode/1
+
+recode(Msg) ->
+    recode(Msg, diameter_gen_base_rfc6733).
+
+recode(#diameter_packet{} = Pkt, Dict) ->
+    diameter_codec:decode(Dict, diameter_codec:encode(Dict, Pkt));
+
+recode(Msg, Dict) ->
+    recode(#diameter_packet{msg = Msg}, Dict).