Possible bug in mqttsn_packet_sender.c

[MarcoLG]

Hello,

I noticed that publishing mqttsn messages with a payload larger than ~255 bytes caused an error in the mqttsn library. I think the problem comes from this definition in mqttsn_packet_sender.c:

#define MQTTSN_PACKET_PUBLISH_LENGTH 7

This definition is used in mqttsn_packet_sender_publish() to allocate a buffer for the encoded packet of size payload_len + MQTTSN_PACKET_PUBLISH_LENGTH.
However, in the subsequent call to MQTTSNSerialize_publish() the following check is made:

if ((len = MQTTSNPacket_len(MQTTSNSerialize_publishLength(payloadlen, topic, qos))) > buflen)
{
    rc = MQTTSNPACKET_BUFFER_TOO_SHORT;
    goto exit;
}

In nominal conditions, MQTTSNSerialize_publishLength() returns payloadlen + 6, and MQTTSNPacket_len(x) returns x > 255 ? x + 3 : x + 1.
So, if payloadLen > 249 bytes, MQTTSNPacket_len will always return a value > buflen, causing the problem i've experienced.

I think the correct value for MQTTSN_PACKET_PUBLISH_LENGTH should be 9 instead of 7 to account for packets with size > 249 bytes.
I suspect the same problem affects other aspects of the library, but i've not looked for it yet.

Can you please confirm that this is an issue with nordic implementation of the mqtt-sn client and that my change does make sense?

Thanks

[Edvin]

Hello,

I received a reply from our Thread team today:

"I went through the code, and indeed, it seems like we have a bug in our MWTT-SN implementation for larger payload lengths. Because in such cases we don't include the additional payload length, the buffer allocated by nrf_malloc doesn't have a sufficient size. I don't see any side effects in the presented fix to increase the defined value from 7 to 9 since that will only result in allocating a larger buffer."

Best regards,

Edvin