From ba48115231b9b5fb6413bc5e12f4adeb9053f465 Mon Sep 17 00:00:00 2001 From: kailixu Date: Fri, 19 Jan 2024 10:24:39 +0800 Subject: [PATCH 1/3] fix: heap user after free --- source/client/inc/clientInt.h | 1 + source/client/src/clientEnv.c | 1 + source/client/src/clientHb.c | 22 +++++++++++++--------- source/client/src/clientMsgHandler.c | 20 ++++++++++++++++++-- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/source/client/inc/clientInt.h b/source/client/inc/clientInt.h index a6d5039be7..fd4776664c 100644 --- a/source/client/inc/clientInt.h +++ b/source/client/inc/clientInt.h @@ -155,6 +155,7 @@ typedef struct STscObj { int8_t biMode; int32_t acctId; uint32_t connId; + int32_t appHbMgrIdx; int64_t id; // ref ID returned by taosAddRef TdThreadMutex mutex; // used to protect the operation on db int32_t numOfReqs; // number of sqlObj bound to this connection diff --git a/source/client/src/clientEnv.c b/source/client/src/clientEnv.c index b6c5701915..f1e9e36433 100644 --- a/source/client/src/clientEnv.c +++ b/source/client/src/clientEnv.c @@ -283,6 +283,7 @@ void *createTscObj(const char *user, const char *auth, const char *db, int32_t c pObj->connType = connType; pObj->pAppInfo = pAppInfo; + pObj->appHbMgrIdx = pAppInfo->pAppHbMgr->idx; tstrncpy(pObj->user, user, sizeof(pObj->user)); memcpy(pObj->pass, auth, TSDB_PASSWORD_LEN); diff --git a/source/client/src/clientHb.c b/source/client/src/clientHb.c index b3d9a9ced5..e076a874d3 100644 --- a/source/client/src/clientHb.c +++ b/source/client/src/clientHb.c @@ -30,7 +30,7 @@ typedef struct { }; } SHbParam; -static SClientHbMgr clientHbMgr = {0}; +SClientHbMgr clientHbMgr = {0}; static int32_t hbCreateThread(); static void hbStopThread(); @@ -1294,9 +1294,8 @@ void hbMgrCleanUp() { taosThreadMutexLock(&clientHbMgr.lock); appHbMgrCleanup(); - taosArrayDestroy(clientHbMgr.appHbMgrs); + clientHbMgr.appHbMgrs = taosArrayDestroy(clientHbMgr.appHbMgrs); taosThreadMutexUnlock(&clientHbMgr.lock); - clientHbMgr.appHbMgrs = NULL; } int hbRegisterConnImpl(SAppHbMgr *pAppHbMgr, SClientHbKey connKey, int64_t clusterId) { @@ -1335,13 +1334,18 @@ int hbRegisterConn(SAppHbMgr *pAppHbMgr, int64_t tscRefId, int64_t clusterId, in } void hbDeregisterConn(STscObj *pTscObj, SClientHbKey connKey) { - SAppHbMgr *pAppHbMgr = pTscObj->pAppInfo->pAppHbMgr; - SClientHbReq *pReq = taosHashAcquire(pAppHbMgr->activeInfo, &connKey, sizeof(SClientHbKey)); - if (pReq) { - tFreeClientHbReq(pReq); - taosHashRemove(pAppHbMgr->activeInfo, &connKey, sizeof(SClientHbKey)); - taosHashRelease(pAppHbMgr->activeInfo, pReq); + SClientHbReq *pReq = NULL; + taosThreadMutexLock(&clientHbMgr.lock); + SAppHbMgr *pAppHbMgr = taosArrayGetP(clientHbMgr.appHbMgrs, pTscObj->appHbMgrIdx); + if (pAppHbMgr) { + pReq = taosHashAcquire(pAppHbMgr->activeInfo, &connKey, sizeof(SClientHbKey)); + if (pReq) { + tFreeClientHbReq(pReq); + taosHashRemove(pAppHbMgr->activeInfo, &connKey, sizeof(SClientHbKey)); + taosHashRelease(pAppHbMgr->activeInfo, pReq); + } } + taosThreadMutexUnlock(&clientHbMgr.lock); if (NULL == pReq) { return; diff --git a/source/client/src/clientMsgHandler.c b/source/client/src/clientMsgHandler.c index e0cedb9924..fbef9dfad4 100644 --- a/source/client/src/clientMsgHandler.c +++ b/source/client/src/clientMsgHandler.c @@ -26,6 +26,8 @@ #include "tname.h" #include "tversion.h" +extern SClientHbMgr clientHbMgr; + static void setErrno(SRequestObj* pRequest, int32_t code) { pRequest->code = code; terrno = code; @@ -63,12 +65,21 @@ int32_t processConnectRsp(void* param, SDataBuf* pMsg, int32_t code) { STscObj* pTscObj = pRequest->pTscObj; - if (NULL == pTscObj->pAppInfo || NULL == pTscObj->pAppInfo->pAppHbMgr) { + if (NULL == pTscObj->pAppInfo) { setErrno(pRequest, TSDB_CODE_TSC_DISCONNECTED); tsem_post(&pRequest->body.rspSem); goto End; } + taosThreadMutexLock(&clientHbMgr.lock); + if (NULL == taosArrayGetP(clientHbMgr.appHbMgrs, pTscObj->appHbMgrIdx)) { + taosThreadMutexUnlock(&clientHbMgr.lock); + setErrno(pRequest, TSDB_CODE_TSC_DISCONNECTED); + tsem_post(&pRequest->body.rspSem); + goto End; + } + taosThreadMutexUnlock(&clientHbMgr.lock); + SConnectRsp connectRsp = {0}; if (tDeserializeSConnectRsp(pMsg->pData, pMsg->len, &connectRsp) != 0) { code = TSDB_CODE_TSC_INVALID_VERSION; @@ -142,7 +153,12 @@ int32_t processConnectRsp(void* param, SDataBuf* pMsg, int32_t code) { pTscObj->authVer = connectRsp.authVer; pTscObj->whiteListInfo.ver = connectRsp.whiteListVer; - hbRegisterConn(pTscObj->pAppInfo->pAppHbMgr, pTscObj->id, connectRsp.clusterId, connectRsp.connType); + taosThreadMutexLock(&clientHbMgr.lock); + SAppHbMgr* pAppHbMgr = taosArrayGetP(clientHbMgr.appHbMgrs, pTscObj->appHbMgrIdx); + if (pAppHbMgr) { + hbRegisterConn(pAppHbMgr, pTscObj->id, connectRsp.clusterId, connectRsp.connType); + } + taosThreadMutexUnlock(&clientHbMgr.lock); tscDebug("0x%" PRIx64 " clusterId:%" PRId64 ", totalConn:%" PRId64, pRequest->requestId, connectRsp.clusterId, pTscObj->pAppInfo->numOfConns); From ff06f9ef4260c92069fa993504da6af00b27bc78 Mon Sep 17 00:00:00 2001 From: kailixu Date: Fri, 19 Jan 2024 14:31:02 +0800 Subject: [PATCH 2/3] fix: heap user after free --- source/client/src/clientMsgHandler.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/source/client/src/clientMsgHandler.c b/source/client/src/clientMsgHandler.c index fbef9dfad4..324b99022b 100644 --- a/source/client/src/clientMsgHandler.c +++ b/source/client/src/clientMsgHandler.c @@ -66,20 +66,12 @@ int32_t processConnectRsp(void* param, SDataBuf* pMsg, int32_t code) { STscObj* pTscObj = pRequest->pTscObj; if (NULL == pTscObj->pAppInfo) { - setErrno(pRequest, TSDB_CODE_TSC_DISCONNECTED); + code = TSDB_CODE_TSC_DISCONNECTED; + setErrno(pRequest, code); tsem_post(&pRequest->body.rspSem); goto End; } - taosThreadMutexLock(&clientHbMgr.lock); - if (NULL == taosArrayGetP(clientHbMgr.appHbMgrs, pTscObj->appHbMgrIdx)) { - taosThreadMutexUnlock(&clientHbMgr.lock); - setErrno(pRequest, TSDB_CODE_TSC_DISCONNECTED); - tsem_post(&pRequest->body.rspSem); - goto End; - } - taosThreadMutexUnlock(&clientHbMgr.lock); - SConnectRsp connectRsp = {0}; if (tDeserializeSConnectRsp(pMsg->pData, pMsg->len, &connectRsp) != 0) { code = TSDB_CODE_TSC_INVALID_VERSION; @@ -106,7 +98,8 @@ int32_t processConnectRsp(void* param, SDataBuf* pMsg, int32_t code) { } if (connectRsp.epSet.numOfEps == 0) { - setErrno(pRequest, TSDB_CODE_APP_ERROR); + code = TSDB_CODE_APP_ERROR; + setErrno(pRequest, code); tsem_post(&pRequest->body.rspSem); goto End; } @@ -157,6 +150,12 @@ int32_t processConnectRsp(void* param, SDataBuf* pMsg, int32_t code) { SAppHbMgr* pAppHbMgr = taosArrayGetP(clientHbMgr.appHbMgrs, pTscObj->appHbMgrIdx); if (pAppHbMgr) { hbRegisterConn(pAppHbMgr, pTscObj->id, connectRsp.clusterId, connectRsp.connType); + } else { + taosThreadMutexUnlock(&clientHbMgr.lock); + code = TSDB_CODE_TSC_DISCONNECTED; + setErrno(pRequest, code); + tsem_post(&pRequest->body.rspSem); + goto End; } taosThreadMutexUnlock(&clientHbMgr.lock); From bc9dfd8a8325bff3a7fc7d081991218a7d333eea Mon Sep 17 00:00:00 2001 From: kailixu Date: Fri, 19 Jan 2024 14:42:02 +0800 Subject: [PATCH 3/3] fix: heap use after free --- source/client/src/clientHb.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/source/client/src/clientHb.c b/source/client/src/clientHb.c index e076a874d3..63a65d7c95 100644 --- a/source/client/src/clientHb.c +++ b/source/client/src/clientHb.c @@ -1334,24 +1334,18 @@ int hbRegisterConn(SAppHbMgr *pAppHbMgr, int64_t tscRefId, int64_t clusterId, in } void hbDeregisterConn(STscObj *pTscObj, SClientHbKey connKey) { - SClientHbReq *pReq = NULL; taosThreadMutexLock(&clientHbMgr.lock); SAppHbMgr *pAppHbMgr = taosArrayGetP(clientHbMgr.appHbMgrs, pTscObj->appHbMgrIdx); if (pAppHbMgr) { - pReq = taosHashAcquire(pAppHbMgr->activeInfo, &connKey, sizeof(SClientHbKey)); + SClientHbReq *pReq = taosHashAcquire(pAppHbMgr->activeInfo, &connKey, sizeof(SClientHbKey)); if (pReq) { tFreeClientHbReq(pReq); taosHashRemove(pAppHbMgr->activeInfo, &connKey, sizeof(SClientHbKey)); taosHashRelease(pAppHbMgr->activeInfo, pReq); + atomic_sub_fetch_32(&pAppHbMgr->connKeyCnt, 1); } } taosThreadMutexUnlock(&clientHbMgr.lock); - - if (NULL == pReq) { - return; - } - - atomic_sub_fetch_32(&pAppHbMgr->connKeyCnt, 1); } // set heart beat thread quit mode , if quicByKill 1 then kill thread else quit from inner