From a664cd8c8431ef1ec36bb04262c6570cb7eeb0a4 Mon Sep 17 00:00:00 2001 From: Harish Kumara M Date: Fri, 23 Nov 2018 21:04:29 +0530 Subject: [PATCH] Fix for crash in BLE receive thread. Synchronizing access to client and server senderinfo as there are chances of data receive handler may be acting on deleted SenderInfo data when connection is closed. When connection closed, all senderInfo for that connection are cleared in another thread. https://github.sec.samsung.net/RS7-IOTIVITY/IoTivity/commit/69383ab5a059d7dd114c69d5d5ff5371b5946c98 (cherry picked from 69383ab5a059d7dd114c69d5d5ff5371b5946c98) Change-Id: Ibc495dba0143eef908eaa35a0173dde77288d4be Signed-off-by: Harish Kumara M Signed-off-by: DoHyun Pyun --- .../connectivity/src/bt_le_adapter/caleadapter.c | 193 +++++++++++++++------ 1 file changed, 138 insertions(+), 55 deletions(-) diff --git a/resource/csdk/connectivity/src/bt_le_adapter/caleadapter.c b/resource/csdk/connectivity/src/bt_le_adapter/caleadapter.c index 01c80b2..d9ac819 100644 --- a/resource/csdk/connectivity/src/bt_le_adapter/caleadapter.c +++ b/resource/csdk/connectivity/src/bt_le_adapter/caleadapter.c @@ -59,6 +59,7 @@ typedef struct uint32_t totalDataLen; uint8_t *defragData; CAEndpoint_t *remoteEndpoint; + uint8_t refCount; } CABLESenderInfo_t; typedef enum @@ -74,6 +75,7 @@ typedef enum * default value is 20 byte. */ static uint16_t g_mtuSize = CA_DEFAULT_BLE_MTU_SIZE; + /** * Callback to provide the status of the network change to CA layer. */ @@ -131,7 +133,6 @@ static oc_mutex g_bleClientReceiveDataMutex = NULL; */ static oc_mutex g_bleServerReceiveDataMutex = NULL; - /** * Callback to be called when network packet received from either * GattServer or GattClient. @@ -281,6 +282,11 @@ static u_arraylist_t *g_bleServerSenderInfo = NULL; static u_arraylist_t *g_bleClientSenderInfo = NULL; /** + * Mutex to synchronize access to all senderInfos + */ +static oc_mutex g_senderInfoMutex = NULL; + +/** * Queue to process the outgoing packets from GATTServer. */ static CAQueueingThread_t *g_bleServerSendQueueHandle = NULL; @@ -511,6 +517,8 @@ static void CALERemoveReceiveQueueData(u_arraylist_t *dataInfoList, /** * get received data info and positioned index from the received data list * for client / server which is matched same leAddress and port. + * CABLESenderInfo_t is reference counted structure, you must call + * CALEFreeSenderInfo on senderInfo to release reference. * * @param[in] leAddress target address to get serderInfo. * @param[in] port target port to get senderInfo. @@ -527,6 +535,36 @@ static CAResult_t CALEGetSenderInfo(const char *leAddress, uint32_t *senderIndex); /** + * Add sender info to list. + * + * @param[in] u_arraylist_t Array list to add sender info to. + * @param[in] senderInfo Sender info to be added to list. + * + * @return true on success, otherwise false. + */ +static bool CALEAddSenderInfoToList(u_arraylist_t *senderInfoList, + CABLESenderInfo_t *senderInfo); + +/** + * Remove desired sender info from list. + * + * @param[in] u_arraylist_t Array list to remove sender info from. + * @param[in] senderInfo Sender info to be removed from list. + * + */ +static void CALERemoveSenderInfoFromList(u_arraylist_t *senderInfoList, + CABLESenderInfo_t *senderInfo); + +/** + * Free sender info. CABLESenderInfo_t is reference counted structure + * and info will be freed only when reference count reaches 0 or negative. + * + * @param[in] senderInfo Sender info to be freed. + * + */ +static CAResult_t CALEFreeSenderInfo(CABLESenderInfo_t *senderInfo); + +/** * get ports related to remote address. It is need because multi application * can have more than 2 senderInfo using same BLE address. So before remove * receive queue data, should get port list from sender Info. @@ -542,7 +580,6 @@ static CAResult_t CALEGetPortsFromSenderInfo(const char *leAddress, static CAResult_t CAInitLEServerQueues() { - CAResult_t result = CAInitLEServerSenderQueue(); if (CA_STATUS_OK != result) { @@ -713,6 +750,7 @@ static CAResult_t CAInitLEServerSenderQueue() static void CALEClearSenderInfoImpl(u_arraylist_t **list) { + oc_mutex_lock(g_senderInfoMutex); const size_t length = u_arraylist_length(*list); for (size_t i = 0; i < length; ++i) { @@ -726,6 +764,7 @@ static void CALEClearSenderInfoImpl(u_arraylist_t **list) } } u_arraylist_free(list); + oc_mutex_unlock(g_senderInfoMutex); } static void CALEClearSenderInfo() @@ -821,6 +860,7 @@ static CAResult_t CALEGetSenderInfo(const char *leAddress, "NULL index argument", CA_STATUS_INVALID_PARAM); + oc_mutex_lock(g_senderInfoMutex); const uint32_t listLength = u_arraylist_length(senderInfoList); const uint32_t addrLength = strlen(leAddress); for (uint32_t index = 0; index < listLength; index++) @@ -831,23 +871,73 @@ static CAResult_t CALEGetSenderInfo(const char *leAddress, continue; } - if (!strncmp(info->remoteEndpoint->addr, leAddress, addrLength)) + if (!strncmp(info->remoteEndpoint->addr, leAddress, addrLength) + && info->remoteEndpoint->port == port) { - if (info->remoteEndpoint->port == port) + *senderIndex = index; + if (senderInfo) { - *senderIndex = index; - if (senderInfo) - { - *senderInfo = info; - } - return CA_STATUS_OK; + *senderInfo = info; + (*senderInfo)->refCount++; } + oc_mutex_unlock(g_senderInfoMutex); + return CA_STATUS_OK; } } + oc_mutex_unlock(g_senderInfoMutex); return CA_STATUS_FAILED; } +static bool CALEAddSenderInfoToList(u_arraylist_t *senderInfoList, CABLESenderInfo_t *senderInfo) +{ + oc_mutex_lock(g_senderInfoMutex); + senderInfo->refCount++; + if (!u_arraylist_add(senderInfoList,(void *)senderInfo)) + { + senderInfo->refCount--; + oc_mutex_unlock(g_senderInfoMutex); + return false; + } + oc_mutex_unlock(g_senderInfoMutex); + return true; +} + +static void CALERemoveSenderInfoFromList(u_arraylist_t *senderInfoList, CABLESenderInfo_t *senderInfo) +{ + oc_mutex_lock(g_senderInfoMutex); + uint32_t idx; + bool elemFound = u_arraylist_get_index(senderInfoList, (const void*)senderInfo, &idx); + if(elemFound) + { + void *info = u_arraylist_remove(senderInfoList, idx); + if (info != NULL) + { + senderInfo->refCount--; + } + } + oc_mutex_unlock(g_senderInfoMutex); +} + +static CAResult_t CALEFreeSenderInfo(CABLESenderInfo_t *senderInfo) +{ + VERIFY_NON_NULL_RET(senderInfo, + CALEADAPTER_TAG, + "NULL senderInfo to remove argument", + CA_STATUS_INVALID_PARAM); + + oc_mutex_lock(g_senderInfoMutex); + senderInfo->refCount--; + if(senderInfo->refCount <= 0) + { + OICFree(senderInfo->defragData); + OICFree(senderInfo->remoteEndpoint); + OICFree(senderInfo); + } + oc_mutex_unlock(g_senderInfoMutex); + return CA_STATUS_OK; +} + static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverType) { OIC_LOG_V(DEBUG, CALEADAPTER_TAG, "%s", __func__); @@ -935,9 +1025,8 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp { OIC_LOG(ERROR, CALEADAPTER_TAG, "This packet is start packet but exist senderInfo. Remove senderInfo"); - u_arraylist_remove(bleData->senderInfo, senderIndex); - OICFree(senderInfo->defragData); - OICFree(senderInfo); + CALERemoveSenderInfoFromList(bleData->senderInfo, senderInfo); + CALEFreeSenderInfo(senderInfo); senderInfo = NULL; senderIndex = 0; } @@ -957,26 +1046,21 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp return; } - CABLESenderInfo_t *newSender = OICMalloc(sizeof(CABLESenderInfo_t)); + CABLESenderInfo_t *newSender = OICCalloc(1, sizeof(CABLESenderInfo_t)); if (!newSender) { OIC_LOG(ERROR, CALEADAPTER_TAG, "Memory allocation failed for new sender"); oc_mutex_unlock(bleReceiveDataMutex); return; } - newSender->recvDataLen = 0; - newSender->totalDataLen = 0; - newSender->defragData = NULL; - newSender->remoteEndpoint = NULL; OIC_LOG(DEBUG, CALEADAPTER_TAG, "Parsing the header"); newSender->totalDataLen = totalLength; - if (!(newSender->totalDataLen)) { OIC_LOG(ERROR, CALEADAPTER_TAG, "Total Data Length is parsed as 0!!!"); - OICFree(newSender); + CALEFreeSenderInfo(newSender); oc_mutex_unlock(bleReceiveDataMutex); return; } @@ -994,7 +1078,7 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp if (NULL == newSender->defragData) { OIC_LOG(ERROR, CALEADAPTER_TAG, "defragData is NULL!"); - OICFree(newSender); + CALEFreeSenderInfo(newSender); oc_mutex_unlock(bleReceiveDataMutex); return; } @@ -1015,8 +1099,7 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp if (NULL == newSender->remoteEndpoint) { OIC_LOG(ERROR, CALEADAPTER_TAG, "remoteEndpoint is NULL!"); - OICFree(newSender->defragData); - OICFree(newSender); + CALEFreeSenderInfo(newSender); oc_mutex_unlock(bleReceiveDataMutex); return; } @@ -1024,9 +1107,7 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp if (newSender->recvDataLen + dataOnlyLen > newSender->totalDataLen) { OIC_LOG(ERROR, CALEADAPTER_TAG, "buffer is smaller than received data"); - OICFree(newSender->defragData); - CAFreeEndpoint(newSender->remoteEndpoint); - OICFree(newSender); + CALEFreeSenderInfo(newSender); oc_mutex_unlock(bleReceiveDataMutex); return; } @@ -1035,22 +1116,16 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp dataOnlyLen); newSender->recvDataLen += dataOnlyLen; - u_arraylist_add(bleData->senderInfo,(void *)newSender); + newSender->refCount = 1; - //Getting newSender index position in bleSenderInfo array list - if (CA_STATUS_OK != - CALEGetSenderInfo(newSender->remoteEndpoint->addr, - newSender->remoteEndpoint->port, - bleData->senderInfo, - NULL, &senderIndex)) + if (!CALEAddSenderInfoToList(bleData->senderInfo, newSender)) { - OIC_LOG(ERROR, CALEADAPTER_TAG, "Existing sender index not found!!"); - OICFree(newSender->defragData); - CAFreeEndpoint(newSender->remoteEndpoint); - OICFree(newSender); + OIC_LOG(ERROR, CALEADAPTER_TAG, "Failed to add sender info!"); + CALEFreeSenderInfo(newSender); oc_mutex_unlock(bleReceiveDataMutex); return; } + senderInfo = newSender; } else @@ -1061,9 +1136,8 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp OIC_LOG_V(ERROR, CALEADAPTER_TAG, "Data Length exceeding error!! Receiving [%zu] total length [%u]", senderInfo->recvDataLen + dataOnlyLen, senderInfo->totalDataLen); - u_arraylist_remove(bleData->senderInfo, senderIndex); - OICFree(senderInfo->defragData); - OICFree(senderInfo); + CALERemoveSenderInfoFromList(bleData->senderInfo, senderInfo); + CALEFreeSenderInfo(senderInfo); oc_mutex_unlock(bleReceiveDataMutex); return; } @@ -1082,10 +1156,8 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp if (NULL == g_networkPacketReceivedCallback) { OIC_LOG(ERROR, CALEADAPTER_TAG, "gReqRespCallback is NULL!"); - - u_arraylist_remove(bleData->senderInfo, senderIndex); - OICFree(senderInfo->defragData); - OICFree(senderInfo); + CALERemoveSenderInfoFromList(bleData->senderInfo, senderInfo); + CALEFreeSenderInfo(senderInfo); oc_mutex_unlock(bleReceiveDataMutex); return; } @@ -1112,10 +1184,10 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp break; default: OIC_LOG_V(ERROR, CALEADAPTER_TAG, "Unsupported rcvr type:%d",receiverType); - u_arraylist_remove(bleData->senderInfo, senderIndex); + CALERemoveSenderInfoFromList(bleData->senderInfo, senderInfo); senderInfo->remoteEndpoint = NULL; senderInfo->defragData = NULL; - OICFree(senderInfo); + CALEFreeSenderInfo(senderInfo); oc_mutex_unlock(bleReceiveDataMutex); return; } @@ -1143,11 +1215,14 @@ static void CALEDataReceiverHandler(void *threadData, CABLEAdapter_t receiverTyp } #endif - u_arraylist_remove(bleData->senderInfo, senderIndex); + CALERemoveSenderInfoFromList(bleData->senderInfo, senderInfo); senderInfo->remoteEndpoint = NULL; senderInfo->defragData = NULL; - OICFree(senderInfo); + CALEFreeSenderInfo(senderInfo); + oc_mutex_unlock(bleReceiveDataMutex); + return; } + CALEFreeSenderInfo(senderInfo); } oc_mutex_unlock(bleReceiveDataMutex); } @@ -2154,6 +2229,16 @@ static CAResult_t CAInitLEAdapterMutex() } } + if (NULL == g_senderInfoMutex) + { + g_senderInfoMutex = oc_mutex_new(); + if (NULL == g_senderInfoMutex) + { + OIC_LOG(ERROR, CALEADAPTER_TAG, "oc_mutex_new failed"); + return CA_STATUS_FAILED; + } + } + return CA_STATUS_OK; } @@ -2168,13 +2253,14 @@ static void CATerminateLEAdapterMutex() oc_mutex_free(g_bleLocalAddressMutex); g_bleLocalAddressMutex = NULL; - - oc_mutex_free(g_bleServerReceiveDataMutex); g_bleServerReceiveDataMutex = NULL; oc_mutex_free(g_bleClientReceiveDataMutex); g_bleClientReceiveDataMutex = NULL; + + oc_mutex_free(g_senderInfoMutex); + g_senderInfoMutex = NULL; } /** @@ -3618,13 +3704,10 @@ static void CALERemoveReceiveQueueData(u_arraylist_t *dataInfoList, const char* dataInfoList, &senderInfo, &senderIndex)) { - u_arraylist_remove(dataInfoList, senderIndex); - OICFree(senderInfo->defragData); - OICFree(senderInfo->remoteEndpoint); - OICFree(senderInfo); - OIC_LOG(DEBUG, CALEADAPTER_TAG, "SenderInfo is removed for disconnection"); + CALERemoveSenderInfoFromList(dataInfoList, senderInfo); + CALEFreeSenderInfo(senderInfo); } else { -- 2.7.4