From a803fcca92e39b0289d6eea19c249e91a415a92b Mon Sep 17 00:00:00 2001 From: Divya Sampath Kumar Date: Tue, 10 Oct 2023 10:53:38 -0700 Subject: [PATCH] API unit tests, move acquire to before null check --- src/source/Crypto/Dtls_openssl.c | 24 +++++++------- src/source/PeerConnection/PeerConnection.c | 12 +++---- tst/DtlsApiTest.cpp | 37 ++++++++++++++++++++++ 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/src/source/Crypto/Dtls_openssl.c b/src/source/Crypto/Dtls_openssl.c index d02d363ac3..9ba7731c42 100644 --- a/src/source/Crypto/Dtls_openssl.c +++ b/src/source/Crypto/Dtls_openssl.c @@ -55,8 +55,8 @@ STATUS dtlsTransmissionTimerCallback(UINT32 timerID, UINT64 currentTime, UINT64 UINT64 timeoutValDefaultTimeUnit = 0; LONG dtlsTimeoutRet = 0; - CHK(pDtlsSession != NULL, STATUS_NULL_ARG); acquireDtlsSession(pDtlsSession); + CHK(pDtlsSession != NULL, STATUS_NULL_ARG); MEMSET(&timeout, 0x00, SIZEOF(struct timeval)); @@ -386,6 +386,7 @@ STATUS beginHandshakeProcess(PDtlsSession pDtlsSession, BOOL isServer, PINT32 ss ENTERS(); STATUS retStatus = STATUS_SUCCESS; + acquireDtlsSession(pDtlsSession); CHK(pDtlsSession != NULL, STATUS_NULL_ARG); CHK(!ATOMIC_LOAD_BOOL(&pDtlsSession->isStarted), retStatus); @@ -408,6 +409,7 @@ STATUS beginHandshakeProcess(PDtlsSession pDtlsSession, BOOL isServer, PINT32 ss *sslRet = SSL_do_handshake(pDtlsSession->pSsl); CleanUp: CHK_LOG_ERR(retStatus); + releaseDtlsSession(pDtlsSession); LEAVES(); return retStatus; } @@ -419,9 +421,8 @@ STATUS dtlsSessionStart(PDtlsSession pDtlsSession, BOOL isServer) BOOL locked = FALSE; INT32 sslRet; - CHK(pDtlsSession != NULL && pDtlsSession != NULL, STATUS_NULL_ARG); - acquireDtlsSession(pDtlsSession); + CHK(pDtlsSession != NULL, STATUS_NULL_ARG); MUTEX_LOCK(pDtlsSession->sslLock); locked = TRUE; @@ -453,9 +454,9 @@ STATUS dtlsSessionHandshakeInThread(PDtlsSession pDtlsSession, BOOL isServer) BOOL dtlsHandshakeErrored = FALSE; BOOL timedOut = FALSE; MEMSET(&timeout, 0x00, SIZEOF(struct timeval)); - CHK(pDtlsSession != NULL && pDtlsSession != NULL, STATUS_NULL_ARG); acquireDtlsSession(pDtlsSession); + CHK(pDtlsSession != NULL, STATUS_NULL_ARG); MUTEX_LOCK(pDtlsSession->sslLock); locked = TRUE; @@ -594,8 +595,8 @@ STATUS dtlsSessionProcessPacket(PDtlsSession pDtlsSession, PBYTE pData, PINT32 p INT32 sslRet = 0, sslErr; INT32 dataLen = 0; - CHK(pDtlsSession != NULL && pDtlsSession != NULL && pDataLen != NULL, STATUS_NULL_ARG); acquireDtlsSession(pDtlsSession); + CHK(pDtlsSession != NULL && pDataLen != NULL, STATUS_NULL_ARG); CHK(ATOMIC_LOAD_BOOL(&pDtlsSession->isStarted), STATUS_SSL_PACKET_BEFORE_DTLS_READY); MUTEX_LOCK(pDtlsSession->sslLock); @@ -662,9 +663,8 @@ STATUS dtlsSessionPutApplicationData(PDtlsSession pDtlsSession, PBYTE pData, INT SIZE_T pending; BOOL locked = FALSE; - CHK(pDtlsSession != NULL && pData != NULL, STATUS_NULL_ARG); - acquireDtlsSession(pDtlsSession); + CHK(pDtlsSession != NULL && pData != NULL, STATUS_NULL_ARG); MUTEX_LOCK(pDtlsSession->sslLock); locked = TRUE; @@ -698,9 +698,9 @@ STATUS dtlsSessionShutdown(PDtlsSession pDtlsSession) STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; + acquireDtlsSession(pDtlsSession); CHK(pDtlsSession != NULL, STATUS_NULL_ARG); - acquireDtlsSession(pDtlsSession); MUTEX_LOCK(pDtlsSession->sslLock); locked = TRUE; @@ -755,9 +755,9 @@ STATUS dtlsSessionIsInitFinished(PDtlsSession pDtlsSession, PBOOL pIsConnected) STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; + acquireDtlsSession(pDtlsSession); CHK(pDtlsSession != NULL && pIsConnected != NULL, STATUS_NULL_ARG); - acquireDtlsSession(pDtlsSession); MUTEX_LOCK(pDtlsSession->sslLock); locked = TRUE; *pIsConnected = SSL_is_init_finished(pDtlsSession->pSsl); @@ -786,9 +786,9 @@ STATUS dtlsSessionPopulateKeyingMaterial(PDtlsSession pDtlsSession, PDtlsKeyingM BYTE keyingMaterialBuffer[MAX_SRTP_MASTER_KEY_LEN * 2 + MAX_SRTP_SALT_KEY_LEN * 2]; BOOL locked = FALSE; + acquireDtlsSession(pDtlsSession); CHK(pDtlsSession != NULL && pDtlsKeyingMaterial != NULL, STATUS_NULL_ARG); - acquireDtlsSession(pDtlsSession); MUTEX_LOCK(pDtlsSession->sslLock); locked = TRUE; @@ -834,9 +834,9 @@ STATUS dtlsSessionGetLocalCertificateFingerprint(PDtlsSession pDtlsSession, PCHA STATUS retStatus = STATUS_SUCCESS; BOOL locked = FALSE; + acquireDtlsSession(pDtlsSession); CHK(pDtlsSession != NULL && pBuff != NULL, STATUS_NULL_ARG); - acquireDtlsSession(pDtlsSession); CHK(buffLen >= CERTIFICATE_FINGERPRINT_LENGTH, STATUS_INVALID_ARG_LEN); MUTEX_LOCK(pDtlsSession->sslLock); @@ -862,9 +862,9 @@ STATUS dtlsSessionVerifyRemoteCertificateFingerprint(PDtlsSession pDtlsSession, X509* pRemoteCertificate = NULL; BOOL locked = FALSE; + acquireDtlsSession(pDtlsSession); CHK(pDtlsSession != NULL && pExpectedFingerprint != NULL, STATUS_NULL_ARG); - acquireDtlsSession(pDtlsSession); MUTEX_LOCK(pDtlsSession->sslLock); locked = TRUE; diff --git a/src/source/PeerConnection/PeerConnection.c b/src/source/PeerConnection/PeerConnection.c index b16b6005f8..67b8316eb5 100644 --- a/src/source/PeerConnection/PeerConnection.c +++ b/src/source/PeerConnection/PeerConnection.c @@ -439,7 +439,11 @@ STATUS onFrameDroppedFunc(UINT64 customData, UINT16 startIndex, UINT16 endIndex, PVOID dtlsSessionStartThread(PVOID args) { PKvsPeerConnection pKvsPeerConnection = (PKvsPeerConnection) args; - dtlsSessionHandshakeInThread(pKvsPeerConnection->pDtlsSession, pKvsPeerConnection->dtlsIsServer); + if (pKvsPeerConnection != NULL) { + dtlsSessionHandshakeInThread(pKvsPeerConnection->pDtlsSession, pKvsPeerConnection->dtlsIsServer); + } else { + DLOGE("Peer connection object NULL, cannot start DTLS handshake"); + } return NULL; } @@ -496,12 +500,8 @@ VOID onIceConnectionStateChange(UINT64 customData, UINT64 connectionState) // wait until DTLS state changes to CONNECTED. // // Reference: https://w3c.github.io/webrtc-pc/#rtcpeerconnectionstate-enum -#ifdef ENABLE_KVS_THREADPOOL -#ifdef KVS_USE_OPENSSL +#if defined(ENABLE_KVS_THREADPOOL) && defined(KVS_USE_OPENSSL) CHK_STATUS(threadpoolContextPush(dtlsSessionStartThread, (PVOID) pKvsPeerConnection)); -#else - CHK_STATUS(dtlsSessionStart(pKvsPeerConnection->pDtlsSession, pKvsPeerConnection->dtlsIsServer)); -#endif #else CHK_STATUS(dtlsSessionStart(pKvsPeerConnection->pDtlsSession, pKvsPeerConnection->dtlsIsServer)); #endif diff --git a/tst/DtlsApiTest.cpp b/tst/DtlsApiTest.cpp index 4c985dd1ee..4fb2bb512c 100644 --- a/tst/DtlsApiTest.cpp +++ b/tst/DtlsApiTest.cpp @@ -24,6 +24,43 @@ TEST_F(DtlsApiTest, createCertificateAndKey_Returns_Success) EXPECT_EQ(pKey, nullptr); } +TEST_F(DtlsApiTest, dtlsSessionIsInitFinished_Null_Check) +{ + PDtlsSession pClient = NULL; + BOOL isDtlsConnected = FALSE; + EXPECT_EQ(STATUS_NULL_ARG, dtlsSessionIsInitFinished(pClient, &isDtlsConnected)); + EXPECT_EQ(FALSE, isDtlsConnected); +} + +TEST_F(DtlsApiTest, dtlsSessionCreated_RefCount) +{ + DtlsSessionCallbacks callbacks; + PDtlsSession pClient = NULL; + TIMER_QUEUE_HANDLE timerQueueHandle = INVALID_TIMER_QUEUE_HANDLE_VALUE; + EXPECT_EQ(STATUS_SUCCESS, timerQueueCreate(&timerQueueHandle)); + EXPECT_EQ(STATUS_SUCCESS, createDtlsSession(&callbacks, timerQueueHandle, 0, FALSE, NULL, &pClient)); + EXPECT_EQ(0, pClient->objRefCount); + freeDtlsSession(&pClient); + EXPECT_EQ(NULL, pClient); + timerQueueFree(&timerQueueHandle); +} + +TEST_F(DtlsApiTest, dtlsProcessPacket_Api_Check) +{ + DtlsSessionCallbacks callbacks; + PDtlsSession pClient = NULL; + BOOL isDtlsConnected = FALSE; + INT32 length; + TIMER_QUEUE_HANDLE timerQueueHandle = INVALID_TIMER_QUEUE_HANDLE_VALUE; + EXPECT_EQ(STATUS_NULL_ARG, dtlsSessionProcessPacket(pClient, NULL, &length)); + EXPECT_EQ(STATUS_SUCCESS, timerQueueCreate(&timerQueueHandle)); + EXPECT_EQ(STATUS_SUCCESS, createDtlsSession(&callbacks, timerQueueHandle, 0, FALSE, NULL, &pClient)); + EXPECT_EQ(STATUS_NULL_ARG, dtlsSessionProcessPacket(pClient, NULL, NULL)); + EXPECT_EQ(STATUS_SSL_PACKET_BEFORE_DTLS_READY, dtlsSessionProcessPacket(pClient, NULL, &length)); + freeDtlsSession(&pClient); + timerQueueFree(&timerQueueHandle); +} + #elif KVS_USE_MBEDTLS TEST_F(DtlsApiTest, createCertificateAndKey_Returns_Success) {