Arduino OTA Updater has memory leak in certain conditions

[DamronDan]

Board

any board using updater.cpp

Device Description

tested on many ESP32 dev boards

Hardware Configuration

mainly WLED strings, not much else.

Version

latest master (checkout manually)

IDE Name

PlatformIO

Operating System

Windows 10/11

Flash frequency

40Mhz/80Mhz

PSRAM enabled

no

Upload speed

921600, 115200

Description

On an OTA firmware attempt, if the transfer fails, and updateClass is defined statically, the buffers used for the update are not freed.

There is no destructor.

Sketch

any sketch using update.cpp

Debug Message

ESP.getFreeHeap() shows heap memory from buffers being allocated, and not used.

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[DamronDan]

Solution to this problem is easy: add this code:

UpdateClass::~UpdateClass(){
    if (_buffer) {
        free(_buffer);
        _buffer = nullptr;
    }
    if (_skipBuffer) {
        free(_skipBuffer);
        _skipBuffer = nullptr;
    }
}

[MathewHDYT]

@DamronDan Is correct this issue can be resolved immediately and does not require Investigation.

@mrengineer7777 Using malloc in combination with delete is undefined behaviour and can therefore cause a multitude of issues. This should be immediately fixed if possible.

See this blog post for more information on the issue.

malloc and new allocate from different heaps, so they can not free or delete each other’s memory. They also operate at different levels – raw memory vs. constructed objects.

The C++ operators new and delete guarantee proper construction and destruction; where constructors or destructors need to be invoked, they are. The C-style functions malloc(), calloc(), free(), and realloc() don’t ensure that. Furthermore, there is no guarantee that the mechanism used by new and delete to acquire and release raw memory is compatible with malloc() and free(). If mixing styles works on your system, you were simply “lucky” – for now.

The affected line where the memory is instantiated can be found here and the line where the memory is deleted and causes undefined behaviour can be found here.

Would be nice if this could be fixed as soon as possible using the code portion above should resolve that issue.

---

Additionally in the aforementioned code portion, the _skipBuffer is also never freed, so we have both a memory leak and undefined behaviour.

It would probably be best if the _skipBuffer could be changed to an array on the stack instead so something like this:

private:
    ...
    uint8_t _skipBuffer[ENCRYPTED_BLOCK_SIZE];
    ...

This would resolve the memory leak and the need to allocate 16 bytes on the heap, which doesn't really make sense for most use cases.

---

I am also assuming that ESP8266 Updater.cpp was a major inspiration for this implementation, but there both issues do not exists, because neither a _skipBuffer is allocated, nor is the memory for the _buffer allocated with malloc, but instead with new as it should.

The question is why is the Updater even implemented that way, if the ESP8266 code has been used as inspiration that would make sense because it does not have the same API as the ESP32

Because with the ESP32 we can include the "esp_ota_ops.h" header and already do we simply don't use it. This header already contains a lot of helper methods to write OTA data, see the offical documentation for more information. There is no need to implement this feature by hand instead we can simply use the esp-idf API.

An example implementation using some of the same methods as the Update.cpp can be found below:

Update.cpp

// Header include.
#include "Update.h"

// Library include.
#include <esp_ota_ops.h>


UpdateClass::UpdateClass() :
    m_ota_handle(0U),
    m_update_partition(nullptr)
{
    // Nothing to do
}

bool UpdateClass::begin(const size_t& firmware_size) {
    const esp_partition_t *running = esp_ota_get_running_partition();
    const esp_partition_t *configured = esp_ota_get_boot_partition();

    if (configured != running) {
        return false;
    }

    const esp_partition_t *update_partition = esp_ota_get_next_update_partition(nullptr);

    if (update_partition == nullptr) {
        return false;
    }

    // Temporary handle is used, because it allows using a void* as the actual ota_handle,
    // allowing us to only include the esp_ota_ops header in the defintion (.cpp) file,
    // instead of also needing to declare it in the declaration (.h) header file
    esp_ota_handle_t ota_handle;
    const esp_err_t error = esp_ota_begin(update_partition, firmware_size, &ota_handle);

    if (error != ESP_OK) {
        return false;
    }

    m_ota_handle = ota_handle;
    m_update_partition = update_partition;
    return true;
}

size_t UpdateClass::write(uint8_t* payload, const size_t& total_bytes) {
    const esp_err_t error = esp_ota_write(m_ota_handle, payload, total_bytes);
    const size_t written_bytes = (error == ESP_OK) ? total_bytes : 0U;
    return written_bytes;
}

void UpdateClass::reset() {
    (void)esp_ota_abort(m_ota_handle);
}

bool UpdateClass::end() {
    esp_err_t error = esp_ota_end(m_ota_handle);
    if (error != ESP_OK) {
        return false;
    }

    error = esp_ota_set_boot_partition(static_cast<const esp_partition_t*>(m_update_partition));
    return error == ESP_OK;
}

UpdateClass Update;

Whereas the header could look something like this.

Update.h

#ifndef ESP32UPDATER_H
#define ESP32UPDATER_H

class UpdateClass {
  public:
    UpdateClass();

    bool begin(const size_t& firmware_size) override;
  
    size_t write(uint8_t* payload, const size_t& total_bytes) override;

    void reset() override;
  
    bool end() override;

    private:
      uint32_t m_ota_handle;
      const void *m_update_partition;
};

extern UpdateClass Update;

#endif // ESP32UPDATER_H

As can be seen below the actual implementation could be much easier and shorter and not even need any internal buffer allocated on the heap that holds temporary memory. Especially 4KB of it, which is not an insignificant amount especially for micro-controllers.