search

espressif / esp-mqtt

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

MQTT: Packet Identifier may be conflict (IDFGH-4243) #176

Closed AxelLin closed 3 years ago

AxelLin commented 3 years ago

The MQTT document says: SUBSCRIBE, UNSUBSCRIBE, and PUBLISH (in cases where QoS > 0) Control Packets MUST contain a non-zero 16-bit Packet Identifier [MQTT-2.3.1-1]. Each time a Client sends a new packet of one of these types it MUST assign it a currently unused Packet Identifier [MQTT-2.3.1-2]. If a Client re-sends a particular Control Packet, then it MUST use the same Packet Identifier in subsequent re-sends of that packet. The Packet Identifier becomes available for reuse after the Client has processed the corresponding acknowledgement packet. In the case of a QoS 1 PUBLISH this is the corresponding PUBACK; in the case of QoS 2 it is PUBCOMP. For SUBSCRIBE or UNSUBSCRIBE it is the corresponding SUBACK or UNSUBACK [MQTT-2.3.1-3]. The same conditions apply to a Server when it sends a PUBLISH with QoS > 0 [MQTT-2.3.1-4].

In current mqtt implementation for QoS1/2, it generates the message_id by platform_random(). However, it does not check if the generated message_id is conflict with the id in oubox. So it's possible to have conflict message_id.

The mqtt spec does not say the Packet Identifier needs to be random, so it's probably easier just use an incremental serial number for id (1 ~ 65535) . (Otherwise, it needs to check all the used message_id in outbox to avoid conflict).

umer-ilyas commented 3 years ago

incremental serial number for id (1 ~ 65535) should be ok

david-cermak commented 3 years ago

Thanks for bringing this to our attention! I also think the incremental id should be okay and seen it used on other mqtt libs as well. We will have to do something about this.