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

Stack corruption calling btc_ble_gattc_get_attr_count uint16_t* assumed to be int* (IDFGH-2560) #4645

Closed paulelong closed 4 years ago

paulelong commented 4 years ago

4100

''' Environment

Problem Description

btc_ble_gattc_get_attr_count takes a uint16_t for count. It, in turn, calls BTA_GATTC_GetDBSizeByType and casts as an int. As an int is 32 bits and uint16 is 16, the result overwrites part of the stack by 16 bytes.

This code change in fixes the issue for me

diff --git a/components/bt/host/bluedroid/btc/profile/std/gatt/btc_gattc.c b/components/bt/host/bluedroid/btc/profile/std/gatt/btc_gattc.c
index 73214a67d..f3a412a9c 100644
--- a/components/bt/host/bluedroid/btc/profile/std/gatt/btc_gattc.c
+++ b/components/bt/host/bluedroid/btc/profile/std/gatt/btc_gattc.c
                                                uint16_t char_handle,
                                                uint16_t *count)
 {
+    int c = *count;
     if (type == ESP_GATT_DB_ALL) {
-        BTA_GATTC_GetDBSize(conn_id, start_handle, end_handle, (int *)count);
+        BTA_GATTC_GetDBSize(conn_id, start_handle, end_handle, (int *)&c);
     } else {
-        BTA_GATTC_GetDBSizeByType(conn_id, type, start_handle, end_handle, char_handle, (int *)count);
+        BTA_GATTC_GetDBSizeByType(conn_id, type, start_handle, end_handle, char_handle, (int *)&c);
     }

+    *count = c;
+
     return ESP_GATT_OK;
 }

Expected Behavior

A call to btc_ble_gattc_get_attr_count should not affect local variables.

Actual Behavior

When I called btc_ble_gattc_get_attr_count, it would overwrite a local variable on the stack. Similar to the gattc_multi_connect example, I would set notify_en=1. In some cases the value would be reset to 0 when btc_ble_gattc_get_attr_count is called.

Steps to reproduce

Debug gattc_multi_connect into BTA_GATTC_GetDBSizeByType and see how the stack is overwritten.

Code to reproduce this issue

You can use gattc_multi_connect and debug into btc_ble_gattc_get_attr_count in the case ESP_GATTC_REG_FOR_NOTIFY_EVT.

GYC-Espressif commented 4 years ago

Hello, thank for your report. We have solved this issue in the master branch in the commit 090843fa1784af4b46fc33cf2d881c548a35e679.

Alvin1Zhang commented 4 years ago

@paulelong Thanks for reporting. Would you please help try the commit by @GYC-Espressif and help share whether your issue could be solved? Thanks.

filzek commented 4 years ago

@Alvin1Zhang I have the similar problem but using Blufi, just open the issue today, its messing with all heap and malloc.

Alvin1Zhang commented 4 years ago

New issue tracked in https://github.com/espressif/esp-idf/issues/4715, will close this one, feel free to reopen if the issue in this ticket still happens. Thanks.