Why so many unnecessary allocations? [IDFGH-4980]

[jhnlmn]

Can you find any need for this seemingly unnecessary strdup/free in this code:

https://github.com/espressif/esp-idf/bl ... m_nimble.c

Code: Select all

gatt_svr_dsc_access(uint16_t conn_handle, uint16_t attr_handle, struct
                    ble_gatt_access_ctxt *ctxt, void *arg)
{
    if (ctxt->op != BLE_GATT_ACCESS_OP_READ_DSC) {
        ESP_LOGE(TAG, "Invalid operation on Read-only Descriptor");
        return BLE_ATT_ERR_UNLIKELY;
    }

    int rc;
    char *temp_outbuf = strdup(ctxt->dsc->arg);
    if (temp_outbuf == NULL) {
        ESP_LOGE(TAG, "Error duplicating user description of characteristic");
        return BLE_ATT_ERR_INSUFFICIENT_RES;
    }

    ssize_t temp_outlen = strlen(temp_outbuf);
    rc = os_mbuf_append(ctxt->om, temp_outbuf, temp_outlen);
    free(temp_outbuf);
    return rc;
}

I am very impressed by ESP-IDF SDK, but I am very concerned by their cavalier use of dynamic allocations.
Dynamic allocations are banned by many embedded development standards. Other embedded libraries usually provide some custom malloc replacement, like fixed block heaps, etc, but ESP-IDF looks like it is written for a desktop GUI or Web server space, not an embedded market.

[ESP_Angus]

Hi jhnlmn,

I've submitted this question internally for the provisioning team to check as I agree it seems unnecessary. An internal ID has been added to the topic. I think the code may have been adapted from the similar Bluedroid version where the underlying layer takes ownership of the string and it is freed later, and the developer missed that the allocation can be removed, but this is just a guess.

In terms of our available RAM resources, ESP32 sits in a strange segment where we have more RAM than most simple microcontroller devices but significantly less RAM than a desktop PC, at least any desktop PC newer than the 1980s. So we do use a very size-efficient heap implementation (previously a simple embedded-style heap and now one based on the TLSF algorithm which is size-efficient but also fast).

There are places where dynamic allocation is used to minimize the total memory use footprint over the lifetime of the firmware (where static allocation would mean worst-case usage at all times), or because we're incorporating external code which requires dynamic allocation. For example the default FreeRTOS APIs assume dynamic allocation for nearly all RTOS resources including task heaps. This doesn't excuse every instance that ESP-IDF uses dynamic allocation though, and certainly if you see more particularly unusual cases like this one then please point them out.

[ESP_Prasad]

Thank you @jhnlmn @ESP_Angus for your valuable inputs. I have raised an internal MR to address this issue.

[jhnlmn]

> the default FreeRTOS APIs assume dynamic allocation for nearly all RTOS resources including task heaps

Not really. FreeRTOS is very carefully written - it never allocates anything at run time (unless you use some diagnostics APIs). All tasks, queues, etc are allocated at init time. So does nimble. As far as I can see, all run time allocations are coming from ESP32 layer.

[ESP_Angus]

> the default FreeRTOS APIs assume dynamic allocation for nearly all RTOS resources including task heaps

Not really. FreeRTOS is very carefully written - it never allocates anything at run time (unless you use some diagnostics APIs). All tasks, queues, etc are allocated at init time. So does nimble. As far as I can see, all run time allocations are coming from ESP32 layer.

My mistake, what I meant is that by default it uses heap allocation not static allocation (allow there is the static allocation option now). You're totally right that it's possible to allocate all resources before the scheduler starts running, though.

[ESP_Alvin]

Hi jhnlmn,

Sorry for late reply, the fix is available at https://github.com/espressif/esp-idf/co ... df4adcd820.