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 some ssl memory leaks #1288

Merged
merged 2 commits into from
Nov 16, 2017
Merged

fix some ssl memory leaks #1288

merged 2 commits into from
Nov 16, 2017

Conversation

ericogr
Copy link
Contributor

@ericogr ericogr commented Nov 14, 2017

I've got memory leak using mqtt with ssl enabled. It occurs when broker restart (using mqtt.connect(...) again to reconnect)

adding ssl_ctx_free(sslContext) when closing and onError apparently worked but I'm not shure if it can cause side effects or best way to do it

@@ -165,7 +165,8 @@ void TcpConnection::onError(err_t err)
#ifdef ENABLE_SSL
if(ssl) {
sslConnected = false;
ssl_free(ssl); // ssl_free frees internally also the SSL context and the SSL extension data
ssl_free(ssl);
ssl_ctx_free(sslContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. looking at the axtls code for ssl_ctx_free it seems that the function internally calls ssl_free. Excerpt:

EXP_FUNC void STDCALL ssl_ctx_free(SSL_CTX *ssl_ctx)
{
    SSL *ssl;
    int i;

    if (ssl_ctx == NULL)
        return;

    ssl = ssl_ctx->head;

    /* clear out all the ssl entries */
    while (ssl)
    {
        SSL *next = ssl->next;
        ssl_free(ssl);
        ssl = next;
    }

// ...

Which means that probably just replacing ssl_free(ssl); with ssl_ctx_free(sslContext); should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's true, I'll try :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ericogr May be one last thing - add the following comment behind the ssl_ctx_free(sslContext); // ssl_ctx_free frees the context. Internally it calls ssl_free which frees SSL extension data and SSL itself.

@@ -165,7 +165,8 @@ void TcpConnection::onError(err_t err)
#ifdef ENABLE_SSL
if(ssl) {
sslConnected = false;
ssl_free(ssl); // ssl_free frees internally also the SSL context and the SSL extension data
ssl_free(ssl);
Copy link
Contributor

@slaff slaff Nov 14, 2017

Choose a reason for hiding this comment

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

@ericogr Please, delete that line above. The line below should take care to do the right free-ing.

@@ -297,8 +298,8 @@ void TcpConnection::close()
#ifdef ENABLE_SSL
if (ssl != nullptr) {
debugf("SSL: closing ...");
// ssl_ctx_free(sslContext);
ssl_free(ssl);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ericogr Same as above - delete that line above. The line below should take care to do the right free-ing.

@slaff slaff added this to the 3.5.0 milestone Nov 16, 2017
@slaff slaff removed the 3 - Review label Nov 16, 2017
@slaff slaff merged commit 841d084 into SmingHub:develop Nov 16, 2017
@ericogr ericogr deleted the ssl-mem-leak branch November 16, 2017 14:33
johndoe8967 added a commit to johndoe8967/Sming that referenced this pull request Dec 18, 2017
* commit 'f8f675415c36dd4d8da45cfc3669f50469c65d8d':
  Preparation for release 3.5.0. (SmingHub#1295)
  Added Stream::indexOf(char c) that finds a character in a stream (SmingHub#1290)
  Made spiffs_mount() compatible with rBoot. (SmingHub#1292)
  Added experimental support for SDK 2.1 (SmingHub#1264)
  Initial test code for improved sendPing and sendPong. (SmingHub#1270)
  Added experimental support for LWIP v2 (SmingHub#1289)
  Fixed ssl memory leaks related to SSL context not being freed (SmingHub#1288)
  Fixed an error breaking SSL session resumption, Http Connection reuse and Http pipelining. (SmingHub#1287)
  Added Adafruit_BME280 Library (SmingHub#1286)
  Allow immediate server deletion if there are no active connections. (SmingHub#1285)
  Deleting an HttpClient should result in freeing the total memory it uses.
  Allow shutting down of TcpServers (SmingHub#1284)
  TcpConnection fixes related to ssl extensions. Styling fixes for HttpClient.
  fix/MemoryLeak(Heap) during TCP Client connection and delete
  Reverted: m_printf: stacksize reduced SmingHub#1097. (SmingHub#1279)
  Preparation for release 3.4.0. (SmingHub#1277)
  Mqtt memory fix: Fix copy and paste error (SmingHub#1276)
  Fix Memory Leak in Mqtt (SmingHub#1273)
  Changed a TcpClient message to be less confusing. (SmingHub#1271)
johndoe8967 added a commit to johndoe8967/Sming that referenced this pull request Jan 16, 2019
* commit '4a0fec18235521e4369d111f111c2624fbd3203b': (50 commits)
  Added the Arduino Libraries.
  Added the latest changes to the third-party projects.
  Preparation for release 3.5.0. (SmingHub#1295)
  Added Stream::indexOf(char c) that finds a character in a stream (SmingHub#1290)
  Made spiffs_mount() compatible with rBoot. (SmingHub#1292)
  Added experimental support for SDK 2.1 (SmingHub#1264)
  Initial test code for improved sendPing and sendPong. (SmingHub#1270)
  Added experimental support for LWIP v2 (SmingHub#1289)
  Fixed ssl memory leaks related to SSL context not being freed (SmingHub#1288)
  Fixed an error breaking SSL session resumption, Http Connection reuse and Http pipelining. (SmingHub#1287)
  Added Adafruit_BME280 Library (SmingHub#1286)
  Allow immediate server deletion if there are no active connections. (SmingHub#1285)
  Deleting an HttpClient should result in freeing the total memory it uses.
  Allow shutting down of TcpServers (SmingHub#1284)
  TcpConnection fixes related to ssl extensions. Styling fixes for HttpClient.
  fix/MemoryLeak(Heap) during TCP Client connection and delete
  Reverted: m_printf: stacksize reduced SmingHub#1097. (SmingHub#1279)
  Preparation for release 3.4.0. (SmingHub#1277)
  Mqtt memory fix: Fix copy and paste error (SmingHub#1276)
  Fix Memory Leak in Mqtt (SmingHub#1273)
  ...
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.

2 participants