tuanpmt / esp_mqtt

MQTT client library for ESP8266
http://tuanpm.net/post/esp_mqtt/
MIT License
1.15k stars 401 forks source link

Ring Buffer overflow causes corrupt data which leads to buffer reset #134

Closed asadislam94 closed 11 months ago

asadislam94 commented 7 years ago

I was deliberately overflowing the message buffer and I saw that in this scenario, The buffer resets after a certain number of overflows occur. I am queuing 50 messages of total packet length 37. Then I queue 3 packets of length 49,48 and 43 respectively. This is all done while making sure no message is sent during this queuing process.

I have found the cause of the problem. while (QUEUE_Puts(&client->msgQueue, client->mqtt_state.outbound_message->data, client->mqtt_state.outbound_message->length) == -1) { MQTT_INFO("MQTT: Queue full\r\n"); if (QUEUE_Gets(&client->msgQueue, dataBuffer, &dataLen, MQTT_BUF_SIZE) == -1) { MQTT_INFO("MQTT: Serious buffer error\r\n"); return FALSE; } } When you put something in queue, the underlying code starts saving data without checking if there is enough space. When the buffer fill count reaches maximum, it just returns '-1' without deleting the partially saved data. This does not cause problem for the initial few overflows, but when the read pointer of the buffer reaches this corrupt data, it seems to clear the whole buffer. I have changed the code to account for this by comparing the message size to available space before trying to call the Queue_PUT function. while ((client->msgQueue.rb.size-client->msgQueue.rb.fill_cnt)<client->mqtt_state.outbound_message->length+2) { MQTT_INFO("MQTT: Queue full\r\n"); if (QUEUE_Gets(&client->msgQueue, dataBuffer, &dataLen, MQTT_BUF_SIZE) == -1) { MQTT_INFO("MQTT: Serious buffer error\r\n"); return FALSE; } } QUEUE_Puts(&client->msgQueue, client->mqtt_state.outbound_message->data, client->mqtt_state.outbound_message->length);

The reason for adding 2 to the message length is that buffer requires 2 extra spaces to mark start and end.

archenroot commented 6 years ago

Any progress on this?

asadislam94 commented 6 years ago

I think this should be fixed with this pull #126 However I haven't tested that yet.