Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
paulelong opened this issue Jan 17, 2020 · 4 comments

Comments

@paulelong
Copy link

#4100
''' Environment

  • Development Kit: NA, but I'm using Hiletgo
  • Module or chip used: ESP32-WROOM-32S
  • IDF version v4.1-dev-1795-gca8fac876
  • Build System: idf.py
  • Compiler version: xtensa-esp32-elf-gcc.exe (crosstool-NG esp-2019r2) 8.2.0
  • Operating System: Windows
  • (Windows only) environment type: PowerShell
  • Using an IDE?: VSCode
  • Power Supply: USB

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.

@github-actions github-actions bot changed the title Stack corruption calling btc_ble_gattc_get_attr_count uint16_t* assumed to be int* Stack corruption calling btc_ble_gattc_get_attr_count uint16_t* assumed to be int* (IDFGH-2560) Jan 17, 2020
@GYC-Espressif
Copy link
Contributor

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

@Alvin1Zhang
Copy link
Collaborator

@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
Copy link

filzek commented Feb 5, 2020

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

@Alvin1Zhang
Copy link
Collaborator

New issue tracked in #4715, will close this one, feel free to reopen if the issue in this ticket still happens. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants