Merge pull request #16994 from rabbitmq/amqp-parser

michaelklishin · web-flow · commit 195e549fc7b3 · 2026-07-21T16:50:12.000-07:00
Encode AMQP 1.0 arrays of zero-octet elements as opaque values
diff --git a/deps/amqp10_common/src/amqp10_binary_generator.erl b/deps/amqp10_common/src/amqp10_binary_generator.erl
@@ -245,4 +245,9 @@ generate2(array, {array, Type, List}) ->
     Array = [constructor(Type),
              [generate2(Type, I) || I <- List]],
     S = iolist_size(Array),
-    [<<(S + 4):32, Count:32>>, Array].
+    [<<(S + 4):32, Count:32>>, Array];
+generate2(array, {as_is, 16#f0, Bin}) ->
+    Bin;
+generate2(array, {as_is, 16#e0, <<_Size:8, Count:8, Type>>}) ->
+    %% Nested array elements are always framed as array32, see constructor/1
+    <<5:32, Count:32, Type>>.
diff --git a/deps/amqp10_common/src/amqp10_binary_parser.erl b/deps/amqp10_common/src/amqp10_binary_parser.erl
@@ -35,8 +35,6 @@
 -define(DESCRIPTOR_CODE_DATA, 16#75).
 -define(DESCRIPTOR_CODE_AMQP_SEQUENCE, 16#76).
 -define(DESCRIPTOR_CODE_AMQP_VALUE, 16#77).
--define(MAX_ZERO_WIDTH_ARRAY_COUNT, 10_000).
-
 
 %% server_mode is a special parsing mode used by RabbitMQ when parsing
 %% AMQP message sections from an AMQP client. This mode:
@@ -124,32 +122,40 @@ parse(<<Type, _/binary>>, B) ->
 
 parse_array(UnitSize, Bin) ->
     <<Count:UnitSize, Bin1/binary>> = Bin,
-    parse_array1(Count, Bin1).
+    parse_array1(UnitSize, Count, Bin1).
 
-parse_array1(Count, <<?DESCRIBED, Rest/binary>>) ->
+parse_array1(UnitSize, Count, <<?DESCRIBED, Rest/binary>>) ->
     {Descriptor, B1} = parse(Rest),
     <<_ParsedDescriptorBin:B1/binary, Rest1/binary>> = Rest,
-    {array, Type, List} = parse_array1(Count, Rest1),
-    Values = lists:map(fun (Value) ->
-                               {described, Descriptor, Value}
-                       end, List),
-    % this format cannot represent an empty array of described types
-    {array, {described, Descriptor, Type}, Values};
-parse_array1(Count, <<Type, ArrayBin/binary>>)
+    case parse_array1(UnitSize, Count, Rest1) of
+        {array, Type, List} ->
+            Values = [{described, Descriptor, Value} || Value <- List],
+            % this format cannot represent an empty array of described types
+            {array, {described, Descriptor, Type}, Values};
+        {as_is, _, _} ->
+            exit({array_of_described_zero_width_elements_unsupported, Count})
+    end;
+parse_array1(UnitSize, Count, <<Type, ArrayBin/binary>>)
   when Type >= 16#40 andalso Type =< 16#45 ->
     %% This is an array that must have zero octets of data.
-    if byte_size(ArrayBin) > 0 ->
-           exit({failed_to_parse_array_extra_input_remaining, Type, byte_size(ArrayBin)});
-       Count > ?MAX_ZERO_WIDTH_ARRAY_COUNT ->
-           exit({failed_to_parse_array_count_exceeds_limit, Type, Count});
-       true ->
-           {Value, _} = parse_array_primitive(Type, <<>>),
-           {array, parse_constructor(Type), lists:duplicate(Count, Value)}
+    case byte_size(ArrayBin) of
+        0 ->
+            %% "Count zero-width elements" costs a handful of bytes on the
+            %% wire no matter how large Count is. Since RabbitMQ has no need to
+            %% interpret this special type of array and to protect against CWE-770,
+            %% instead of materialized as Count terms, keep it as an opaque,
+            %% constant-size value.
+            case UnitSize of
+                8 -> {as_is, 16#e0, <<2:8, Count:8, Type>>};
+                32 -> {as_is, 16#f0, <<5:32, Count:32, Type>>}
+            end;
+        Size ->
+            exit({failed_to_parse_array_extra_input_remaining, Type, Size})
     end;
-parse_array1(Count, <<Type, ArrayBin/binary>>)
+parse_array1(_UnitSize, Count, <<Type, ArrayBin/binary>>)
   when Count > byte_size(ArrayBin) ->
     exit({failed_to_parse_array_count_exceeds_input, Type, Count, byte_size(ArrayBin)});
-parse_array1(Count, <<Type, ArrayBin/binary>>) ->
+parse_array1(_UnitSize, Count, <<Type, ArrayBin/binary>>) ->
     parse_array2(Count, Type, ArrayBin, []).
 
 parse_array2(0, Type, <<>>, Acc) ->
diff --git a/deps/amqp10_common/test/binary_parser_SUITE.erl b/deps/amqp10_common/test/binary_parser_SUITE.erl
@@ -23,6 +23,7 @@ all_tests() ->
      array_with_extra_input,
      array32_count_exceeds_data,
      array_of_zero_width_elements,
+     array_of_described_zero_width_elements_unsupported,
      unsupported_type,
      peek_described_section_total_sizes,
      peek_described_section,
@@ -53,8 +54,10 @@ roundtrip(_Config) ->
               {binary, <<"outer value">>}},
              {array, ubyte, [{ubyte, 1}, {ubyte, 255}]},
              {array, boolean, [true, false, true]},
-             {array, null, [null, null, null]},
-             {array, null, []},
+             %% Zero-width array elements (e.g. null repeated N times) are
+             %% represented opaquely rather than expanded into N terms.
+             {as_is, 16#f0, <<5:32, 3:32, 16#40>>},
+             {as_is, 16#f0, <<5:32, 0:32, 16#40>>},
              true,
              {list, [{utf8, <<"hi">>},
                      {described,
@@ -90,42 +93,72 @@ array_with_extra_input(_Config) ->
     ?assertExit(Expected, amqp10_binary_parser:parse_many(Bin, [])).
 
 array32_count_exceeds_data(_Config) ->
-    Count = 16#FFFFFFFF,
-    Type = 16#45,
-    ArrayPayload = <<Count:32, Type>>,
-    Size = byte_size(ArrayPayload),
-    Bin = <<16#f0, Size:32, ArrayPayload/binary>>,
-    ?assertExit(
-       {failed_to_parse_array_count_exceeds_limit, Type, Count},
-       amqp10_binary_parser:parse(Bin)),
-    %% Also verify smaller mismatches are caught: 10 elements but only 3 data bytes
-    SmallType = 16#50,
-    SmallPayload = <<10:32, SmallType, 1, 2, 3>>,
-    SmallSize = byte_size(SmallPayload),
-    SmallBin = <<16#f0, SmallSize:32, SmallPayload/binary>>,
-    ?assertExit(
-       {failed_to_parse_array_count_exceeds_input, SmallType, 10, 3},
-       amqp10_binary_parser:parse(SmallBin)).
-
+    TypeArray32 = 16#f0,
+    TypeUByte = 16#50,
+    %% 10 elements declared but only 3 data bytes available.
+    Count = 10,
+    Payload = <<Count:32, TypeUByte, 1, 2, 3>>,
+    Bin = <<TypeArray32, (byte_size(Payload)):32, Payload/binary>>,
+    ?assertExit({failed_to_parse_array_count_exceeds_input, TypeUByte, Count, 3},
+                amqp10_binary_parser:parse(Bin)).
+
+%% Zero-width array elements (null, booleans, uint0, ulong0, list0) cost zero
+%% octets on the wire no matter how large Count is. Rather than materializing
+%% Count terms (CWE-770: a small frame could otherwise amplify into gigabytes
+%% of heap), the parser represents such an array as an opaque, constant-size value.
 array_of_zero_width_elements(_Config) ->
-    %% Array8 (0xe0), Size=2, Count=10, Null Constructor=0x40 (width 0)
-    {Parsed40, 4} = amqp10_binary_parser:parse(<<16#e0, 2, 10, 16#40>>),
-    ?assertEqual({array, null, lists:duplicate(10, null)}, Parsed40),
-
-    {Parsed41, 4} = amqp10_binary_parser:parse(<<16#e0, 2, 10, 16#41>>),
-    ?assertEqual({array, boolean, lists:duplicate(10, true)}, Parsed41),
-
-    {Parsed42, 4} = amqp10_binary_parser:parse(<<16#e0, 2, 10, 16#42>>),
-    ?assertEqual({array, boolean, lists:duplicate(10, false)}, Parsed42),
-
-    {Parsed43, 4} = amqp10_binary_parser:parse(<<16#e0, 2, 10, 16#43>>),
-    ?assertEqual({array, uint, lists:duplicate(10, {uint, 0})}, Parsed43),
-
-    {Parsed44, 4} = amqp10_binary_parser:parse(<<16#e0, 2, 10, 16#44>>),
-    ?assertEqual({array, ulong, lists:duplicate(10, {ulong, 0})}, Parsed44),
-
-    {Parsed45, 4} = amqp10_binary_parser:parse(<<16#e0, 2, 10, 16#45>>),
-    ?assertEqual({array, list, lists:duplicate(10, {list, []})}, Parsed45).
+    TypeArray32 = 16#f0,
+    TypeArray8 = 16#e0,
+
+    Check32 = fun(Type, Count) ->
+                      Encoded = <<5:32, Count:32, Type>>,
+                      Bin = <<TypeArray32, Encoded/binary>>,
+                      {Parsed, ParsedSize} = amqp10_binary_parser:parse(Bin),
+                      ?assertEqual({as_is, TypeArray32, Encoded}, Parsed),
+                      ?assertEqual(byte_size(Bin), ParsedSize),
+                      %% Re-encoding reproduces the exact original bytes.
+                      ?assertEqual(Bin, iolist_to_binary(amqp10_binary_generator:generate(Parsed)))
+              end,
+    [Check32(Type, 10) || Type <- [16#40, 16#41, 16#42, 16#43, 16#44, 16#45]],
+
+    %% No limit on Count: an absurd 32-bit count is represented exactly as cheaply as a small one.
+    Check32(16#40, 16#FFFFFFFF),
+
+    %% Array8 form (0xe0) is parsed just as cheaply, and the selector is
+    %% preserved rather than normalized to array32: re-encoding reproduces
+    %% the exact original 4 bytes, not an inflated 10-byte array32 form.
+    Check8 = fun(Type, Count) ->
+                     Encoded = <<2, Count, Type>>,
+                     Bin = <<TypeArray8, Encoded/binary>>,
+                     {Parsed, ParsedSize} = amqp10_binary_parser:parse(Bin),
+                     ?assertEqual({as_is, TypeArray8, Encoded}, Parsed),
+                     ?assertEqual(byte_size(Bin), ParsedSize),
+                     ?assertEqual(Bin, iolist_to_binary(amqp10_binary_generator:generate(Parsed)))
+             end,
+    [Check8(Type, 10) || Type <- [16#40, 16#41, 16#42, 16#43, 16#44, 16#45]],
+    Check8(16#40, 255),
+
+    %% Nested as an element of an outer array of arrays, the preserved array8
+    %% selector must be upgraded to array32 framing: RabbitMQ's generator
+    %% always uses the large (32-bit) form for nested array elements (see
+    %% constructor/1: "use large array type for all nested arrays").
+    {Inner8, _} = amqp10_binary_parser:parse(<<TypeArray8, 2:8, 10:8, 16#40>>),
+    OuterBin = iolist_to_binary(amqp10_binary_generator:generate({array, array, [Inner8]})),
+    {{array, array, [InnerParsedBack]}, _} = amqp10_binary_parser:parse(OuterBin),
+    ?assertEqual({as_is, TypeArray32, <<5:32, 10:32, 16#40>>}, InnerParsedBack).
+
+%% An array of described zero-width elements (e.g. a described null repeated
+%% Count times) carries no information beyond its count and descriptor. This
+%% combination isn't supported: it is rejected cleanly instead of being
+%% expanded (unbounded memory) or given a bespoke opaque representation.
+array_of_described_zero_width_elements_unsupported(_Config) ->
+    %% ?DESCRIBED, utf8 "URL" descriptor, null (zero-width) element type.
+    DescribedNull = <<0, 16#a1, 3, "URL", 16#40>>,
+    Count = 16#FFFFFFFF,
+    CountAndV = <<Count:32, DescribedNull/binary>>,
+    Bin = <<16#f0, (byte_size(CountAndV)):32, CountAndV/binary>>,
+    ?assertExit({array_of_described_zero_width_elements_unsupported, Count},
+                amqp10_binary_parser:parse(Bin)).
 
 unsupported_type(_Config) ->
     UnsupportedType = 16#02,
diff --git a/deps/amqp10_common/test/prop_SUITE.erl b/deps/amqp10_common/test/prop_SUITE.erl
@@ -336,7 +336,10 @@ fields() ->
               )}.
 
 amqp_array() ->
-    Gens = fixed_and_variable_width_types(),
+    %% Excludes amqp_null(): an array of null is encoded with the zero-width
+    %% null constructor (0x40), which the parser represents opaquely as
+    %% {as_is, ...} rather than as {array, null, List}.
+    Gens = fixed_and_variable_width_types() -- [amqp_null()],
     ?LET(N,
          integer(1, length(Gens)),
          begin
diff --git a/deps/rabbit/test/amqp_client_SUITE.erl b/deps/rabbit/test/amqp_client_SUITE.erl
@@ -7194,12 +7194,16 @@ decimal_types(Config) ->
     Decimal128Zero = <<16#22, 16#08, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>>,
     Decimal3242 = <<16#22, 16#50, 16#00, 16#2A>>, % 42
     Decimal32NaN = <<16#7C, 0, 0, 0>>,
+    %% array8 of 5 nulls: size=2, count=5, null element constructor (0x40).
+    NullArray5 = <<2, 5, 16#40>>,
     Body = #'v1_0.amqp_value'{content = {list, [{as_is, 16#74, Decimal32Zero},
                                                 {as_is, 16#84, Decimal64Zero},
-                                                {as_is, 16#94, Decimal128Zero}]}},
+                                                {as_is, 16#94, Decimal128Zero},
+                                                {as_is, 16#e0, NullArray5}]}},
     MsgAnns = #{<<"x-decimal-32">> => {as_is, 16#74, Decimal3242},
                 <<"x-decimal-64">> => {as_is, 16#84, Decimal64Zero},
                 <<"x-decimal-128">> => {as_is, 16#94, Decimal128Zero},
+                <<"x-null-array">> => {as_is, 16#e0, NullArray5},
                 <<"x-list">> => {list, [{as_is, 16#94, Decimal128Zero}]},
                 <<"x-map">> => {map, [{{utf8, <<"key-1">>},
                                        {as_is, 16#94, Decimal128Zero}}]}},
@@ -7223,6 +7227,7 @@ decimal_types(Config) ->
     ?assertMatch(#{<<"x-decimal-32">> := {as_is, 16#74, Decimal3242},
                    <<"x-decimal-64">> := {as_is, 16#84, Decimal64Zero},
                    <<"x-decimal-128">> := {as_is, 16#94, Decimal128Zero},
+                   <<"x-null-array">> := {as_is, 16#e0, NullArray5},
                    <<"x-list">> := [{as_is, 16#94, Decimal128Zero}],
                    <<"x-map">> := [{{utf8, <<"key-1">>},
                                     {as_is, 16#94, Decimal128Zero}}]},
