Wrong calculation of message length in MQTTSNPacket_len()

Hi,

I ran in an issue regarding the calculation of the paket length and could not find an open ticket to that so I opened a new one. Please correct me if I'm mistaken or overlooked something, but I think the calculation of the message length at the MQTT-SN implementation for embedded systems is currently wrong.

The current implementation (https://github.com/eclipse/paho.mqtt-sn.embedded-c/blob/master/MQTTSNPacket/src/MQTTSNPacket.c) states:

int MQTTSNPacket_len(int length)
{
    return (length > 255) ? length + 3 : length + 1;
}

This function does not consider the field "length" itself in the condition. This leads to a problem, when exactly hitting the message length of 255 Byte (payload plus header without the length information), e. g. constructing a publish containing 249 Bytes of payload and using the current implementation (https://github.com/eclipse/paho.mqtt-sn.embedded-c/blob/master/MQTTSNPacket/src/MQTTSNSerializePublish.c) :

int MQTTSNSerialize_publish(unsigned char* buf, int buflen, unsigned char dup, int qos, unsigned char retained, unsigned short packetid,
        MQTTSN_topicid topic, unsigned char* payload, int payloadlen)
{
    unsigned char *ptr = buf;
    MQTTSNFlags flags;
    int len = 0;
    int rc = 0;

    FUNC_ENTRY;
    if ((len = MQTTSNPacket_len(MQTTSNSerialize_publishLength(payloadlen, topic, qos))) > buflen)
    {
        rc = MQTTSNPACKET_BUFFER_TOO_SHORT;
        goto exit;
    }
    ptr += MQTTSNPacket_encode(ptr, len); /* write length */
    writeChar(&ptr, MQTTSN_PUBLISH);      /* write message type */

       // ...
    return rc;
`}

The function MQTTSNSerialize_publishLength (https://github.com/eclipse/paho.mqtt-sn.embedded-c/blob/master/MQTTSNPacket/src/MQTTSNSerializePublish.c) adds 6 Byte to the total length message (here QoS < 3 to simplify the problem):

int MQTTSNSerialize_publishLength(int payloadlen, MQTTSN_topicid topic, int qos)
{
    int len = 6;

    if (topic.type == MQTTSN_TOPIC_TYPE_NORMAL && qos == 3)
        len += topic.data.long_.len;

    return payloadlen + len;
}

Therefore MQTTSNPacket_len is called with the parameter length equal to 255. The function returns the length of 256 Byte only considering one Byte for the message length. Now the function MQTTSNPacket_encode (https://github.com/eclipse/paho.mqtt-sn.embedded-c/blob/master/MQTTSNPacket/src/MQTTSNPacket.c) is called:

int MQTTSNPacket_encode(unsigned char* buf, int length)
{
    int rc = 0;

    FUNC_ENTRY;
    if (length > 255)
    {
        writeChar(&buf, 0x01);
        writeInt(&buf, length);
        rc += 3;
    }
    else
        buf[rc++] = length;

    FUNC_EXIT_RC(rc);
    return rc;
}

The length of 256 Byte is given to the function MQTTSNPacket_encode which inserts three bytes of length information (two more than decided in function MQTTSNPacket_len) at the start of the message. After finishing MQTTSNSerialize_publish we have a Message that has the length of 258 Byte, but the length information states that the message length is only 256 Byte.

Therefore I think the current implementation of MQTTSNPacket_len is incorrect.

In my oppinion the correct implementation for MQTTSNPacket_len() needs to add one byte to the length first and evaluate whether there needs to be added two further length bytes:

int MQTTSNPacket_len(int length)
{
    int len = length + 1;

    if (len > 255)
        len += 2

    return len;
}

Alternetively it can correct the condition to

int MQTTSNPacket_len(int length)
{
    return (length > 254) ? length + 3 : length + 1;
}

int MQTTSNPacket_len(int length)
{
    return (length >= 255) ? length + 3 : length + 1;
}

int MQTTSNPacket_len(int length)
{
    return ((length + 1) > 255) ? length + 3 : length + 1;
}

Please let me know soon if you can confirm my assumption or where I made a mistake.

best regards Johannes

eclipse / paho.mqtt-sn.embedded-c

Wrong calculation of message length in MQTTSNPacket_len() #210