espressif / esp-mqtt

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

MQTT 5 Bug - Checks packet header but there is none (IDFGH-9603) #255

Closed IvanSivius closed 1 year ago

IvanSivius commented 1 year ago

Hi, I am currently working on a project that uses mqtt for the transmission of data from a monitoring system. I found out that the library has a bug. When you want to send a packet with a length that exceeds the maximum length of mqtt established, the Buffer is fragmented and sent in pieces. In the funtion:

int esp_mqtt_client_publish(esp_mqtt_client_handle_t client, const char topic, const char data, int len, int qos, int retain)

you do:

   while (sending)  {
        if (mqtt_write_data(client) != ESP_OK) {}

This then goes in :

void esp_mqtt5_flow_control(esp_mqtt5_client_handle_t client){
  if (client->connect_info.protocol_ver == MQTT_PROTOCOL_V_5) {
      int msg_type = mqtt5_get_type(client->mqtt_state.outbound_message->data);
      if (msg_type == MQTT_MSG_TYPE_PUBLISH) {
          int msg_qos = mqtt5_get_qos(client->mqtt_state.outbound_message->data);
          if (msg_qos > 0) {
             client->send_publish_packet_count ++;
           ESP_LOGD(TAG, "Sent (%d) qos > 0 publish packet without ack", client->send_publish_packet_count)}}}}'

The first time you do this it is correct, you get the msg_type and msg_qos, but the second... we already entered the While (sending) in esp_mqtt_client_publish and then:

You copy the following buffer fragment in connection->buffer

Finally the bug:

         connection->message.data = connection->buffer;
        client->mqtt_state.outbound_message = &connection->message;

The problem here is that it then re-enters esp_mqtt5_flow_control but the header doesn't exist anymore because it's the next fragment with the payload only, causing the masks that want to get the msg_qos and the msg_type e to return wrong data. This then increments the counter which then triggers a connectivity error as if you wanted to try to push things with Qos1 or Qos2 in esp_mqtt5_client_publish_check()

From my point of view esp_mqtt5_flow_control() should not be called again after the first fragment.

This is the first time I upload a bug, sorry if it was not the place. Thanks.

ESP-YJM commented 1 year ago

@IvanSivius Thanks for reporting the issue. I will fix it as soon as possible.