espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.51k stars 7.26k forks source link

[TW#14025] esp_ble_gatts_set_attr_value memory leak #796

Closed lucashutch closed 7 years ago

lucashutch commented 7 years ago

Hi All,

I have recently been using the esp32 as a GATT Server with a service table

I had been using the function esp_ble_gatts_set_attr_value to update a characteristic. Whenever I call this function, then print the amount of free stack, the amount would drop by the length of the data specified in the function + 20 bytes.

attached is a debug log showing the issue: image

code:

ESP_LOGI("update_char_value", "setting attr value, len: %i", len);
esp_ble_gatts_set_attr_value(handle, len, data);
ESP_LOGI("Stack Free", "%08i", esp_get_free_heap_size());

It seems as if memory is being allocated, without freeing the previous buffer?

Please let me know if i am wrong.

HubbyGitter commented 7 years ago
lucashutch commented 7 years ago

Hi @HubbyGitter, Thanks for your reply.

1) Sorry wrote it down incorrectly and then just kept using the same name. I do mean heap.

2) I cant share the whole program. However I have setup a task with a loop that has a small delay that calls a function update_char_value. Task Loop:

uint8_t data[20] = {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20};
while(1)
{
    vTaskDelay(10 / portTICK_RATE_MS);
    update_char_value(data, sizeof(data));
}

Function:

void update_char_value(uint8_t* data, uint16_t len)
{
    uint16_t handle = handle_table[IDX_TX_VAL];
    ESP_LOGI("update_char_value", "setting attr value, len: %i", len);
    esp_ble_gatts_set_attr_value(handle, len, data);
    ESP_LOGI("Heap Free", "%08i", esp_get_free_heap_size());

    if(notifications_enabled())
    {
        esp_ble_gatts_send_indicate(profile_tab.gatts_if,
                        profile_tab.conn_id, handle, len, data, false);
    }
}

When I call the function as it is, the available heap seems to only go down, by 40 bytes every time I call esp_ble_gatts_set_attr_value. If I comment this line out, and solely use esp_ble_gatts_send_indicate the data still reaches the BLE central, but there is no longer a memory leak.

3) Given how the leak disappears if I comment out that single line of code only I don't see how it could be because of a memory allocation problem elsewhere in my program. However if you can think of a reason I am all ears.

I hope this helps to clarify a few things. Let me know if there is anything else that i can provide to help with this.

HubbyGitter commented 7 years ago

Thank you for sharing how you isolated the potential cause of your observation.

I looked a bit into this out of curiosity and it seems that later in the calling sequence, two buffers are allocated for this message via osi_malloc(), which allocates zeroed memory on heap and generates an allocation record if CONFIG_BLUEDROID_MEM_DEBUG is defined.

I currently don't have the time to dig deeper. Hope this is going to be fixed.

Weijian-Espressif commented 7 years ago

@lucazader thanks for your reminder , we will fix the bug in version 3.0 . you can free it in the bta_gatts_act.c , bta_gatts_set_attr_value() :

void bta_gatts_set_attr_value(tBTA_GATTS_SRVC_CB p_srvc_cb, tBTA_GATTS_DATA p_msg) { tBTA_GATTS_RCB *p_rcb = &bta_gatts_cb.rcb[p_srvc_cb->rcb_idx]; UINT16 service_id = p_srvc_cb->service_id; tBTA_GATTS cb_data; tBTA_GATT_STATUS gatts_status; gatts_status = GATTS_SetAttributeValue(p_msg->api_add_char_descr.hdr.layer_specific, p_msg->api_set_val.length, p_msg->api_set_val.value);

cb_data.attr_val.server_if = p_rcb->gatt_if;
cb_data.attr_val.service_id = service_id;
cb_data.attr_val.attr_id = p_msg->api_set_val.hdr.layer_specific;
cb_data.attr_val.status = gatts_status;
// free the value
 if (p_msg->api_set_val.value  != NULL){
    GKI_freebuf(p_msg->api_set_val.value);
}

if (p_rcb->p_cback) {
    (*p_rcb->p_cback)(BTA_GATTS_SET_ATTR_VAL_EVT, &cb_data);
}

}

lucashutch commented 7 years ago

Hi @Weijian-Espressif

Thanks for this!

Quick question: Is there any reason why we only free the value p_msg->api_set_val.value rather than the whole message p_msg ?

Weijian-Espressif commented 7 years ago

hi @lucazader ,

there is a task thread, btu_task_thread_handler() in btu_task.c, when you set attr value, the task used bta_sys_event( ) function to handle it , the function would free the p_msg. void bta_sys_event(BT_HDR *p_msg) { UINT8 id; BOOLEAN freebuf = TRUE;

APPL_TRACE_EVENT("BTA got event 0x%x\n", p_msg->event);

/* get subsystem id from event */
id = (UINT8) (p_msg->event >> 8);

/* verify id and call subsystem event handler */
if ((id < BTA_ID_MAX) && (bta_sys_cb.reg[id] != NULL)) {
    freebuf = (*bta_sys_cb.reg[id]->evt_hdlr)(p_msg);
} else {
    APPL_TRACE_WARNING("BTA got unregistered event id %d\n", id);
}

//** free **// if (freebuf) { GKI_freebuf(p_msg); }

}

lucashutch commented 7 years ago

resolved with commit ea3b304