search

igrr / axtls-8266

axTLS port for ESP8266
Other
79 stars 33 forks source link

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

Closed earlephilhower closed 7 years ago

earlephilhower commented 7 years ago

@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.

igrr commented 7 years ago

Thank you for the PR, @earlephilhower!

slaff commented 7 years ago

Until this version 425067abe69606ab7002b34c047e46a007f29895 ssl_ext_free for freeing correctly the data, but then the free-ing issue was introduced in 2213f304490e059d72fd511cd6548b23a685ee8f. 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 425067abe69606ab7002b34c047e46a007f29895 which is:

if (ssl_ext->host_name != NULL) 
{
         free(ssl_ext->host_name);
}
Jeroen88 commented 7 years ago

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

slaff commented 7 years ago

@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 commented 7 years ago

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

igrr commented 7 years ago

@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 commented 7 years ago

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