Fix ordering of AVPs in relayed messages

6.1.9 of RFC 6733 states this:

  A relay or proxy agent MUST append a Route-Record AVP to all requests
  forwarded.

The AVP was inserted as the head of the AVP list, not appended, since
the entire AVP list was reversed relative to the received order.

Thanks to Andrzej Trawiński.

2 files changed, 33 insertions, 9 deletions

diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl
index a2b04bfd63..07ad5f97d7 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -90,7 +90,7 @@ encode(Mod, Msg) ->
                                   msg = Msg}).
 
 e(_, #diameter_packet{msg = [#diameter_header{} = Hdr | As]} = Pkt) ->
-    try encode_avps(As) of
+    try encode_avps(reorder(As)) of
         Avps ->
             Length = size(Avps) + 20,
 
@@ -183,26 +183,50 @@ values(Avps) ->
 
 %% Message as a list of #diameter_avp{} ...
 encode_avps(_, _, [#diameter_avp{} | _] = Avps) ->
-    encode_avps(reorder(Avps, [], Avps));
+    encode_avps(reorder(Avps));
 
 %% ... or as a tuple list or record.
 encode_avps(Mod, MsgName, Values) ->
     Mod:encode_avps(MsgName, Values).
 
 %% reorder/1
+%%
+%% Reorder AVPs for the relay case using the index field of
+%% diameter_avp records. Decode populates this field in collect_avps
+%% and presents AVPs in reverse order. A relay then sends the reversed
+%% list with a Route-Record AVP prepended. The goal here is just to do
+%% lists:reverse/1 in Grouped AVPs and the outer list, but only in the
+%% case there are indexed AVPs at all, so as not to reverse lists that
+%% have been explicilty sent (unindexed, in the desired order) as a
+%% diameter_avp list. The effect is the same as lists:keysort/2, but
+%% only on the cases we expect, not a general sort.
+
+reorder(Avps) ->
+    case reorder(Avps, []) of
+        false ->
+            Avps;
+        Sorted ->
+            Sorted
+    end.
 
-reorder([#diameter_avp{index = 0} | _] = Avps, Acc, _) ->
+%% reorder/3
+
+%% In case someone has reversed the list already. (Not likely.)
+reorder([#diameter_avp{index = 0} | _] = Avps, Acc) ->
     Avps ++ Acc;
 
-reorder([#diameter_avp{index = N} = A | Avps], Acc, _)
+%% Assume indexed AVPs are in reverse order.
+reorder([#diameter_avp{index = N} = A | Avps], Acc)
   when is_integer(N) ->
     lists:reverse(Avps, [A | Acc]);
 
-reorder([H | T], Acc, Avps) ->
-    reorder(T, [H | Acc], Avps);
+%% An unindexed AVP.
+reorder([H | T], Acc) ->
+    reorder(T, [H | Acc]);
 
-reorder([], Acc, _) ->
-    Acc.
+%% No indexed members.
+reorder([], _) ->
+    false.
 
 %% encode_avps/1
 
diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl
index 3b62afca47..3717e43e4a 100644
--- a/lib/diameter/src/base/diameter_traffic.erl
+++ b/lib/diameter/src/base/diameter_traffic.erl
@@ -592,7 +592,7 @@ resend(false,
     Route = #diameter_avp{data = {Dict0, 'Route-Record', OH}},
     Seq = diameter_session:sequence(Mask),
     Hdr = Hdr0#diameter_header{hop_by_hop_id = Seq},
-    Msg = [Hdr, Route | Avps],
+    Msg = [Hdr, Route | Avps],  %% reordered at encode
     resend(send_request(SvcName, App, Msg, Opts), Caps, Dict0, Pkt).
 %% The incoming request is relayed with the addition of a
 %% Route-Record. Note the requirement on the return from call/4 below,