Skip to content

Potential heap corruption in fs::SPIFFSFS after PR #4443 #4491

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

Closed
thewavelength opened this issue Nov 4, 2020 · 3 comments
Closed

Potential heap corruption in fs::SPIFFSFS after PR #4443 #4491

thewavelength opened this issue Nov 4, 2020 · 3 comments
Labels
Status: Stale Issue is stale stage (outdated/stuck)

Comments

@thewavelength
Copy link
Contributor

thewavelength commented Nov 4, 2020

@me-no-dev I think I've found a potential bug in my PR #4443.

Accidentally, I've created a local helper function in my project where I accept fs::SPIFFSFS as parameter - you can see: I forgot the reference. The correct declaration for my helper function parameter would have been fs::SPIFFSFS &. It cost me an hour of debugging because the compiler didn't spit out any error or warning. The error was a heap corruption.

While normally one should not copy fs::SPIFFSFS because obviously it can have unintended side effects, it did happen to me.

I don't know the implementation details of that class, but it could be PR #4443 is the root cause:

There is no copy constructor defined, so the compiler generates the default one. The default copy constructor will plainly copy all POD types and this happens to the member variable pointer partitionLabel_. Then, when the local helper function in my project is returning, the destructor is called, the heap will be freed and now in the original instance, partitionLabel_ points to invalid heap memory.

There are the following solutions to this:

  1. Delete the default copy constructor and default operator= (maybe also their move semantics counterpart)
  2. Implement proper constructors/operators which copy the pointer correctly
  3. Use smart pointers from the C++ std lib (though I am not sure this is a dependency Arduino Core wants to have?)
  4. Re-introduce the String object which I used in the orignal commit of that PR

I am in favor of 4. Please let me know which solution you prefer and I can create a PR for this.

@lorol
Copy link
Contributor

lorol commented Nov 5, 2020

Other things may be interesting for you:
The alternate FS LITTLEFS requires the label, will not mount otherwise if NULL provided (default for SPIFFS)
https://github.com/espressif/arduino-esp32/blob/esp32s2/libraries/SPIFFS/src/SPIFFS.cpp#L60
https://github.com/espressif/arduino-esp32/blob/esp32s2/libraries/LITTLEFS/src/LITTLEFS.cpp#L46
LTTLEFS currently reuses same partition label, type and subtype as default SPIFFS.
https://github.com/espressif/arduino-esp32/blob/esp32s2/libraries/LITTLEFS/src/LITTLEFS.cpp#L15
Mounting point name was used to separate some SPIFFS vs. LITTLEFS differences see:
882b12c
Having a choice for more than one partition may create tool's and partition scheme challenges

@stale
Copy link

stale bot commented Jan 6, 2021

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: Stale Issue is stale stage (outdated/stuck) label Jan 6, 2021
@stale
Copy link

stale bot commented Jan 20, 2021

[STALE_DEL] This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Issue is stale stage (outdated/stuck)
Projects
None yet
Development

No branches or pull requests

2 participants