espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
610 stars 257 forks source link

Can't stop / destroy mqtt client that fails to connect to the broker (IDFGH-1387) #120

Closed alexbarteau-temboo closed 5 years ago

alexbarteau-temboo commented 5 years ago

It is critical that we can shutdown and cleanup failed mqtt connections. Attempting to do so now crashes the application.

If the esp_mqtt_client_config_t passed to esp_mqtt_client_init() is invalid the client cannot be shutdown/destroyed. This was tested with an invalid uri and cert_pem, but likely extends to port and all other parameters used to connect to broker.

Attempting to shutdown a connection that did not reach the "MQTT_EVENT_CONNECTED" state results in a panic as the "disconnect" message can't be sent. The backtrace is below. Additionally, you cannot "destroy" the client because that calls the esp_mqtt_client_stop.

Trace on stop call:

0x400dbbc7: fini_message at <idf path>/components/mqtt/esp-mqtt/lib/mqtt_msg.c:141

0x400dc0b1: mqtt_msg_disconnect at <idf path>/components/mqtt/esp-mqtt/lib/mqtt_msg.c:549

0x400db3a4: esp_mqtt_client_stop at <idf path>/components/mqtt/esp-mqtt/mqtt_client.c:1121 (discriminator 3)

Commenting out the call to stop in esp_mqtt_client_destroy results in a different panic as the destroy method appears to be attempting to free an uninitialized memory reference.

0x400888e8: invoke_abort at <idf path>/components/esp32/panic.c:156

0x40088b3d: abort at <idf path>/components/esp32/panic.c:171

0x4008bcb2: multi_heap_assert at <idf path>/components/heap/multi_heap_platform.h:54
 (inlined by) get_prev_free_block at <idf path>/components/heap/multi_heap.c:187

0x4008c1fd: multi_heap_free_impl at <idf path>/components/heap/multi_heap.c:484

0x400831f2: heap_caps_free at <idf path>/components/heap/heap_caps.c:268

0x4008860d: free at <idf path>/components/newlib/heap.c:47

0x400db220: esp_mqtt_client_destroy at <idf path>/components/mqtt/esp-mqtt/mqtt_client.c:459
david-cermak commented 5 years ago

Thanks for the report @alexbarteau-temboo.

we've already had a PR addressing this issue -- just been merged. This should resolve the crash your describe (please reopen otherwise) Thanks!

alexbarteau-temboo commented 5 years ago

@david-cermak It appears the repository reference in the esp-idf repository is old. It is still linking to the old 11f8846 commit. Manually checking out the master branch of the esp-mqtt submodule does fix this issue.

When can we expect esp-idf to reference a commit with this fix?

david-cermak commented 5 years ago

This usually takes some time until changes propagate to IDF, sorry cannot give any estimate. There's a corresponding IDF issue https://github.com/espressif/esp-idf/issues/3619, which would close once merged to IDF master (so you can subscribe to get notified)

Subhashbk commented 5 years ago

Any updates on this issue? I don't see fix for this issue is updated to master branch??