From a63a93e6f16368fb82d24b8efd8f8ff9aabf8013 Mon Sep 17 00:00:00 2001
From: Anders Svensson <anders@erlang.org>
Date: Mon, 16 Feb 2015 11:32:53 +0100
Subject: Fix handling of length errors on Grouped AVPs

The decode of a Grouped AVP ignored the case that extracting component
AVPs with diameter_codec:collect_avps/1 returned a tuple, in the case of
a truncated AVP header.
---
 lib/diameter/include/diameter_gen.hrl    | 36 +++++++++++++++++++++++++-------
 lib/diameter/src/base/diameter_codec.erl | 16 +++++++-------
 lib/diameter/src/base/diameter_types.erl |  2 +-
 3 files changed, 37 insertions(+), 17 deletions(-)

(limited to 'lib')

diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl
index 8916286275..8272904856 100644
--- a/lib/diameter/include/diameter_gen.hrl
+++ b/lib/diameter/include/diameter_gen.hrl
@@ -334,8 +334,9 @@ d(Name, Avp, Acc) ->
             {[H | Avps], pack_avp(Name, A, T)}
     catch
         throw: {?TAG, {grouped, RC, ComponentAvps}} ->
-            {Avps, {Rec, Failed}} = Acc,
-            {[[Avp | ComponentAvps] | Avps], {Rec, [{RC, Avp} | Failed]}};
+            {Avps, {Rec, Errors}} = Acc,
+            A = trim(Avp),
+            {[[A | trim(ComponentAvps)] | Avps], {Rec, [{RC, A} | Errors]}};
         error: Reason ->
             d(undefined == Failed orelse is_failed(),
               Reason,
@@ -355,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.
 
@@ -599,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, Es} = decode_avps(Name, diameter_codec:collect_avps(Data)),
-    [] == Es orelse throw({?TAG, {grouped, 5004, Avps}}),  %% decode failure
-    {Rec, Avps};
-%% 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 a2b04bfd63..dd9039d7dc 100644
--- a/lib/diameter/src/base/diameter_codec.erl
+++ b/lib/diameter/src/base/diameter_codec.erl
@@ -561,14 +561,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.
 
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
-- 
cgit v1.2.3