Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix double-free when connecting to WPA2-Enterprise networks #8529

Merged
merged 5 commits into from
Jun 2, 2022

Conversation

Flole998
Copy link
Contributor

@Flole998 Flole998 commented Apr 4, 2022

Fixes: #8082

This patches the callx0 instruction to a nop in eap.o which is part of libwpa2.a.
It looks like espressif fixed the Bug in newer SDK versions, so if we update to the latest NONOS-SDK it is most likely not necessary to add/adapt this patch.
Also modifies the fix_sdk_libs.sh script as it even changed files if no changes were necessary, for example adding multiple system_func1 exports.

tools/sdk/lib/fix_sdk_libs.sh Outdated Show resolved Hide resolved
Fixes: esp8266#8082

This patches the callx0 instruction to a nop in eap.o which is part of libwpa2.a.
It looks like espressif fixed the Bug in newer SDK versions, so if we update to the latest NONOS-SDK it is most likely not necessary to add/adapt this patch.
Also modifies the fix_sdk_libs.sh script as it even changed files if no changes were necessary, for example adding multiple system_func1 exports.
Copy link
Collaborator

@mcspr mcspr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the status here btw? everything was tested and works after these?

tools/sdk/lib/fix_sdk_libs.sh Outdated Show resolved Hide resolved
tools/sdk/lib/fix_sdk_libs.sh Outdated Show resolved Hide resolved
tools/sdk/lib/fix_sdk_libs.sh Outdated Show resolved Hide resolved
@Flole998
Copy link
Contributor Author

It does work, but there is still a memory leak upon reconnection (which was there before aswell, so no regression). That is not easily fixable though (I am not sure it can be fixed at all at the moment). If you can live with that leak then this is perfectly usable.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !
Next step is to add a basic example in the WiFi example directory
and maybe a WiFi API around the espressif API.

VERSION=$(basename ${PWD})

addSymbol_system_func1() {
ADDRESS=$1
xtensa-lx106-elf-objcopy --add-symbol system_func1=.irom0.text:${ADDRESS},function,global user_interface.o
if ! xtensa-lx106-elf-nm user_interface.o | grep -q " T system_func1"; then # Don't add symbol if it already exists
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just noticed that grep -q will always return 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it's not in there yet? That's strange (be aware that it got already added twice or so...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I noticed when I tried some modifications. grep -q appears to be an exception on returning non-zero exit status.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it works:

$ echo ThisIsATest | grep -q Test
$ echo ?$
0
$ echo ThisIsATest | grep -q Fail
$ echo $?
1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I am confused. It wasn't working for me and now it is. I had duplicate entries and the man page says grep -q exits with 0. However, I just tried everything again and it worked.

Sorry, I don't know what happened. Just ignore me, I am going to take a break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version as it is in the repo at the moment is having duplicate entries, those should be removed at some point. So maybe that caused the confusion (and that's why I added that check)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, strange. I only picked up the script and tried it and had duplicate entries. As well as other problems from running the script at the wrong directory which creates a libmain.a which creates a lot of grief.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Flole998 what do you mean "duplicate entries in the current version" ?
Is it in master or in this pull request ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In master (and also in this pull request because I didn't remove/fix it as that would be something for another PR I figured)

@d-a-v d-a-v merged commit f5ef02d into esp8266:master Jun 2, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic on WPA2 Enterprise Connection
4 participants