diff --git a/apps/emqx/src/emqx_frame.erl b/apps/emqx/src/emqx_frame.erl index c5ce7dc8f..c8283f594 100644 --- a/apps/emqx/src/emqx_frame.erl +++ b/apps/emqx/src/emqx_frame.erl @@ -330,10 +330,14 @@ parse_connect2( <<>> -> ConnPacket1#mqtt_packet_connect{username = Username, password = Password}; _ -> - ?PARSE_ERR(malformed_connect_data) + ?PARSE_ERR(#{ + hint => malformed_connect, + unexpected_trailing_bytes => size(Rest7) + }) end; -parse_connect2(_ProtoName, _, _) -> - ?PARSE_ERR(malformed_connect_header). +parse_connect2(_ProtoName, Bin, _StrictMode) -> + %% sent less than 32 bytes + ?PARSE_ERR(#{hint => malformed_connect, header_bytes => Bin}). parse_packet( #mqtt_packet_header{type = ?CONNECT}, @@ -486,7 +490,7 @@ parse_will_message( ) -> {Props, Rest} = parse_properties(Bin, Ver, StrictMode), {Topic, Rest1} = parse_utf8_string_with_hint(Rest, StrictMode, invalid_topic), - {Payload, Rest2} = parse_binary_data(Rest1), + {Payload, Rest2} = parse_will_payload(Rest1), { Packet#mqtt_packet_connect{ will_props = Props, @@ -687,20 +691,24 @@ parse_utf8_string(Bin, _) when -> ?PARSE_ERR(#{reason => malformed_utf8_string_length}). -parse_binary_data(<>) -> +parse_will_payload(<>) -> {Data, Rest}; -parse_binary_data(<>) when +parse_will_payload(<>) when Len > byte_size(Rest) -> ?PARSE_ERR(#{ - hint => malformed_binary_data, + hint => malformed_will_payload, parsed_length => Len, - remaining_bytes_length => byte_size(Rest) + remaining_bytes => byte_size(Rest) }); -parse_binary_data(Bin) when +parse_will_payload(Bin) when 2 > byte_size(Bin) -> - ?PARSE_ERR(malformed_binary_data_length). + ?PARSE_ERR(#{ + hint => malformed_will_payload, + length_bytes => size(Bin), + expected_bytes => 2 + }). %%-------------------------------------------------------------------- %% Serialize MQTT Packet diff --git a/apps/emqx/test/emqx_frame_SUITE.erl b/apps/emqx/test/emqx_frame_SUITE.erl index 935a0d8ec..52c513985 100644 --- a/apps/emqx/test/emqx_frame_SUITE.erl +++ b/apps/emqx/test/emqx_frame_SUITE.erl @@ -57,11 +57,12 @@ groups() -> t_serialize_parse_v5_connect, t_serialize_parse_connect_without_clientid, t_serialize_parse_connect_with_will, + t_serialize_parse_connect_with_malformed_will, t_serialize_parse_bridge_connect, t_parse_invalid_remaining_len, t_parse_malformed_properties, t_malformed_connect_header, - t_malformed_connect_payload, + t_malformed_connect_data, t_reserved_connect_flag, t_invalid_clientid ]}, @@ -277,6 +278,37 @@ t_serialize_parse_connect_with_will(_) -> ?assertEqual(Bin, serialize_to_binary(Packet)), ?assertMatch({ok, Packet, <<>>, _}, emqx_frame:parse(Bin)). +t_serialize_parse_connect_with_malformed_will(_) -> + Packet2 = #mqtt_packet{ + header = #mqtt_packet_header{type = ?CONNECT}, + variable = #mqtt_packet_connect{ + proto_ver = ?MQTT_PROTO_V3, + proto_name = <<"MQIsdp">>, + clientid = <<"mosqpub/10452-iMac.loca">>, + clean_start = true, + keepalive = 60, + will_retain = false, + will_qos = ?QOS_1, + will_flag = true, + will_topic = <<"/will">>, + will_payload = <<>> + } + }, + <<16, 46, Body:44/binary, 0, 0>> = serialize_to_binary(Packet2), + %% too short + BadBin1 = <<16, 45, Body/binary, 0>>, + ?ASSERT_FRAME_THROW( + #{hint := malformed_will_payload, length_bytes := 1, expected_bytes := 2}, + emqx_frame:parse(BadBin1) + ), + %% too long + BadBin2 = <<16, 47, Body/binary, 0, 2, 0>>, + ?ASSERT_FRAME_THROW( + #{hint := malformed_will_payload, parsed_length := 2, remaining_bytes := 1}, + emqx_frame:parse(BadBin2) + ), + ok. + t_serialize_parse_bridge_connect(_) -> Bin = <<16, 86, 0, 6, 77, 81, 73, 115, 100, 112, 131, 44, 0, 60, 0, 19, 67, 95, 48, 48, 58, 48, @@ -643,16 +675,14 @@ t_parse_malformed_properties(_) -> ). t_malformed_connect_header(_) -> - ?assertException( - throw, - {frame_parse_error, malformed_connect_header}, + ?ASSERT_FRAME_THROW( + #{hint := malformed_connect, header_bytes := _}, emqx_frame:parse(<<16, 11, 0, 6, 77, 81, 73, 115, 100, 112, 3, 130, 1, 6>>) ). -t_malformed_connect_payload(_) -> - ?assertException( - throw, - {frame_parse_error, malformed_connect_data}, +t_malformed_connect_data(_) -> + ?ASSERT_FRAME_THROW( + #{hint := malformed_connect, unexpected_trailing_bytes := _}, emqx_frame:parse(<<16, 15, 0, 6, 77, 81, 73, 115, 100, 112, 3, 0, 0, 0, 0, 0, 0>>) ).