API unit tests, move acquire to before null check

awslabs · Oct 10, 2023 · a803fcc · a803fcc
1 parent edcd6e3
commit a803fcc
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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)
 {