Skip to content

Fixes stream load memory leak in WifiSecureClient for SSL CACert, Certificate, and Private Key #6387

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 26, 2022
Merged

Fixes stream load memory leak in WifiSecureClient for SSL CACert, Certificate, and Private Key #6387

merged 2 commits into from
Apr 26, 2022

Conversation

beeboopx
Copy link
Contributor

@beeboopx beeboopx commented Mar 8, 2022

Summary

This PR fixes massive memory leak in WifiSecureClient when stream loading the TLS CACert, Certificate, and Private Key.

Issue is not presented on the first load. Issue presents during all subsequent invocations of loadCACert, loadCertificate, and loadPrivateKey, respectively.

Issue was verified to exist and changes verified to work by printing the available heap once per second in my application and following the below procedure.

Issue verified by:

  1. First connect to a WiFi network, establish a TLS session utilizing loadCACert + loadCertificate + loadPrivateKey. Once session is established, note the free heap.
  2. Disable WiFi network, and the TLS session is ended.
  3. Re-Enable WiFi network, establish a new TLS session utilizing loadCACert + loadCertificate + loadPrivateKey again. Once session is established, note the free heap has permanently decreased by the size of the CA, Cert, and Private Key char arrays. In my case, this was about 5kB decrease on every new session established.

Fix verified by:

  1. First connect to a WiFi network, establish a TLS session utilizing loadCACert + loadCertificate + loadPrivateKey. Once session is established, note the free heap.
  2. Disable WiFi network, and the TLS session is ended.
  3. Re-Enable WiFi network, establish a new TLS session utilizing loadCACert + loadCertificate + loadPrivateKey again. Once session is established, the free heap no longer decreases as before fix applied.

Impact

Fixes above summarized memory leak in WifiClientSecure.cpp and does nothing else.

Related links

@beeboopx
Fixes stream load memory leak in WifiSecureClient for SSL CACert, Cer…
cec0701 
…tificate, and

Private Key. Issue presented during any subsequent invocation of loadCACert, loadCertificate, and
loadPrivateKey, respectively, after the first invocation.
@CLAassistant
Copy link

CLAassistant commented Mar 8, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@beeboopx
Copy link
Contributor Author

beeboopx commented Mar 8, 2022 •
edited
Loading

Hmmm it's working well for initial load, and if same cert is reloaded again, but if there is a different cert loaded it will panic with:

assert failed: heap_caps_realloc heap_caps.c:345 (heap != NULL && "free() pointer is outside heap areas")

i will investigate a bit more and push another commit with fixes.

Anyone have any ideas on this?

@mrengineer7777
Copy link
Collaborator

Possibly related:
#6062
#6055

@timr49
Copy link
Contributor

timr49 commented Mar 21, 2022

Hmmm it's working well for initial load, and if same cert is reloaded again, but if there is a different cert loaded it will panic with:

assert failed: heap_caps_realloc heap_caps.c:345 (heap != NULL && "free() pointer is outside heap areas")

i will investigate a bit more and push another commit with fixes.

Anyone have any ideas on this?

Feedback on proposed fix:

  1. In each of these three cases, if _streamLoad() returns null (e.g. because the stream has insufficient data available or malloc() fails) then _CA_cert/_cert/_private_key etc. are left pointing to space that has now been free()d. Suggest that between each call to free() and the following _streamLoad(), the member just free()d is set to null.2.

  2. _CA_cert/_cert/_private_key may have been passed by the calling application via setCACert/setCertificate/setPrivateKey in which case the calling application would (should?) be free()ing them, leading to the same space being free()d twice.

  3. There are still opportunities for memory leaks. For example ...

(a) the destructor for WiFiClientSecure does not appear to free any of these things. For example, if the example was changed to instantiate a new WiFiClientSecure object each time, it would get the some amount of leakage.

(b) a call to setInsecure() will leak any of these things that have previously been allocated;

  1. note that the solution is not as simple as free()ing them in ~WiFiClientSecure() etc. because they may have been allocated by the calling application and passed via calls to setCACert/setCertificate/setPrivateKey, in which case the calling application will be expecting to free() them.

  2. therefore, a solution needs allocator/reallocator/deallocator methods which take account of how they were previously allocated (or not), deallocates if appropriate before allocating/reallocating, and which deallocates when ~WiFiClientSecure() (or Insecure() or anything else that currently just assigns null) are called.

@github-actions
Copy link
Contributor

Unit Test Results

0 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌

Results for commit 46bf03c.

@me-no-dev
Copy link
Member

Hi @fillybrese ! It is always good to make sure that we do not leak any memory. Thanks for the contribution!

@me-no-dev me-no-dev merged commit d7ffd57 into espressif:master Apr 26, 2022
@TD-er
Copy link
Contributor

TD-er commented Apr 26, 2022

I only now noticed this PR...
I think there are multiple other issues here, especially with the mbedtls data structure.
This one isn't initialized correct, so the internal members may still contain random data resuling in pointers not being null and thus cause crashes when destructing the structure.
I did mention this several times in related issues.
See also the 4 "Helpers/ESPEasy_xxx" files in this PR: https://github.com/letscontrolit/ESPEasy/pull/3788/files#diff-e1489f0e20f34ec2d70451ffec1baaeeac1c789eb4c2dc54219e75e5ea6c10d5
These are copy/paste versions of the files in this repository, but including changes.

What happens is if a connection is failed and retried, then you may run into these issues. Partly fixed in this PR, but not in the other files (as far as I know)

Jason2866 added a commit to tasmota/arduino-esp32 that referenced this pull request Apr 26, 2022
@Jason2866 @valeros
@Ouss4 @pedrominatel @P-R-O-C-H-Y @ferbar @beeboopx @PaulZC @gonzabrusco @ankostis @me-no-dev @dreamcmi @UnexpectedMaker
updates from upstream (#134)
da277a0 
* LittleFs is working with C3

* Delete .skip.esp32c3

* Add support for extra flash images (espressif#6625)

This PR adds support for uploading additional flash images (e.g. Adafruit Tiny UF2 bootloader) specified in board manifests.

Additionally, the PR switches the PlatformIO CI script to the upstream version of the ESP32 dev-platform (basically reverts changes introduced in espressif#5387 as they are no longer required).

* publish.yml: Remove a leftover parenthesis that was making the workflow (espressif#6620)

Description of Change

Remove a leftover parenthesis that was making the workflow that was making the workflow invalid.

Tests scenarios

Github Workflow.

Related links

https://github.com/espressif/arduino-esp32/actions/runs/2213167501

Signed-off-by: Abdelatif Guettouche <[email protected]>

* Enable LittleFs sketches for C3 (espressif#6618)

* LittleFs is working with C3

* Delete .skip.esp32c3

* Update LittleFS PlatformIO example (espressif#6617)

* Added RainMaker support on Arduino IDE for ESP32-C3/S2/S3 (espressif#6598)

* Added RainMaker support on Arduino IDE for ESP32-C3/S2/S3

Closes espressif#6573
Note related to the issue espressif#6435

* Touch change to init only selected GPIO. (espressif#6609)

* Separated init for touch / channel called by touchRead()

* compile error

* Fixed touch_V2 + ISR

* Allow BluetoothSerial::connect() with specified channel and more options (espressif#6380)

* BTAddress const, add bool()

* BTAdvertisedDevice: const functions

* BluetoothSerial: add: getChannels, add isClosed, add read/peek timeout, add connect with channel#

* BluetoothSerial: add sec_mask, role in ::connect

* BluetoothSerial add discover and connect with channel number example

* DiscoverConnect: add SPP_ENABLED check

* DiscoverConnect: disable on esp32s3

* Fixes stream load memory leak in WifiSecureClient for SSL CACert, Certificate, and (espressif#6387)

Private Key. Issue presented during any subsequent invocation of loadCACert, loadCertificate, and
loadPrivateKey, respectively, after the first invocation.

* Call i2c_set_timeout in i2cSetClock (espressif#6537)

* Uniform behaviour of WiFiClientSecure and WiFiClient setTimeout() (espressif#6562)

* Uniform timeout WiFiClient-WiFiClientSecure

* Added missing prototype

* Add socket check on setTimeout

* enh(log) salvage TAG from ESP_IDF log-statements > (espressif#6567)

by converting result log-rows from the 1st line to the 2nd (`NET` is the tag):
```
[ 73419][D][telelogger.cpp:915] telemetry(): state: 33C

[ 73419][D][telelogger.cpp:915] telemetry(): [NET] state: 33C
```

Co-authored-by: Me No Dev <[email protected]>

* add AirM2M_CORE_ESP32C3 board (espressif#6613)

* add AirM2M_CORE_ESP32C3 board

* Added Unexpected Maker Reflow Master Pro (UM RMP) board (espressif#6630)

Fixed wrong SCK and MISO pins for TinyS2

* Tasmota changes (#24)

* Update install-arduino-core-esp32.sh

* Update CMakeLists.txt

* Update Esp.cpp

* Update Updater.cpp

Co-authored-by: Valerii Koval <[email protected]>
Co-authored-by: Abdelatif Guettouche <[email protected]>
Co-authored-by: Pedro Minatel <[email protected]>
Co-authored-by: Jan Procházka <[email protected]>
Co-authored-by: Christian Ferbar <[email protected]>
Co-authored-by: Billy <[email protected]>
Co-authored-by: Paul <[email protected]>
Co-authored-by: Gonzalo Brusco <[email protected]>
Co-authored-by: Kostis Anagnostopoulos <[email protected]>
Co-authored-by: Me No Dev <[email protected]>
Co-authored-by: Darren Cheng <[email protected]>
Co-authored-by: Unexpected Maker <[email protected]>
@beeboopx
Copy link
Contributor Author

beeboopx commented May 9, 2022

Thank you @mrengineer7777 @timr49 @me-no-dev @TD-er for your feedback on the PR. I will review the feedback and implement the changes as soon as i can. Just have been short on time recently, will do my best.

There were the issues discovered by myself and others in the original PR code. I would have recommended merging to master be delayed until these raised points are addressed.

@mrengineer7777
Copy link
Collaborator

I will review the feedback and implement the changes as soon as i can. Just have been short on time recently, will do my best.

There were the issues discovered by myself and others in the original PR code. I would have recommended merging to master be delayed until these raised points are addressed.

Understood. Your PR was helpful, so can't complain about the merge. Create a new PR to address the remaining issues when you have a chance. Thanks in advance for your contribution!

@mrengineer7777 mrengineer7777 mentioned this pull request Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory leak on multiple calls to loadCACert and others on WiFiClientSecure
6 participants
@beeboopx @CLAassistant @mrengineer7777 @timr49 @me-no-dev @TD-er