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 of ssl_ext_host_name, losing memory every SSL disconnect #50

Merged
merged 1 commit into from
Jul 23, 2017
Merged

Fix memory leak of ssl_ext_host_name, losing memory every SSL disconnect #50

merged 1 commit into from
Jul 23, 2017

Conversation

earlephilhower
Copy link
Contributor

@Jeroen88 found a memory leak in axtls where the new ext_host_name pointer was leaking on every SSL reconnection, eventually eating up all RAM and crashing the chip (see esp8266/Arduino#3428 ).

This 1-line patch just frees the allocated hostname in during the ssl_ext_free call, eliminating the leakage.

ssl_ext_set_host_name uses strdup() to set a hostname into the ssl_ext
structure, but this memory was not being freed in ssl_ext_free.

Add a one-liner to free it during the ssl_ext_free routine, fixing the leak.
@igrr igrr merged commit 5de79d7 into igrr:master Jul 23, 2017
@igrr
Copy link
Owner

igrr commented Jul 23, 2017

Thank you for the PR, @earlephilhower!

igrr added a commit to esp8266/Arduino that referenced this pull request Jul 23, 2017
Includes two PRs:

- igrr/axtls-8266#46 by @earlephilhower:
  Move debug strings from RAM to Flash

- igrr/axtls-8266#50:
  Fix memory leak in ssl_ext_host_name
@slaff
Copy link
Contributor

slaff commented Jul 23, 2017

Until this version 425067a ssl_ext_free for freeing correctly the data, but then the free-ing issue was introduced in 2213f30. Glad that someone noticed it.

The current fix though relies on the fact that host-name will always be set free(ssl_ext->host_name);
. Which is a wrong assumption. I would prefer to have the original code from 425067a which is:

if (ssl_ext->host_name != NULL) 
{
         free(ssl_ext->host_name);
}

@Jeroen88
Copy link

@slaff freeing aan null pointer is perfectly legal in cpp and has no effect

@slaff
Copy link
Contributor

slaff commented Jul 23, 2017

@slaff freeing aan null pointer is perfectly legal in cpp and has no effect

Yep, that is fine but axTLS can be used in C context. IMHO it would be better if we have code that works well on C and C++.

@Jeroen88
Copy link

@slaff same is true for ansi c: if ptr is a null pointer, no action occurs :)

@igrr
Copy link
Owner

igrr commented Jul 23, 2017

@Jeroen88 is right, passing NULL to free should be a no-op, if this is not the case it is a bug in implementation of free on a given platform. Some coding standards explicitly discourage checking a pointer for the single purpose of freeing it.

@slaff
Copy link
Contributor

slaff commented Jul 24, 2017

@igrr @Jeroen88 Thanks for pointing that out. I will have to brush up my dusty C/C++ skills.

d-a-v pushed a commit to d-a-v/Arduino that referenced this pull request Sep 29, 2017
Includes two PRs:

- igrr/axtls-8266#46 by @earlephilhower:
  Move debug strings from RAM to Flash

- igrr/axtls-8266#50:
  Fix memory leak in ssl_ext_host_name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants