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 potential memory leak in WiFiManagerParameter::setValue #1206

Merged
merged 2 commits into from
Mar 11, 2021
Merged

fix potential memory leak in WiFiManagerParameter::setValue #1206

merged 2 commits into from
Mar 11, 2021

Conversation

cpainchaud
Copy link
Contributor

No description provided.

@tablatronix
Copy link
Collaborator

So this would address a memory leak when calling setValue multiple times with changing values, correct?

@cpainchaud
Copy link
Contributor Author

I think so

@tablatronix tablatronix added the DEV Help Wanted Developer Needs Help label Feb 5, 2021
@tablatronix
Copy link
Collaborator

I need reviewers lol, ill try to look at this when i get a chance

@tablatronix tablatronix merged commit 65d3aff into tzapu:master Mar 11, 2021
tablatronix pushed a commit that referenced this pull request Mar 17, 2021
This reverts commit 65d3aff, reversing
changes made to fe8d297.
@tablatronix
Copy link
Collaborator

reverted, crashes esp32

bad array access

assertion "heap != NULL && "free() target pointer is outside heap areas"

@tablatronix
Copy link
Collaborator

@cpainchaud I had to revert this it was crashing esp32

@tablatronix tablatronix added bug Validated BUG QA Quality Assurance testing needed labels Mar 18, 2021
@cpainchaud
Copy link
Contributor Author

I saw that, it's weird actually, it means that another part of the code is relying on that memory to never be freed while it's obviously leaked ....

@tablatronix
Copy link
Collaborator

yeah I did not really look at it at all, was busy on stuff. it crashed immediately also which is odd..

@tablatronix tablatronix added this to the dev milestone Jun 21, 2021
@hurricanefrog
Copy link

This is because _value is not initialized in the constructor, or rather the init function - meaning the pointer is random.

Simply do _value = nullptr; in init and it's fixed

tablatronix added a commit that referenced this pull request Aug 31, 2022
@tablatronix
Copy link
Collaborator

I think this is right, please check

@hurricanefrog
Copy link

Works fine, doesn't crash

@hurricanefrog
Copy link

Works fine, doesn't crash

Or maybe it does? Just re-tested this. Initial flash goes fine, but if I OTA update "my" application again, I get a StoreProhibited error. Need to check where exactly, though

@hurricanefrog
Copy link

I think this is because _length is uninitialized as well! So if(_length != length) yields random results

@tablatronix
Copy link
Collaborator

tablatronix commented Sep 2, 2022

Thanks for testing this, let me know what you find or if you do not, etc

That error could also be a partition size issue or ota size issue

@zenz
Copy link

zenz commented Jul 7, 2023

on ESP8266, this
delete[] _value;
_value = new char[....]
will cause a crash.
So I use unique_ptr to try to solve the issue.
And I make a pull request, please check.

@tablatronix
Copy link
Collaborator

tablatronix commented Jul 8, 2023

Thanks, I havent had time to really look at this, and I am pretty sure I left a mix of null and nullptr in here, It need to be reworked to be memory safe, and I was just trying to figure out how make this safer when these are out of scope ( which alot of people do ) and make it not crash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Validated BUG DEV Help Wanted Developer Needs Help In Progress QA Quality Assurance testing needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants