From eaf796f1dc6a0e719c8f957f629b35c4faf2c01a Mon Sep 17 00:00:00 2001 From: "Mike J. Chen" Date: Fri, 27 Jun 2014 19:01:10 -0700 Subject: [PATCH] Fix GKI buffer leak with discovery information service reading If the discovery information service of the LE client has the fields model number, serial number, fw version, etc, the service would allocate PKI buffer and never do anything with it, so it would leak. It looks like it should have been assigned the a callback string array, however fixing that still doesn't fix the leak because the code that receives the string array, bta_hh_le_dis_cback(), never uses it and never frees it. I believe the semantic is that the string arrays are kept around as a cache in the srvc engine connection structure, so it's the srvc engine dealloc of the callback structure that needs to also free the string buffers if they have been allocated. After fixing the string array allocation, add code to free the string array entries if they are not null. Also fixed an off by one error in DIS_SrUpdate() that would also lead to a GKI buffer leak. Improve two string termination cases to use a simple set of the last entry in the char array instead of memsetting the whole array when most of it will be filled by a following memcpy. Change-Id: I7905cd771dbbe166e3c2b42e019bac9f5a312877 Signed-off-by: Mike J. Chen --- stack/srvc/srvc_dis.c | 10 ++++++---- stack/srvc/srvc_eng.c | 10 +++++++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/stack/srvc/srvc_dis.c b/stack/srvc/srvc_dis.c index 50ffe12a6..c9d8a395a 100644 --- a/stack/srvc/srvc_dis.c +++ b/stack/srvc/srvc_dis.c @@ -291,9 +291,10 @@ void dis_c_cmpl_cback (tSRVC_CLCB *p_clcb, tGATTC_OPTYPE op, GKI_freebuf(p_str); if ((p_str = (UINT8 *)GKI_getbuf((UINT16)(p_data->att_value.len + 1))) != NULL) { - memset(p_str, 0, p_data->att_value.len + 1); p_clcb->dis_value.attr_mask |= DIS_UUID_TO_ATTR_MASK (read_type); memcpy(p_str, p_data->att_value.value, p_data->att_value.len); + p_str[p_data->att_value.len] = 0; + p_clcb->dis_value.data_string[read_type - GATT_UUID_MODEL_NUMBER_STR] = p_str; } break; @@ -314,7 +315,7 @@ void dis_c_cmpl_cback (tSRVC_CLCB *p_clcb, tGATTC_OPTYPE op, ** ** Function DIS_SrInit ** -** Description Initializa the Device Information Service Server. +** Description Initialize the Device Information Service Server. ** *******************************************************************************/ tDIS_STATUS DIS_SrInit (tDIS_ATTR_MASK dis_attr_mask) @@ -393,15 +394,16 @@ tDIS_STATUS DIS_SrUpdate(tDIS_ATTR_BIT dis_attr_bit, tDIS_ATTR *p_info) if (dis_attr_bit & (UINT16)(1 << i)) { if (dis_cb.dis_value.data_string[i - 1] != NULL) - GKI_freebuf(dis_cb.dis_value.data_string[i]); + GKI_freebuf(dis_cb.dis_value.data_string[i - 1]); /* coverity[OVERRUN-STATIC] False-positive : when i = 8, (1 << i) == DIS_ATTR_PNP_ID_BIT, and it will never come down here CID 49902: Out-of-bounds read (OVERRUN_STATIC) Overrunning static array "dis_cb.dis_value.data_string", with 7 elements, at position 7 with index variable "i". */ if ((dis_cb.dis_value.data_string[i - 1] = (UINT8 *)GKI_getbuf((UINT16)(p_info->data_str.len + 1))) != NULL) { - memset(dis_cb.dis_value.data_string[i - 1], 0, p_info->data_str.len + 1); /* make sure null terminate */ + memcpy(dis_cb.dis_value.data_string[i - 1], p_info->data_str.p_data, p_info->data_str.len); + dis_cb.dis_value.data_string[i - 1][p_info->data_str.len] = 0; /* make sure null terminate */ st = DIS_SUCCESS; } else diff --git a/stack/srvc/srvc_eng.c b/stack/srvc/srvc_eng.c index 1c96056cb..f0d6dfbfb 100644 --- a/stack/srvc/srvc_eng.c +++ b/stack/srvc/srvc_eng.c @@ -29,6 +29,8 @@ //#endif #include "srvc_battery_int.h" +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0])) + static void srvc_eng_s_request_cback (UINT16 conn_id, UINT32 trans_id, UINT8 op_code, tGATTS_DATA *p_data); static void srvc_eng_connect_cback (tGATT_IF gatt_if, BD_ADDR bda, UINT16 conn_id, BOOLEAN connected, tGATT_DISCONN_REASON reason, tBT_TRANSPORT transport); @@ -185,7 +187,7 @@ tSRVC_CLCB *srvc_eng_clcb_alloc (UINT16 conn_id, BD_ADDR bda) ** ** Description The function deallocates a GATT profile connection link control block ** -** Returns NTrue the deallocation is successful +** Returns True the deallocation is successful ** *******************************************************************************/ BOOLEAN srvc_eng_clcb_dealloc (UINT16 conn_id) @@ -197,6 +199,12 @@ BOOLEAN srvc_eng_clcb_dealloc (UINT16 conn_id) { if (p_clcb->in_use && p_clcb->connected && (p_clcb->conn_id == conn_id)) { + unsigned j; + for (j = 0; j < ARRAY_SIZE(p_clcb->dis_value.data_string); j++) { + if (p_clcb->dis_value.data_string[j]) { + GKI_freebuf(p_clcb->dis_value.data_string[j]); + } + } memset(p_clcb, 0, sizeof(tSRVC_CLCB)); return TRUE; } -- 2.11.0