Fix relay encode of decoded diameter_avp lists

Commit c74b593a fixed the problem that a decoded deep diameter_avp list
couldn't be encoded, but did so in the wrong way: there's no need to
reencode component AVPs since the Grouped AVP itself already contains
the encoded binary. The blunder caused diameter_codec:pack_avp/1 to fail
if the first element of the AVP list to be encoded was itself a list.

Thanks to Andrzej Trawiński for reporting the problem.

2 files changed, 54 insertions, 11 deletions

diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl
index bf2fe8e7ca..2ad971a422 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -655,16 +655,23 @@ split_data(Bin, Len) ->
 
 %% The normal case here is data as an #diameter_avp{} list or an
 %% iolist, which are the cases that generated codec modules use. The
-%% other case is as a convenience in the relay case in which the
+%% other cases are a convenience in the relay case in which the
 %% 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)});
+%% Decoded Grouped AVP with decoded components: ignore components
+%% since they're already encoded in the Grouped AVP.
+pack_avp([#diameter_avp{} = Grouped | _Components]) ->
+    pack_avp(Grouped);
 
-%% ... data as a type/value tuple ...
+%% Grouped AVP whose components need packing. It's intentional that
+%% this isn't equivalent to [Grouped | Components]: here the
+%% components need to be encoded before wrapping with the Grouped AVP,
+%% and the list is flat, nesting being accomplished in the data
+%% fields.
+pack_avp(#diameter_avp{data = [#diameter_avp{} | _] = Components} = Grouped) ->
+    pack_avp(Grouped#diameter_avp{data = encode_avps(Components)});
+
+%% 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)});
diff --git a/lib/diameter/test/diameter_relay_SUITE.erl b/lib/diameter/test/diameter_relay_SUITE.erl
index 7142239bbb..5f7837e879 100644
--- a/lib/diameter/test/diameter_relay_SUITE.erl
+++ b/lib/diameter/test/diameter_relay_SUITE.erl
@@ -333,13 +333,39 @@ realm(Host) ->
 
 call(Server) ->
     Realm = realm(Server),
+    %% Include some arbitrary AVPs to exercise encode/decode, that
+    %% are received back in the STA.
+    Avps = [#diameter_avp{code = 111,
+                          data = [#diameter_avp{code = 222,
+                                                data = <<222:24>>},
+                                  #diameter_avp{code = 333,
+                                                data = <<333:16>>}]},
+            #diameter_avp{code = 444,
+                          data = <<444:24>>},
+            #diameter_avp{code = 555,
+                          data = [#diameter_avp{code = 666,
+                                                data = [#diameter_avp
+                                                        {code = 777,
+                                                         data = <<7>>}]},
+                                  #diameter_avp{code = 888,
+                                                data = <<8>>},
+                                  #diameter_avp{code = 999,
+                                                data = <<9>>}]}],
+
     Req = ['STR', {'Destination-Realm', Realm},
                   {'Destination-Host', [Server]},
                   {'Termination-Cause', ?LOGOUT},
-                  {'Auth-Application-Id', ?APP_ID}],
+                  {'Auth-Application-Id', ?APP_ID},
+                  {'AVP', Avps}],
+
     #diameter_base_STA{'Result-Code' = ?SUCCESS,
                        'Origin-Host' = Server,
-                       'Origin-Realm' = Realm}
+                       'Origin-Realm' = Realm,
+                       %% Unknown AVPs can't be decoded as Grouped since
+                       %% types aren't known.
+                       'AVP' = [#diameter_avp{code = 111},
+                                #diameter_avp{code = 444},
+                                #diameter_avp{code = 555}]}
         = call(Req, [{filter, realm}]).
 
 call(Req, Opts) ->
@@ -433,9 +459,18 @@ request(_Pkt, #diameter_caps{origin_host = {OH, _}})
 request(#diameter_packet{msg = #diameter_base_STR{'Session-Id' = SId,
                                                   'Origin-Host' = Host,
                                                   'Origin-Realm' = Realm,
-                                                  'Route-Record' = Route}},
+                                                  'Route-Record' = Route,
+                                                  'AVP' = Avps}},
         #diameter_caps{origin_host  = {OH, _},
                        origin_realm = {OR, _}}) ->
+
+    %% Payloads of unknown AVPs aren't decoded, so we don't know that
+    %% some types here are Grouped.
+    [#diameter_avp{code = 111, vendor_id = undefined},
+     #diameter_avp{code = 444, vendor_id = undefined, data = <<444:24>>},
+     #diameter_avp{code = 555, vendor_id = undefined}]
+        = Avps,
+
     %% The request should have the Origin-Host/Realm of the original
     %% sender.
     R = realm(?CLIENT),
@@ -446,4 +481,5 @@ request(#diameter_packet{msg = #diameter_base_STR{'Session-Id' = SId,
     {reply, #diameter_base_STA{'Result-Code' = ?SUCCESS,
                                'Session-Id' = SId,
                                'Origin-Host' = OH,
-                               'Origin-Realm' = OR}}.
+                               'Origin-Realm' = OR,
+                               'AVP' = Avps}}.