Merge pull request #3301 from csutcliff/pr/mqtt-task-stack

fix(mqtt): 8 KB MQTT task stack when TLS verifies a CA certificate

CHANGELOG_LATEST.md

@@ -71,6 +71,7 @@ This release is based on the latest Espressif/Arduino core version 3. It brings
 - a second boiler monitor `0xE4` carrying `0xFFFF` cleared a valid `pc0Flow`. That empty value is now ignored [#3224](https://github.com/emsesp/EMS-ESP32/issues/3224)
 - writing a negative custom entity value was always 1 too high because float-to-int rounding always added +0.5. Negative values now round the same way as positive ones [#3297](https://github.com/emsesp/EMS-ESP32/issues/3297)
 - after a cold boot, SoftAP fallback no longer abandons STA forever. If a home WiFi SSID is configured, EMS-ESP keeps retrying STA while the captive portal is up so a late-booting router (e.g. after a power outage) can still be joined; SoftAP stops once STA gets an IP
+- MQTT with TLS and a CA certificate crashed on every connect (a boot loop) on boards with PSRAM when the broker's certificate chain is EC, such as Let's Encrypt ISRG Root X2: verifying it needs about 6 KB of stack and the MQTT task only had 5120 bytes. The MQTT task now gets an 8 KB stack when TLS verifies a CA certificate, and keeps 5120 bytes for insecure TLS and plain MQTT
 
 ## Changed
 

lib/espMqttClient/src/MqttClient.cpp

@@ -13,7 +13,7 @@ using espMqttClientInternals::PacketType;
 using espMqttClientTypes::DisconnectReason;
 using espMqttClientTypes::Error;
 
-MqttClient::MqttClient(espMqttClientTypes::UseInternalTask useInternalTask, uint8_t priority, uint8_t core)
+MqttClient::MqttClient(espMqttClientTypes::UseInternalTask useInternalTask, uint8_t priority, uint8_t core, uint32_t stackSize)
 : _useInternalTask(useInternalTask)
 , _transport(nullptr)
 , _onConnectCallback(nullptr)
@@ -55,6 +55,7 @@ MqttClient::MqttClient(espMqttClientTypes::UseInternalTask useInternalTask, uint
 , _disconnectReason(DisconnectReason::TCP_DISCONNECTED)
 #if defined(ARDUINO_ARCH_ESP32) && ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO
 , _highWaterMark(4294967295)
+, _stackSize(stackSize)
 #endif
   {
   EMC_GENERATE_CLIENTID(_generatedClientId);
@@ -64,12 +65,12 @@ MqttClient::MqttClient(espMqttClientTypes::UseInternalTask useInternalTask, uint
   if (_useInternalTask == espMqttClientTypes::UseInternalTask::YES) {
 #ifdef CONFIG_FREERTOS_UNICORE
     // no core to pin to on single-core targets (S2, C3, C6)
-    xTaskCreate((TaskFunction_t)_loop, "mqttclient", EMC_TASK_STACK_SIZE, this, priority, &_taskHandle);
+    xTaskCreate((TaskFunction_t)_loop, "mqttclient", stackSize, this, priority, &_taskHandle);
 #else
     if (core > 1) {
-      xTaskCreate((TaskFunction_t)_loop, "mqttclient", EMC_TASK_STACK_SIZE, this, priority, &_taskHandle);
+      xTaskCreate((TaskFunction_t)_loop, "mqttclient", stackSize, this, priority, &_taskHandle);
     } else {
-      xTaskCreatePinnedToCore((TaskFunction_t)_loop, "mqttclient", EMC_TASK_STACK_SIZE, this, priority, &_taskHandle, core);
+      xTaskCreatePinnedToCore((TaskFunction_t)_loop, "mqttclient", stackSize, this, priority, &_taskHandle, core);
     }
 #endif
   }
@@ -77,6 +78,7 @@ MqttClient::MqttClient(espMqttClientTypes::UseInternalTask useInternalTask, uint
   (void) useInternalTask;
   (void) priority;
   (void) core;
+  (void) stackSize;
 #endif
   _clientId = _generatedClientId;
   _core = core;
@@ -320,7 +322,7 @@ void MqttClient::loop() {
   size_t waterMark = uxTaskGetStackHighWaterMark(NULL);
   if (waterMark < _highWaterMark) {
     _highWaterMark = waterMark;
-    emc_log_i("Stack usage: %zu/%i", EMC_TASK_STACK_SIZE - _highWaterMark, EMC_TASK_STACK_SIZE);
+    emc_log_i("Stack usage: %zu/%zu", _stackSize - _highWaterMark, _stackSize);
   }
   #endif
 }

lib/espMqttClient/src/MqttClient.h

@@ -81,7 +81,7 @@ class MqttClient {
   }
     
  protected:
-  explicit MqttClient(espMqttClientTypes::UseInternalTask useInternalTask, uint8_t priority = 1, uint8_t core = 1);
+  explicit MqttClient(espMqttClientTypes::UseInternalTask useInternalTask, uint8_t priority = 1, uint8_t core = 1, uint32_t stackSize = EMC_TASK_STACK_SIZE);
   espMqttClientTypes::UseInternalTask _useInternalTask;
   espMqttClientInternals::Transport* _transport;
 
@@ -207,6 +207,7 @@ class MqttClient {
   #if defined(ARDUINO_ARCH_ESP32)
   #if ARDUHAL_LOG_LEVEL >= ARDUHAL_LOG_LEVEL_INFO
   size_t _highWaterMark;
+  size_t _stackSize;
   #endif
   #endif
 };

lib/espMqttClient/src/MqttClientSetup.h

@@ -208,8 +208,8 @@ class MqttClientSetup : public MqttClient {
   */
 
  protected:
-  explicit MqttClientSetup(espMqttClientTypes::UseInternalTask useInternalTask, uint8_t priority = 1, uint8_t core = 1)
-  : MqttClient(useInternalTask, priority, core) {
+  explicit MqttClientSetup(espMqttClientTypes::UseInternalTask useInternalTask, uint8_t priority = 1, uint8_t core = 1, uint32_t stackSize = EMC_TASK_STACK_SIZE)
+  : MqttClient(useInternalTask, priority, core, stackSize) {
     #if EMC_MULTIPLE_CALLBACKS
     _onConnectCallback = [this](bool sessionPresent) {
       for (auto callback : _onConnectCallbacks) if (callback.first) callback.first(sessionPresent);

lib/espMqttClient/src/espMqttClient.cpp

@@ -15,8 +15,8 @@ espMqttClient::espMqttClient(espMqttClientTypes::UseInternalTask useInternalTask
     _transport = &_client;
 }
 
-espMqttClient::espMqttClient(uint8_t priority, uint8_t core)
-    : MqttClientSetup(espMqttClientTypes::UseInternalTask::YES, priority, core)
+espMqttClient::espMqttClient(uint8_t priority, uint8_t core, uint32_t stackSize)
+    : MqttClientSetup(espMqttClientTypes::UseInternalTask::YES, priority, core, stackSize)
     , _client() {
     _transport = &_client;
 }
@@ -27,8 +27,8 @@ espMqttClientSecure::espMqttClientSecure(espMqttClientTypes::UseInternalTask use
     _transport = &_client;
 }
 
-espMqttClientSecure::espMqttClientSecure(uint8_t priority, uint8_t core)
-    : MqttClientSetup(espMqttClientTypes::UseInternalTask::YES, priority, core)
+espMqttClientSecure::espMqttClientSecure(uint8_t priority, uint8_t core, uint32_t stackSize)
+    : MqttClientSetup(espMqttClientTypes::UseInternalTask::YES, priority, core, stackSize)
     , _client() {
     _transport = &_client;
 }

lib/espMqttClient/src/espMqttClient.h

@@ -48,7 +48,7 @@ class espMqttClientSecure : public MqttClientSetup<espMqttClientSecure> {
 class espMqttClient : public MqttClientSetup<espMqttClient> {
   public:
     explicit espMqttClient(espMqttClientTypes::UseInternalTask useInternalTask);
-    explicit espMqttClient(uint8_t priority = 1, uint8_t core = 1);
+    explicit espMqttClient(uint8_t priority = 1, uint8_t core = 1, uint32_t stackSize = EMC_TASK_STACK_SIZE);
 
   protected:
     espMqttClientInternals::ClientSync _client;
@@ -57,7 +57,7 @@ class espMqttClient : public MqttClientSetup<espMqttClient> {
 class espMqttClientSecure : public MqttClientSetup<espMqttClientSecure> {
   public:
     explicit espMqttClientSecure(espMqttClientTypes::UseInternalTask useInternalTask);
-    explicit espMqttClientSecure(uint8_t priority = 1, uint8_t core = 1);
+    explicit espMqttClientSecure(uint8_t priority = 1, uint8_t core = 1, uint32_t stackSize = EMC_TASK_STACK_SIZE);
     espMqttClientSecure & setInsecure();
     espMqttClientSecure & setCACert(const char * rootCA);
     espMqttClientSecure & setCertificate(const char * clientCa);

src/ESP32React/MqttSettingsService.cpp

@@ -33,9 +33,12 @@ void MqttSettingsService::begin() {
 
 void MqttSettingsService::startClient() {
     static bool     isSecure  = false;
+    static uint32_t stackSize = EMC_TASK_STACK_SIZE;
+    // only verifying a CA chain needs the larger stack, insecure TLS and plain MQTT fit in the default
+    const uint32_t wantStack = (_state.enableTLS && _state.rootCA != "insecure") ? EMSESP_MQTT_STACKSIZE_CA : EMC_TASK_STACK_SIZE;
     if (_mqttClient) {
         // do we need to change the client?
-        if ((isSecure && _state.enableTLS) || (!isSecure && !_state.enableTLS)) {
+        if (isSecure == _state.enableTLS && stackSize == wantStack) {
             return;
         }
 
@@ -43,12 +46,14 @@ void MqttSettingsService::startClient() {
         _mqttClient = nullptr;
     }
 
+    stackSize = wantStack;
+
     if (_state.enableTLS) {
         isSecure = true;
         if (emsesp::EMSESP::system_.PSram() == 0) {
             _mqttClient = new espMqttClientSecure(espMqttClientTypes::UseInternalTask::NO);
         } else {
-            _mqttClient = new espMqttClientSecure(EMSESP_MQTT_PRIORITY, EMSESP_MQTT_RUNNING_CORE);
+            _mqttClient = new espMqttClientSecure(EMSESP_MQTT_PRIORITY, EMSESP_MQTT_RUNNING_CORE, wantStack);
         }
 
         if (!_mqttClient) {

src/ESP32React/MqttSettingsService.h

@@ -60,6 +60,11 @@
 #define EMC_TASK_STACK_SIZE EMSESP_MQTT_STACKSIZE
 #endif
 
+// MQTT task stack when TLS verifies the broker against a CA. BearSSL needs ~6 KB to verify an EC (P-384, P-521) chain
+#ifndef EMSESP_MQTT_STACKSIZE_CA
+#define EMSESP_MQTT_STACKSIZE_CA 8192
+#endif
+
 #ifndef EMSESP_MQTT_PRIORITY
 #define EMSESP_MQTT_PRIORITY 1
 #endif