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

Rework SSL code #1427

Merged
merged 2 commits into from
Aug 4, 2019
Merged

Rework SSL code #1427

merged 2 commits into from
Aug 4, 2019

Conversation

josesimoes
Copy link
Member

Description

  • Add missing calls to platform free memory and mbedTLS API calls to free TLS vars memory on TLS context init failure and exit.
  • The above fixed a memory leak with TLS context. Related with SslStream crashes MCU or throws SocketException in what appears to be random Home#509.
  • Rework ssl close socket to account for bad or non existing TLS context and not nulling required TLS context vars required to properly close context and free memory.
  • Several renames in SSL_Context struct for clarity.
  • Rename ssl close socket (was missing an _).
  • Rework several declarations to match our coding style guidelines.
  • Replace booleans at several code locations.
  • Replace SSL private functions with ones from C standard libs.
  • Clean up code and includes in several files.
  • Remove ssl types source file (wasn't being used).
  • Remove calls to (not used anymore) time callbacks.

Motivation and Context

How Has This Been Tested?

  • SSL sample code running for 20 minutes. No authentication or closing socket errors occurred.

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: José Simões jose.simoes@eclo.solutions

@josesimoes josesimoes added Type: bug Type: enhancement Area: Common libs Everything related with common libraries labels Jul 31, 2019
@nfbot
Copy link
Member

nfbot commented Jul 31, 2019

Hi @josesimoes,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

@josesimoes josesimoes requested a review from AdrianSoundy July 31, 2019 12:30
@josesimoes josesimoes force-pushed the rework-ssl branch 3 times, most recently from a19d5b2 to 808fef7 Compare July 31, 2019 23:50
- Add missing calls to platform free memory and mbedTLS API calls to free TLS vars memory on TLS context init failure and exit.
- The above fixed a memory leak with TLS context. Related with nanoframework/Home#509.
- Rework ssl close socket to account for bad or non existing TLS context and not nulling required TLS context vars required to properly close context and free memory.
- Several renames in SSL_Context struct for clarity.
- Rename ssl close socket (was missing an _).
- Rework several declarations to match our coding style guidelines.
- Replace booleans at several code locations.
- Replace SSL private functions with ones from C standard libs.
- Clean up code and includes in several files.
- Remove ssl types source file (wasn't being used).
- Remove calls to (not used anymore) time callbacks.

Signed-off-by: José Simões <jose.simoes@eclo.solutions>
@AdrianSoundy
Copy link
Member

Looking at changes i will need some time to go through them so i will do it at the weekend.

Copy link
Member

@AdrianSoundy AdrianSoundy left a comment

Choose a reason for hiding this comment

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

Happy with code changes accept for one small thing.

The prototype for ssl_closesocket_internal is:-

int  ssl_closesocket_internal( int sd );

But you are returning a bool not an int.

Have been through all the memory allocation and freeing and it looks ok.

Currently the SSL sample will only run about 15 times on ESP32 before getting exceptions.
Also i see the current available unmanaged heap is loosing about 4k per connection attempt. Now this is still with older MbedTls so will sort out why its not building with correct version and test again.

Managed to get it to build with newer MbedTLS by cloning it first. Still an issue with getting MbedTls automatically but that's some set up issue with ninja when cloning with git in cmake. Investigate later.

Found that the new build(Debug) no longer fits in ESP32 flash partition(1MB) when everything enabled :-) Fails when booting.

Still an issue with system heap and still fails to connect after about 12 connects
mbedtls_ssl_handshake returned -0x4c ( Error : Reading information from the socket failed. )

Looks like system heap memory leak is caused by the new X509Certificate() as i had that in the connection loop. Without that the memory stabilizes and connections don't fail. I would have had a number of letsEncryptCACert & digiCertGlobalRootCACert in managed heap maybe they use some system heap as well.

Copy link
Member

@AdrianSoundy AdrianSoundy left a comment

Choose a reason for hiding this comment

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

Change the return back to 0 for No error. Doesn't seem to be any code checking the return anyway

@AdrianSoundy
Copy link
Member

Tested SSL sample running for a couple of hours.

X509Certificate() seems to use system heap which causes problems when in the connection test loop, OK if you just use it once. This wasn't part of this PR will have a look at it to make sure there isn't a problem there. Probably just things are not being disposed due to GC not running. Maybe it should be using managed heap instead.

@AdrianSoundy AdrianSoundy merged commit 9fd78b8 into nanoframework:develop Aug 4, 2019
@josesimoes
Copy link
Member Author

Thanks for taking care of the the changes! 😉

morali pushed a commit to morali/nf-interpreter that referenced this pull request Oct 4, 2019
* Rework SSL code

- Add missing calls to platform free memory and mbedTLS API calls to free TLS vars memory on TLS context init failure and exit.
- The above fixed a memory leak with TLS context. Related with nanoframework/Home#509.
- Rework ssl close socket to account for bad or non existing TLS context and not nulling required TLS context vars required to properly close context and free memory.
- Several renames in SSL_Context struct for clarity.
- Rename ssl close socket (was missing an _).
- Rework several declarations to match our coding style guidelines.
- Replace booleans at several code locations.
- Replace SSL private functions with ones from C standard libs.
- Clean up code and includes in several files.
- Remove ssl types source file (wasn't being used).
- Remove calls to (not used anymore) time callbacks.

Signed-off-by: José Simões <jose.simoes@eclo.solutions>

* Update ssl_close_socket_internal.cpp

Return 0 ( no error )

(cherry picked from commit 9fd78b8)
@josesimoes josesimoes deleted the rework-ssl branch October 30, 2019 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Common libs Everything related with common libraries Type: bug Type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SslStream crashes MCU or throws SocketException in what appears to be random
3 participants