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 memory leak when parsing X509 certs #2010

Merged
merged 4 commits into from
Aug 13, 2021
Merged

Fix memory leak when parsing X509 certs #2010

merged 4 commits into from
Aug 13, 2021

Conversation

alberk8
Copy link
Contributor

@alberk8 alberk8 commented Aug 13, 2021

Description

  • Release/free the memory after use.

Motivation and Context

  • Calling any TLS/SSL related website over multiple times will cause the call to end up with error.
  • Fixes memory leak when parsing X509 certs.

How Has This Been Tested?

Run the code below:

  1. 20K iteration

  2. 12hours of soak time.

while (true)
{
	try
	{
	   
		Debug.WriteLine("Start HTTP Request");
		X509Certificate rootCACert1 = new X509Certificate(Resources.GetBytes(Resources.BinaryResources.rootca));
	
		
		using var httpWebRequest = (HttpWebRequest)WebRequest.Create("https://www.google.com");
		
		Debug.WriteLine("Get Request");
		httpWebRequest.Method = "GET";
		httpWebRequest.KeepAlive = true;
		
		httpWebRequest.SslProtocols = System.Net.Security.SslProtocols.Tls12;
		httpWebRequest.HttpsAuthentCert = rootCACert1;
		
		Debug.WriteLine("Get Response");
		var httpWebResponse = (HttpWebResponse)httpWebRequest.GetResponse();
		Debug.WriteLine("HTTP Content Length: " + httpWebResponse.ContentLength);

		byte[ ] buffer = new byte[100];
		Debug.WriteLine("Get Response Stream");
		using (Stream stream = httpWebResponse.GetResponseStream())
		{
			int bytesRead = stream.Read(buffer, 0, buffer.Length);
			string content = Encoding.UTF8.GetString(buffer, 0, bytesRead);
			Debug.WriteLine("HTTP Content: " + content);
		}

	
		Debug.WriteLine("Http(s) Closing");

		httpWebResponse.Close();
		count++;
		
		Debug.WriteLine("Count: " + count);
		Debug.WriteLine("Memory: " + nanoFramework.Runtime.Native.GC.Run(false));
	}
	catch(Exception ex)
	{
		Debug.WriteLine("HTTPS Catch Exception: " + ex.Message);
	}
	
}
  1. ESP32 WROOM with and without PSRAM
  2. ESP32 WROVER KIT

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)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (work on Unit Tests, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

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.

@nfbot
Copy link
Member

nfbot commented Aug 13, 2021

@alberk8 I've fixed the checklist for you.
FYI, the correct format is [x], no spaces inside brackets.

Automated fixes for code style.
@nfbot
Copy link
Member

nfbot commented Aug 13, 2021

@alberk8 there are issues with the code style on the source files.
A PR was submitted with the code style fixes. Please click alberk8#1, review the changes if you want and merge it.

Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that.

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Nice catch! So there was a memory leak after all... 😅

alberk8 and others added 2 commits August 13, 2021 15:42
…1c8-631c-4e0c-8425-b85dfb11b3a9

Code style fixes for nanoframework/nf-interpreter PR#2010
@josesimoes josesimoes added Area: Common libs Everything related with common libraries and removed Type: enhancement labels Aug 13, 2021
@josesimoes josesimoes changed the title Fix mbedtls_x509_crt structs memory not released Fix memory leak when parsing X509 certs Aug 13, 2021
@josesimoes josesimoes enabled auto-merge (squash) August 13, 2021 07:47
@josesimoes josesimoes merged commit 261206d into nanoframework:develop Aug 13, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants