search

espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
603 stars 255 forks source link

Remaining length is not calculated according to the MQTT 3.1 standard #96

Closed Thomseeen closed 5 years ago

Thomseeen commented 5 years ago

Seems like there is an issue with the remaining length in the messages header. Sending a message with 16kB data and a topic length of 11B causes the header to be

0x30, // Publish, no DUP, QoS 0, no Retain
0x8D, 0x7D, // remaining length of 16013B
0x00, 0x0B, // topic length 11B
topic...

which works fine and is right according to MQTT 3.1. Sending a message with 17kB data and a topic length of 11B causes the header to be

0x30, // Publish, no DUP, QoS 0, no Retain
0xF5, 0x84, // ... this is wrong. Beginning with the LSB (0x84) there is a continuation bit
               which would indicate a 3B long remaining length field. So the following 0x00 gets 
               included into the remaining length field too. As a result the topic length field ix 0x0B 
               and the first byte of the topic. That causes a wrong topic length and unreadable 
               MQTT package. The right remaining length would be 3B (0xF5, 0x84, 0x01).
0x00, 0x0B, // topic length 11B
topic...

The library only seems to distinguish between a remaining length field with 1 or 2 bytes while the standard describes a field length of up to 4 bytes.

david-cermak commented 5 years ago

Hi @Thomseeen

Indeed there was an issue with publishing longer messages, but it has been resolved in commit https://github.com/espressif/esp-mqtt/commit/e0bbbebc08884118931f7adaedb0b8cba06db00c (from Oct 2018).

Current version shall support up to 4 bytes len, just tested sending a 50k message:

0x30,  -> flags
0xdf, 0x86, 0x03 -> size 50015
0x00, 0x0d -> topic len 13

which looks correct

Thomseeen commented 5 years ago

I see. I'm stuck with IDF 3.1 so I have to use https://github.com/espressif/esp-mqtt/tree/ESP-MQTT_FOR_IDF_3.1 which still includes the error. Sorry to bother.