Fix use of dangling pointers in esp-idf MQTT backend (esphome#4239)

fixes esphome/issues#3406
vidplace7 · Jan 11, 2023 · 351ea04 · 351ea04
1 parent 86a8e1f
commit 351ea04
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 19 deletions.
diff --git a/esphome/components/mqtt/mqtt_backend_idf.cpp b/esphome/components/mqtt/mqtt_backend_idf.cpp
@@ -69,7 +69,7 @@ void MQTTBackendIDF::loop() {
   }
 }
 
-void MQTTBackendIDF::mqtt_event_handler_(const esp_mqtt_event_t &event) {
+void MQTTBackendIDF::mqtt_event_handler_(const Event &event) {
   ESP_LOGV(TAG, "Event dispatched from event loop event_id=%d", event.event_id);
   switch (event.event_id) {
     case MQTT_EVENT_BEFORE_CONNECT:
@@ -104,28 +104,24 @@ void MQTTBackendIDF::mqtt_event_handler_(const esp_mqtt_event_t &event) {
       break;
     case MQTT_EVENT_DATA: {
       static std::string topic;
-      if (event.topic) {
-        // not 0 terminated - create a string from it
-        topic = std::string(event.topic, event.topic_len);
+      if (event.topic.length() > 0) {
+        topic = event.topic;
       }
       ESP_LOGV(TAG, "MQTT_EVENT_DATA %s", topic.c_str());
-      auto data_len = event.data_len;
-      if (data_len == 0)
-        data_len = strlen(event.data);
-      this->on_message_.call(event.topic ? const_cast<char *>(topic.c_str()) : nullptr, event.data, data_len,
+      this->on_message_.call(event.topic.length() > 0 ? topic.c_str() : nullptr, event.data.data(), event.data.size(),
                              event.current_data_offset, event.total_data_len);
     } break;
     case MQTT_EVENT_ERROR:
       ESP_LOGE(TAG, "MQTT_EVENT_ERROR");
-      if (event.error_handle->error_type == MQTT_ERROR_TYPE_TCP_TRANSPORT) {
-        ESP_LOGE(TAG, "Last error code reported from esp-tls: 0x%x", event.error_handle->esp_tls_last_esp_err);
-        ESP_LOGE(TAG, "Last tls stack error number: 0x%x", event.error_handle->esp_tls_stack_err);
-        ESP_LOGE(TAG, "Last captured errno : %d (%s)", event.error_handle->esp_transport_sock_errno,
-                 strerror(event.error_handle->esp_transport_sock_errno));
-      } else if (event.error_handle->error_type == MQTT_ERROR_TYPE_CONNECTION_REFUSED) {
-        ESP_LOGE(TAG, "Connection refused error: 0x%x", event.error_handle->connect_return_code);
+      if (event.error_handle.error_type == MQTT_ERROR_TYPE_TCP_TRANSPORT) {
+        ESP_LOGE(TAG, "Last error code reported from esp-tls: 0x%x", event.error_handle.esp_tls_last_esp_err);
+        ESP_LOGE(TAG, "Last tls stack error number: 0x%x", event.error_handle.esp_tls_stack_err);
+        ESP_LOGE(TAG, "Last captured errno : %d (%s)", event.error_handle.esp_transport_sock_errno,
+                 strerror(event.error_handle.esp_transport_sock_errno));
+      } else if (event.error_handle.error_type == MQTT_ERROR_TYPE_CONNECTION_REFUSED) {
+        ESP_LOGE(TAG, "Connection refused error: 0x%x", event.error_handle.connect_return_code);
       } else {
-        ESP_LOGE(TAG, "Unknown error type: 0x%x", event.error_handle->error_type);
+        ESP_LOGE(TAG, "Unknown error type: 0x%x", event.error_handle.error_type);
       }
       break;
     default:
@@ -140,7 +136,7 @@ void MQTTBackendIDF::mqtt_event_handler(void *handler_args, esp_event_base_t bas
   // queue event to decouple processing
   if (instance) {
     auto event = *static_cast<esp_mqtt_event_t *>(event_data);
-    instance->mqtt_events_.push(event);
+    instance->mqtt_events_.push(Event(event));
   }
 }
 

diff --git a/esphome/components/mqtt/mqtt_backend_idf.h b/esphome/components/mqtt/mqtt_backend_idf.h
@@ -12,6 +12,33 @@
 namespace esphome {
 namespace mqtt {
 
+struct Event {
+  esp_mqtt_event_id_t event_id;
+  std::vector<char> data;
+  int total_data_len;
+  int current_data_offset;
+  std::string topic;
+  int msg_id;
+  bool retain;
+  int qos;
+  bool dup;
+  esp_mqtt_error_codes_t error_handle;
+
+  // Construct from esp_mqtt_event_t
+  // Any pointer values that are unsafe to keep are converted to safe copies
+  Event(const esp_mqtt_event_t &event)
+      : event_id(event.event_id),
+        data(event.data, event.data + event.data_len),
+        total_data_len(event.total_data_len),
+        current_data_offset(event.current_data_offset),
+        topic(event.topic, event.topic_len),
+        msg_id(event.msg_id),
+        retain(event.retain),
+        qos(event.qos),
+        dup(event.dup),
+        error_handle(*event.error_handle) {}
+};
+
 class MQTTBackendIDF final : public MQTTBackend {
  public:
   static const size_t MQTT_BUFFER_SIZE = 4096;
@@ -99,7 +126,7 @@ class MQTTBackendIDF final : public MQTTBackend {
 
  protected:
   bool initialize_();
-  void mqtt_event_handler_(const esp_mqtt_event_t &event);
+  void mqtt_event_handler_(const Event &event);
   static void mqtt_event_handler(void *handler_args, esp_event_base_t base, int32_t event_id, void *event_data);
 
   struct MqttClientDeleter {
@@ -134,7 +161,7 @@ class MQTTBackendIDF final : public MQTTBackend {
   CallbackManager<on_unsubscribe_callback_t> on_unsubscribe_;
   CallbackManager<on_message_callback_t> on_message_;
   CallbackManager<on_publish_user_callback_t> on_publish_;
-  std::queue<esp_mqtt_event_t> mqtt_events_;
+  std::queue<Event> mqtt_events_;
 };
 
 }  // namespace mqtt