Skip to content

Conversation

@mhightower83
Copy link
Contributor

WiFi Enterprise option can leak up to 3 allocations per connect/disconnect cycle (configuration dependent): anonymous Identity, password, and some unidentified allocation.

This solution patches eap.o from libwpa2.a to call a special 2-part wrapper instead of vPortFree at cleanup.

This is supplemental to #8529 (comment) and should not be merged prior.

There may be a better way of doing this. My knowledge of xtensa-lx106-elf-objcopy is near zero and everything that sounded promising didn't work the way I needed.

I don't have a use case for WiFi Enterprise so my testing has been limited. More testing is needed.

To minimize resources used, the patch will only be linked in when you have a call to enable_wifi_enterprise_patch().

Use something like this to enable patch:

#include <ESP8266WiFi.h>
#include <coredecls.h>
// ...
void setup() {
  enable_wifi_enterprise_patch();
// ...
}
@mhightower83 mhightower83 force-pushed the pr-enterprise-patch-leak branch from 61b2e7b to f8675fa Compare May 13, 2022 21:07
@Flole998
Copy link
Contributor

Excellent job, I am really impressed. I tried to do something similar but failed miserably.

@mhightower83 mhightower83 force-pushed the pr-enterprise-patch-leak branch from f8675fa to abc5cff Compare May 14, 2022 03:11
WiFi Enterprise option can leak up to 3 allocations per connect/disconnect
cycle: anonymous Identity, password, and some unidentified allocation.

This solution patches eap.o from libwpa2 to call a special 2 part
wrapper instead of vPortFree for cleanup.

Corrected typos and adjusted tabs in script.

Added script eval_fix_sdks.sh to aid in evaluating similarity between
patch sections of .o files being patched across different SDKs.
@mhightower83 mhightower83 force-pushed the pr-enterprise-patch-leak branch from abc5cff to a8d2054 Compare May 14, 2022 21:45
@mhightower83 mhightower83 marked this pull request as ready for review June 1, 2022 21:16
WiFi Enterprise option can leak up to 3 allocations per connect/disconnect
cycle: anonymous Identity, password, and some unidentified allocation.

This solution patches eap.o from libwpa2 to call a special 2 part
wrapper instead of vPortFree for cleanup.

Corrected typos and adjusted tabs in script.

Added script eval_fix_sdks.sh to aid in evaluating similarity between
patch sections of .o files being patched across different SDKs.
@mhightower83 mhightower83 force-pushed the pr-enterprise-patch-leak branch from a2d2aa2 to 1d88df0 Compare June 1, 2022 21:16
@d-a-v d-a-v merged commit 9e2103f into esp8266:master Jun 2, 2022
@earlephilhower
Copy link
Collaborator

I've been watching this for a while and just want to say, great work @mhightower83 ! You must have spent much time delving through disassembled code to fix this!

@mhightower83
Copy link
Contributor Author

@earlephilhower Thanks! I started using this bug to test some code I was working on to catch a double free. I ended out making several more debug enhancements to umm_malloc. These helped reduce the amount of assembly code I was reading.

@mhightower83 mhightower83 deleted the pr-enterprise-patch-leak branch June 3, 2022 14:54
mcspr pushed a commit that referenced this pull request Dec 16, 2022
## 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.
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
* Patch eap.o memory leak

WiFi Enterprise option can leak up to 3 allocations per connect/disconnect
cycle: anonymous Identity, password, and some unidentified allocation.

This solution patches eap.o from libwpa2 to call a special 2 part
wrapper instead of vPortFree for cleanup.

Corrected typos and adjusted tabs in script.

Added script eval_fix_sdks.sh to aid in evaluating similarity between
patch sections of .o files being patched across different SDKs.

* Add some dev debug code and improve comments

* Patch eap.o memory leak

WiFi Enterprise option can leak up to 3 allocations per connect/disconnect
cycle: anonymous Identity, password, and some unidentified allocation.

This solution patches eap.o from libwpa2 to call a special 2 part
wrapper instead of vPortFree for cleanup.

Corrected typos and adjusted tabs in script.

Added script eval_fix_sdks.sh to aid in evaluating similarity between
patch sections of .o files being patched across different SDKs.

* Add some dev debug code and improve comments
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
## WPA2 Enterprise connections
References - merged PRs:
* esp8266#8529
* esp8266#8566 - these occurred with connect/disconnect with WPA-Enterprise
* esp8266#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants