WiFiClientSecure memory leak in SSLContext::connect

[Jeroen88]

Basic Infos

Hardware

Hardware: NodeMCU 1.0 (ESP-12E Module)
Core Version: 2.1.0-rc2 and latest GitHub version

Description

Memory is leaking (some 24 bytes, depending on hostName size) every time I use ESP8266HTTPClient to GET data from a secure site.

Drilled down the problem to SSLContext::connect(ClientContext* ctx, const char* hostName, uint32_t timeout_ms). In this function memory is allocated using the function ssl_ext_set_host_name(ext, hostName); this memory is, however in my opinion, never freed.

Added just before the end of the connect function: ssl_ext_set_host_name(ext, nullptr); Now no more memory is leaking.

Complete function:

void connect(ClientContext* ctx, const char* hostName, uint32_t timeout_ms)
{
    SSL_EXTENSIONS* ext = ssl_ext_new();
    ssl_ext_set_host_name(ext, hostName);
    ssl_ext_set_max_fragment_size(ext, 4096);
    if (_ssl) {
        /* Creating a new TLS session on top of a new TCP connection.
           ssl_free will want to send a close notify alert, but the old TCP connection
           is already gone at this point, so reset s_io_ctx. */
        s_io_ctx = nullptr;
        ssl_free(_ssl);
        _available = 0;
        _read_ptr = nullptr;
    }
    s_io_ctx = ctx;
    _ssl = ssl_client_new(_ssl_ctx, 0, nullptr, 0, ext);
    uint32_t t = millis();

    while (millis() - t < timeout_ms && ssl_handshake_status(_ssl) != SSL_OK) {
        uint8_t* data;
        int rc = ssl_read(_ssl, &data);
        if (rc < SSL_OK) {
            break;
        }
    }
    ssl_ext_set_host_name(ext, nullptr); // THIS FUNCTION FREEs the host_name
}

Settings in IDE

Module: NodeMCU 1.0 (ESP-12E Module)
Flash Size: 4MB/1MB
CPU Frequency: 80Mhz
Flash Mode: qio
Flash Frequency: 40Mhz
Upload Using: OTA / SERIAL
Reset Method: ?ck / nodemcu?

Sketch

#include <Arduino.h>

void setup() {

}

void loop() {

}

Debug Messages

messages here

[Defozo]

Please somebody smart look into this. This may solve some issues that pops up here lately.

#3392
#3402
#2075

[earlephilhower]

I think I saw this in my own work on the WiFiSecureServer patch 3001, too. I didn't dig into it, but after reconnecting to a SSL MQTT I always ended up dropping 24 bytes (3 allocation uints), as well.

Looking at AXTLS code it seems like that's where the real problem is.: the ssl_ext_free doesn't ever free the strdup()'d hostname, which seems like it should be part of the cleanup instead of requiring people to send in a NULL new hostname before ssl_ext_free().

Also, the ssl_ext_set_host_name() call which does the strdup() always free's the old hostname...even if it was NULL before (i.e. right after ssl_ext creation!). I need to double check this, though, as I'm not quite sure the use of the ext_host_name.

Maybe a (upstream?) patch there would be the "right" place to fix this, instead of the SSL wrapper code?

[Jeroen88]

@earlephilhower I completely agree with you, my solution was a work aroud, should be changed in ssl_ext_free().

[earlephilhower]

@Jeroen88 Cool.

I've sent in a 3-line pull request ( igrr/axtls-8266#49 ) that should fix this.

If you're running the git head IDE (and not the 2.3.0 official release) could you either build my patch locally ( https://github.com/earlephilhower/axtls-8266 ) or you can use the attached file to try and re-link your app, after removing your ssl_set_host_name(NULL) call? If it works you can ping in on the PR and close this.

libaxtls.zip

[Jeroen88]

@earlephilhower thanks for the pull request! Could even be easier, checking for null is not necessary, for freeing a null pointer has no effect.

I am quite new at GitHub and do not know yet how to use it properly.. :), so for now I am happy with the work around and the fix in the next release.

I think I found another issue, an incorrect bug fix in the upcoming release, would be great if it would be fixed too. Could you have a look at it an probably make a pull request too? It is #3415.

[marcelowajnsztok]

@Jeroen88 Adding ssl_ext_set_host_name(ext, nullptr); fixed my problem too. I had the same 24 bytes leaking and now it looks stable.
Thanks!

[Jeroen88]

@marcelowajnsztok you're welcome!

[pwrsoft]

I'm doing my custom project for ESP8266. Got this error when tried to use libaxtls.zip from @earlephilhower 's post.
@section ".text' will not fit in region `iram1_0_seg'"

[ajlop]

Hi! I´m interested in trying the Jeroen88'solution. But i got this errors:

:C:\Users\Particular\Documents\Arduino\libraries\ESP8266WiFi\src\WiFiClientSecure.cpp: In member function 'void SSLContext::connect(ClientContext*, const char*, uint32_t)':

C:\Users\Particular\Documents\Arduino\libraries\ESP8266WiFi\src\WiFiClientSecure.cpp:103:2: error: 'SSL_EXTENSIONS' was not declared in this scope

SSL_EXTENSIONS* ext = ssl_ext_new();

^

C:\Users\Particular\Documents\Arduino\libraries\ESP8266WiFi\src\WiFiClientSecure.cpp:103:18: error: 'ext' was not declared in this scope

SSL_EXTENSIONS* ext = ssl_ext_new();

              ^

C:\Users\Particular\Documents\Arduino\libraries\ESP8266WiFi\src\WiFiClientSecure.cpp:103:36: error: 'ssl_ext_new' was not declared in this scope

SSL_EXTENSIONS* ext = ssl_ext_new();

                                ^

C:\Users\Particular\Documents\Arduino\libraries\ESP8266WiFi\src\WiFiClientSecure.cpp:104:40: error: 'ssl_ext_set_host_name' was not declared in this scope

 ssl_ext_set_host_name(ext, hostName);

                                    ^

C:\Users\Particular\Documents\Arduino\libraries\ESP8266WiFi\src\WiFiClientSecure.cpp:105:44: error: 'ssl_ext_set_max_fragment_size' was not declared in this scope

 ssl_ext_set_max_fragment_size(ext, 4096);

                                        ^

it's a library problem but I can't find the solution.
Thank you!!!

[NMi-ru]

Had 56 bytes leaking. Downloaded a latest git. Downloaded latest axtls, recompiled, got libaxtls.a, moved to esp8266 directory, recompiled the sample sketch (as shown above), still no luck.

Moved the client declaration/initialization from top of the sketch to the inner function ("loop" in the demo case), the memory leak has vanished.

WiFiClientSecure client;

[devyte]

@NMi-ru did you try pr #4516 ?

[NMi-ru]

@NMi-ru did you try pr #4516 ?

I do not see the problem I have described since I have applied this patch. I hope this little problem has gone forever, thanks for the suggestion!

P.S. Is it better to have the client declaration in the top section? What [dis/]advantages does it have if I have only one place in my code where I use the client?

[jsdevel]

Moved the client declaration/initialization from top of the sketch to the inner function ("loop" in the demo case), the memory leak has vanished.

Fixed my problem too! Thanks @NMi-ru !

For others coming to this thread, I started searching when I saw Exception (29) in the serial monitor. Using this in my loop function, I was able to narrow it down to a memory leak:

Serial.print("Free HEAP: ");
Serial.println(ESP.getFreeHeap());