Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 9748af13 authored by Mike J. Chen's avatar Mike J. Chen
Browse files

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: default avatarMike J. Chen <mjchen@google.com>
parent d6d3cee2

system/stack/srvc/srvc_dis.c

+6 −4
Original line number Diff line number Diff line
@@ -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

system/stack/srvc/srvc_eng.c

+9 −1
Original line number Diff line number Diff line
@@ -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;

        }