espressif / esp-protocols

Collection of ESP-IDF components related to networking protocols
181 stars 126 forks source link

[mqtt_cxx] Race condition in idf::mqtt::Client constructor (IDFGH-13475) #631

Open snake-4 opened 1 month ago

snake-4 commented 1 month ago

Answers checklist.

General issue report

There is a race condition in the constructor of the Client class, where the event handler may be invoked before subclass of Client is done constructing. This results in the program terminating if the dispatched event is MQTT_EVENT_CONNECTED or MQTT_EVENT_DATA since on_data and on_connected are pure virtual functions.

Pure virtual functions cannot be called before the object is fully constructed, otherwise a compiler generated termination function will be called.

The exact line is below: https://github.com/espressif/esp-protocols/blob/e425a3c504e308ada7e8f594239c24ecb033d3b6/components/esp_mqtt_cxx/esp_mqtt_cxx.cpp#L166

This cannot be solved by making the aforementioned functions normal virtual functions, as that would result in an undelivered event instead. A lock in the constructor would also not work as the base class can't know when the subclass is done constructing. Preferably the MQTT client should have a separate function for starting the underlying client and this should not be done in the constructor.

We discovered this while integration testing our code on the Linux target with a MQTT broker on the localhost. The connection is made so fast that the subclass doesn't have time to finish constructing before an MQTT_EVENT_CONNECTED event is dispatched.