Heap addendum to handle changes in NON-OS SDK 3.0.x (#8746)

## WPA2 Enterprise connections References - merged PRs: * #8529 * #8566 - these occurred with connect/disconnect with WPA-Enterprise * #8736 (comment) The NON-OS SDK 3.0.x has breaking changes to the [`pvPortMalloc`](https://github.com/espressif/ESP8266_NONOS_SDK/blob/bf890b22e57a41d5cda00f9c8191f3f7035a87b4/include/mem.h#L42) function. They added a new `bool` argument for selecting a heap. ```cpp void *pvPortMalloc (size_t sz, const char *, unsigned, bool); ``` To avoid breaking the build, I added a new thin wrapper function `sdk3_pvPortMalloc` to `heap.cpp`. Edited new SDK LIBs to call `pvPortMalloc`'s replacement `sdk3_pvPortMalloc`. They also added `pvPortZallocIram` and `pvPortCallocIram`, which are not a problem to support. Support added to `heap.cpp`. Issues with WPA2 Enterprise in new SDKs: * v3.0.0 and v3.0.1 - have the same memory leak and duplicate free bugs from before * v3.0.2 through v3.0.5 - have the same memory leak; however, _no_ duplicate free crash. * memory leak can be seen by cycling through setup, connect, disconnect, and clear setup - repeatedly. Updated `wpa2_eap_patch.cpp` and binary patch scripts to handle v3.0.0 through v3.0.5. Patched SDKs v3.0.0 through v3.0.5 ## Duplicate Non-32-bit exception handler Issue: At v3.0.0 and above `libmain.a` supplies a built-in exception handler (`load_non_32_wide_handler`) for non-32-bit access. Our non-32-bit access handler (`non32xfer_exception_handler`) overrides it. Solution: Add "weak" attribute to symbol `load_non_32_wide_handler`. Adjust the build to default to the SDK's built-in non-32-bit handler. If there is a need to use our non-32-bit handler, make the selection from the Arduino IDE Tools menu `Non-32-Bit Access: "Byte/Word access to IRAM/PROGMEM (very slow)"`. With SDKs v3.0.0 and above a "non-32-bit exception handler" is always present.
esp8266 · Dec 16, 2022 · 4a0b66b · 4a0b66b
1 parent d3c8d27
commit 4a0b66b
Show file tree

Hide file tree

Showing 76 changed files with 317 additions and 66 deletions.
diff --git a/cores/esp8266/core_esp8266_main.cpp b/cores/esp8266/core_esp8266_main.cpp
@@ -496,7 +496,7 @@ extern "C" void user_init(void) {
     install_vm_exception_handler();
 #endif
 
-#if defined(NON32XFER_HANDLER) || defined(MMU_IRAM_HEAP)
+#if defined(NON32XFER_HANDLER) || (defined(MMU_IRAM_HEAP) && (NONOSDK < (0x30000 - 1)))
     install_non32xfer_exception_handler();
 #endif
 

diff --git a/cores/esp8266/core_esp8266_non32xfer.cpp b/cores/esp8266/core_esp8266_non32xfer.cpp
@@ -26,10 +26,14 @@
  * Code taken directly from @pvvx's public domain code in
  * https://github.com/pvvx/esp8266web/blob/master/app/sdklib/system/app_main.c
  *
+ * In Espressif versions NONOSDK v3.0.0+ a similar feature was added
+ * load_non_32_wide_handler. Theirs is always loaded. Add weak attribute to
+ * theirs so we can choose by adding an alias to ours.
  *
  */
 
 #include <Arduino.h>
+#include <user_interface.h>
 #define VERIFY_C_ASM_EXCEPTION_FRAME_STRUCTURE
 #include <esp8266_undocumented.h>
 #include <core_esp8266_non32xfer.h>
@@ -58,10 +62,13 @@ extern "C" {
 
 #define EXCCAUSE_LOAD_STORE_ERROR 3 /* Non 32-bit read/write error */
 
+#if (defined(NON32XFER_HANDLER) || defined(MMU_IRAM_HEAP)) && (NONOSDK < (0x30000 - 1))
 static fn_c_exception_handler_t old_c_handler = NULL;
+#endif
 
+#if defined(NON32XFER_HANDLER) || (defined(MMU_IRAM_HEAP) && (NONOSDK < (0x30000 - 1)))
 static
-IRAM_ATTR void non32xfer_exception_handler(struct __exception_frame *ef, int cause)
+IRAM_ATTR void non32xfer_exception_handler(struct __exception_frame *ef, [[maybe_unused]] int cause)
 {
   do {
     uint32_t insn, excvaddr;
@@ -138,6 +145,7 @@ IRAM_ATTR void non32xfer_exception_handler(struct __exception_frame *ef, int cau
   } while(false);
 
 /* Fail request, die */
+#if (NONOSDK < (0x30000 - 1))
   /*
     The old handler points to the SDK. Be alert for HWDT when Calling with
     INTLEVEL != 0. I cannot create it any more. I thought I saw this as a
@@ -148,16 +156,23 @@ IRAM_ATTR void non32xfer_exception_handler(struct __exception_frame *ef, int cau
     old_c_handler(ef, cause);
     return;
   }
-
+#endif
   /*
     Calling _xtos_unhandled_exception(ef, cause) in the Boot ROM, gets us a
     hardware wdt.
 
     Use panic instead as a fall back. It will produce a stack trace.
    */
+#if defined(DEBUG_ESP_PORT) || defined(DEBUG_ESP_MMU)
   panic();
+#else
+  // For non-debug builds, save on resources
+  abort();
+#endif
 }
+#endif // #if defined(NON32XFER_HANDLER) || (defined(MMU_IRAM_HEAP) && (NONOSDK < (0x30000 - 1)))
 
+#if (defined(NON32XFER_HANDLER) || defined(MMU_IRAM_HEAP)) && (NONOSDK < (0x30000 - 1))
 /*
   To operate reliably, this module requires the new
   `_xtos_set_exception_handler` from `exc-sethandler.cpp` and
@@ -174,5 +189,17 @@ void install_non32xfer_exception_handler(void) {
       non32xfer_exception_handler);
   }
 }
+#else
+// For v3.0.x SDKs, no need for install - call_user_start will do the job.
+// Need this for build dependencies
+void install_non32xfer_exception_handler(void) __attribute__((weak));
+void install_non32xfer_exception_handler(void) {}
+#endif
+
+#if defined(NON32XFER_HANDLER)
+// For SDKs 3.0.x, we made load_non_32_wide_handler in
+// libmain.c:user_exceptions.o a weak symbol allowing this override to work.
+extern void load_non_32_wide_handler(struct __exception_frame *ef, int cause) __attribute__((alias("non32xfer_exception_handler")));
+#endif
 
 };
diff --git a/cores/esp8266/exc-sethandler.cpp b/cores/esp8266/exc-sethandler.cpp
@@ -39,8 +39,10 @@
  * architecture, I am not convinced it can be done safely.
  *
 */
+#include <user_interface.h> // need NONOSDK
 
-#if defined(NON32XFER_HANDLER) || defined(MMU_IRAM_HEAP) || defined(NEW_EXC_C_WRAPPER) || defined(MMU_EXTERNAL_HEAP)
+#if defined(NON32XFER_HANDLER) || defined(MMU_IRAM_HEAP) || \
+    defined(NEW_EXC_C_WRAPPER) || defined(MMU_EXTERNAL_HEAP) || (NONOSDK >= (0x30000 - 1))
 
 /*
  * The original module source code came from:

diff --git a/cores/esp8266/heap.cpp b/cores/esp8266/heap.cpp
@@ -356,25 +356,25 @@ void system_show_malloc(void)
 void* IRAM_ATTR pvPortMalloc(size_t size, const char* file, int line)
 {
     HeapSelectDram ephemeral;
-    return heap_pvPortMalloc(size,  file, line);;
+    return heap_pvPortMalloc(size, file, line);;
 }
 
 void* IRAM_ATTR pvPortCalloc(size_t count, size_t size, const char* file, int line)
 {
     HeapSelectDram ephemeral;
-    return heap_pvPortCalloc(count, size,  file, line);
+    return heap_pvPortCalloc(count, size, file, line);
 }
 
 void* IRAM_ATTR pvPortRealloc(void *ptr, size_t size, const char* file, int line)
 {
     HeapSelectDram ephemeral;
-    return heap_pvPortRealloc(ptr, size,  file, line);
+    return heap_pvPortRealloc(ptr, size, file, line);
 }
 
 void* IRAM_ATTR pvPortZalloc(size_t size, const char* file, int line)
 {
     HeapSelectDram ephemeral;
-    return heap_pvPortZalloc(size,  file, line);
+    return heap_pvPortZalloc(size, file, line);
 }
 
 void IRAM_ATTR vPortFree(void *ptr, const char* file, int line)
@@ -384,7 +384,62 @@ void IRAM_ATTR vPortFree(void *ptr, const char* file, int line)
     // correct context. umm_malloc free internally determines the correct heap.
     HeapSelectDram ephemeral;
 #endif
-    return heap_vPortFree(ptr,  file, line);
+    return heap_vPortFree(ptr, file, line);
 }
 
+#if (NONOSDK >= (0x30000))
+////////////////////////////////////////////////////////////////////////////////
+/*
+  New for NON-OS SDK 3.0.0 and up
+  Needed for WPA2 Enterprise support. This was not present in SDK pre 3.0
+
+  The NON-OS SDK 3.0.x has breaking changes to pvPortMalloc. They added one more
+  argument for selecting a heap. To avoid breaking the build, I renamed their
+  broken version pvEsprMalloc. To be used, the LIBS need to be edited.
+
+  They also added pvPortZallocIram and pvPortCallocIram, which are not a
+  problem.
+
+  WPA2 Enterprise connect crashing is fixed at v3.0.2 and up.
+
+  Not used for unreleased version NONOSDK3V0.
+*/
+void* IRAM_ATTR sdk3_pvPortMalloc(size_t size, const char* file, int line, bool iram)
+{
+    if (iram) {
+        HeapSelectIram ephemeral;
+        return heap_pvPortMalloc(size, file, line);
+    } else {
+        HeapSelectDram ephemeral;
+        return heap_pvPortMalloc(size, file, line);
+    }
+}
+
+void* IRAM_ATTR pvPortCallocIram(size_t count, size_t size, const char* file, int line)
+{
+    HeapSelectIram ephemeral;
+    return heap_pvPortCalloc(count, size, file, line);
+}
+
+void* IRAM_ATTR pvPortZallocIram(size_t size, const char* file, int line)
+{
+    HeapSelectIram ephemeral;
+    return heap_pvPortZalloc(size, file, line);
+}
+
+/*
+  uint32_t IRAM_ATTR user_iram_memory_is_enabled(void)
+  {
+    return CONFIG_ENABLE_IRAM_MEMORY;
+  }
+
+  We do not need the function user_iram_memory_is_enabled().
+  1. It was used by mem_manager.o which was replaced with this custom heap
+     implementation. IRAM memory selection is handled differently.
+  2. In libmain.a, Cache_Read_Enable_New uses it for cache size. However, When
+     using IRAM for memory or running with 48K IRAM for code, we use a
+     replacement Cache_Read_Enable to correct the cache size ignoring
+     Cache_Read_Enable_New's selected value.
+*/
+#endif
 };
diff --git a/cores/esp8266/wpa2_eap_patch.cpp b/cores/esp8266/wpa2_eap_patch.cpp
@@ -5,12 +5,44 @@
  * modules.
  *
  */
-
 #include <string.h>
 #include <ets_sys.h>
 #include <pgmspace.h>
 #include "coredecls.h"
 
+#if defined(NONOSDK22x_190703) || \
+    defined(NONOSDK22x_191122) || \
+    defined(NONOSDK22x_191105) || \
+    defined(NONOSDK22x_191124) || \
+    defined(NONOSDK22x_190313) || \
+    defined(NONOSDK221)        || \
+    defined(NONOSDK3V0)        || \
+    defined(NONOSDK300)        || \
+    defined(NONOSDK301)        || \
+    defined(NONOSDK302)        || \
+    defined(NONOSDK303)        || \
+    defined(NONOSDK304)        || \
+    defined(NONOSDK305)
+
+// eap_peer_config_deinit() - For this list of SDKs there are no significant
+// changes in the function. Just the line number reference for when vPortFree
+// is called. When vPortFree is called, register a12 continues to hold a pointer
+// to the struct StateMachine. Our cleanup routine should continue to work.
+#if defined(NONOSDK300) || defined(NONOSDK301)
+    // Minor changes only line number changed
+    #define SDK_LEAK_LINE 809
+#elif defined(NONOSDK302) || defined(NONOSDK303) || defined(NONOSDK304)
+    // Minor changes only line number changed
+    #define SDK_LEAK_LINE 831
+#elif defined(NONOSDK305)
+    // At v3.0.5 Espressif moved `.text.eap_peer_config_deinit` to
+    // `eap_peer_config_deinit` then later in latest git they moved it
+    // back. For our linker script both are placed in flash.
+    #define SDK_LEAK_LINE 831
+#else
+    #define SDK_LEAK_LINE 799
+#endif
+
 #ifdef  DEBUG_WPA2_EAP_PATCH
 #include "esp8266_undocumented.h"
 #define DEBUG_PRINTF ets_uart_printf
@@ -100,7 +132,7 @@ struct StateMachine { // size 200 bytes
  * same line.
  */
 void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a12) {
-    if (799 == line) {
+    if (SDK_LEAK_LINE == line) {
         // This caller is eap_peer_config_deinit()
         struct StateMachine* sm = (struct StateMachine*)a12;
         if (ptr == sm->config[0]) {
@@ -126,21 +158,38 @@ void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a
         }
 #endif
     }
-#if 0
-    // This is not needed because the call was NO-OPed in the library. This code
-    // snippit is just to show how a future memory free issue might be resolved.
-    else if (672 == line) {
+
+#if defined(NONOSDK300) || defined(NONOSDK301)
+    else if (682 == line) {
         // This caller is wpa2_sm_rx_eapol()
         // 1st of a double free
         // let the 2nd free handle it.
         return;
     }
+#elif defined(NONOSDK302) || defined(NONOSDK303) || defined(NONOSDK304) || defined(NONOSDK305)
+    // It looks like double free is fixed. WPA2 Enterpise connections work
+    // without crashing. wpa2_sm_rx_eapol() has a few changes between NONOSDK301
+    // and NONOSDK302. However, this set of releases still have memory leaks.
+#else
+    // This is not needed because the call was NO-OPed in the library.
+    // Keep code snippit for reference.
+    // else if (672 == line) {
+    //     // This caller is wpa2_sm_rx_eapol()
+    //     // 1st of a double free
+    //     // let the 2nd free handle it.
+    //     return;
+    // }
 #endif
     vPortFree(ptr, file, line);
 }
 
 };
 
+#else
+#error "Internal error: A new SDK has been added. This module must be updated."
+#error "  Need to test WPA2 Enterpise connectivity."
+#endif
+
 /*
  * This will minimize code space for non-wifi enterprise sketches which do not
  * need the patch and disable_extra4k_at_link_time().