Update MQTTSNPacket.c - Githubissues

davegish commented 4 years ago

Looks like a bug in MQTTSNPacket_len(). If the input parameter is 255, it will return 256, but the actual packet size will be 258.

martinkirsche commented 4 years ago

There is already a pull request containing this fix at https://github.com/eclipse/paho.mqtt-sn.embedded-c/pull/187 but unfortunately the maintainer reverted it within https://github.com/eclipse/paho.mqtt-sn.embedded-c/pull/189

ty4tw commented 4 years ago

Hi,

Could you explain why the actual length is 258? According to the spec,

Messages with lengths smaller than 256 octets may use the shorter 1-octet format.

martinkirsche commented 4 years ago

Could you explain why the actual length is 258?

The length will become 258 because ...

the output of the function MQTTSNPacket_len is fet into MQTTSNPacket_encode. So if I put 255 as the length of the packet without the length field into MQTTSNPacket_len it will currently return 256. But MQTTSNPacket_encode will use a 3-octed length field for such a message. The result is a MQTT-SN message that is longer (258 bytes) than its length field specifies (256 bytes).

See #187 for a demonstration of the effect.

ty4tw commented 4 years ago

Hi Martin, It was a bug of GatewayTester and fixed. MQTTSNPacket_encode() is correct.

/**
 * Encodes the MQTT-SN message length
 * @param buf the buffer into which the encoded data is written
 * @param length the length to be encoded
 * @return the number of bytes written to the buffer
 */
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 specification defines the message length as follows:

5.2.1 Length The Length field is either 1- or 3-octet long and specifies the total number of octets contained in the message (including the Length field itself). If the first octet of the Length field is coded “0x01” then the Length field is 3-octet long; in this case, the two following octets specify the total number of octets of the message (most-significant octet first). Otherwise, the Length field is only 1-octet long and specifies itself the total number of octets contained in the message. The 3-octet format allows the encoding of message lengths up to 65535 octets. Messages with lengths smaller than 256 octets may use the shorter 1-octet format. Note that because MQTT-SN does not support message fragmentation and reassembly, the maximum message length that could be used in a network is governed by the maximum packet size that is supported by that network, and not by the maximum length that could be encoded by MQTT-SN.

davegish commented 4 years ago

Hi Tomoaki,

As Martin said, the bug is in MQTTSNPacket_len(), not MQTTSNPacket_encode().

/**

Calculates the full packet length including length field
@param length the length of the MQTT-SN packet without the length field
@return the total length of the MQTT-SN packet including the length field */ int MQTTSNPacket_len(int length) { return (length > 255) ? length + 3 : length + 1; } If the input length parameter value is 255, MQTTSNPacket_len() will return 256. In this case, the length field in the MQTTSN packet will be set to 256 but MQTTSNPacket_encode() will encode 258 bytes. So this is the bug.

To fix it, you could either use: return (length >= 255) ? length + 3 : length + 1; or return (length > 254) ? length + 3 : length + 1;

Hope this helps, Dave

--

Dave Gish

Principal Software Architect

The Nielsen Company

Skype: dave.gish

973-831-8208

On Sun, Apr 12, 2020 at 8:34 PM Tomoaki Yamaguchi notifications@github.com wrote:

Hi Martin, It was a bug of GatewayTester and fixed. MQTTSNPacket_encode() is correct.

/**

Encodes the MQTT-SN message length

@param buf the buffer into which the encoded data is written

@param length the length to be encoded

@return the number of bytes written to the buffer

*/

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 specification defines the message length as follows:

5.2.1 Length The Length field is either 1- or 3-octet long and specifies the total number of octets contained in the message (including the Length field itself). If the first octet of the Length field is coded “0x01” then the Length field is 3-octet long; in this case, the two following octets specify the total number of octets of the message (most-significant octet first). Otherwise, the Length field is only 1-octet long and specifies itself the total number of octets contained in the message. The 3-octet format allows the encoding of message lengths up to 65535 octets. Messages with lengths smaller than 256 octets may use the shorter 1-octet format. Note that because MQTT-SN does not support message fragmentation and reassembly, the maximum message length that could be used in a network is governed by the maximum packet size that is supported by that network, and not by the maximum length that could be encoded by MQTT-SN.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/eclipse/paho.mqtt-sn.embedded-c/pull/192#issuecomment-612699628, or unsubscribe https://github.com/notifications/unsubscribe-auth/APEPUBU7XU2OHK44BK54U43RMJMXZANCNFSM4MF7W5TQ .

ty4tw commented 4 years ago

Hi davegish, you are misunderstanding the length parameter. @param length the length of the MQTT-SN packet without the length field int MQTTSNPacket_len(int length); int MQTTSNPacket_encode(unsigned char* buf, int length) when the length is 255 MQTTSNPacket_len returns 256 and MQTTSNPacket_encode returns 1. Nothing is wrong. see the definition of the message length in the specification.

return (length >*=* 255) ? length + 3 : length + 1;

This returns 258 when the length is 255.

davegish commented 4 years ago

Hi Tomoaki,

I don't see how I'm misunderstanding it.

According to the spec: 5.2.1 Length The Length field is either 1- or 3-octet long and specifies the total number of octets contained in the message (including the Length field itself).

The number 256 is impossible to carry as 1 byte, so the length field expands to 3 bytes. In other words, according to the spec, it doesn't seem possible to have a packet size of 256 or 257 bytes.

The input parameter to MQTTSNPacket_len() is: @param https://github.com/param length the length of the MQTT-SN packet without the length field

The return value of MQTTSNPacket_len() is: @return the total length of the MQTT-SN packet including the length field

If the MQTT-SN packet without the length field is 255 bytes, then the total length of the MQTT-SN packet including the length field will be 258 bytes.

--

Dave Gish

Principal Software Architect

The Nielsen Company

Skype: dave.gish

973-831-8208

On Mon, Apr 13, 2020 at 4:47 AM Tomoaki Yamaguchi notifications@github.com wrote:

Hi davegish, you are misunderstanding the length parameter. @param https://github.com/param length the length of the MQTT-SN packet without the length field int MQTTSNPacket_len(int length); int MQTTSNPacket_encode(unsigned char* buf, int length) when the length is 255 functions return 256. Nothing is wrong. see the definition of the message length of the specification.

return (length >= 255) ? length + 3 : length + 1;
This will return 258  when the length is 255.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/eclipse/paho.mqtt-sn.embedded-c/pull/192#issuecomment-612812414>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APEPUBWARRDN6HGKJKCO6MTRMLGT5ANCNFSM4MF7W5TQ>
.

davegish commented 4 years ago

Tomoaki: Does this mean a message with length 255 octet may use the shorter 1 octet format ... Dave: Yes.

Tomoaki: ... and the total length is 255 + 1 = 256 ? Dave: No. According to the spec, the length value in the header is the total length of the packet, including the length field itself. 5.2.1 Length The Length field is either 1- or 3-octet long and specifies the total number of octets contained in the message (including the Length field itself).

Examples: 1) if the payload length before adding the length field is 254 octets, the total length is 255, which may use the shorter 1 octet length field. In this case, the value of the length field in the header is 255.

2) if the payload length before adding the length field is 255 octets, the total length is over 255, which requires the 3-octet length field. In this case, the value of the length field in the header is 258.

--

Dave Gish

Principal Software Architect

The Nielsen Company

Skype: dave.gish

973-831-8208

On Mon, Apr 13, 2020 at 9:20 AM Tomoaki Yamaguchi notifications@github.com wrote:

Messages with lengths smaller than 256 octets may use the shorter 1-octet format.

Dose this means message with length 255 octet may use the shorter 1 octet format and total length is 255 + 1 = 256 ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/eclipse/paho.mqtt-sn.embedded-c/pull/192#issuecomment-612894900, or unsubscribe https://github.com/notifications/unsubscribe-auth/APEPUBXIXVSP6W7IPJESJLLRMMGR5ANCNFSM4MF7W5TQ .

ty4tw commented 4 years ago

Hi, Ok I understood. I will fix the bugs. Thank you for your cooperation.

saumilsdk commented 4 years ago

hi @ty4tw is that bug fixed in latest release? There are multiple PRs getting rejected and those fixes we are missing in our implementation. Can you please also apply those fixes and release a new version?

eclipse / paho.mqtt-sn.embedded-c

Update MQTTSNPacket.c #192