Addressing error handling concerns for an edge case (#1443)

* Addressing error handling concerns for an edge case * Clang format * Removed useless status_success check, changed test to look for correct error value
awslabs · Apr 5, 2022 · 866aee4 · 866aee4
1 parent 3eb3194
commit 866aee4
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 12 deletions.
diff --git a/src/source/PeerConnection/JitterBuffer.c b/src/source/PeerConnection/JitterBuffer.c
@@ -221,12 +221,14 @@ STATUS jitterBufferInternalParse(PJitterBuffer pJitterBuffer, BOOL bufferClosed)
         } else {
             lastNonNullIndex = index;
             retStatus = hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue);
-            pCurPacket = (PRtpPacket) hashValue;
-            if (retStatus == STATUS_SUCCESS || retStatus == STATUS_HASH_KEY_NOT_PRESENT) {
-                retStatus = STATUS_SUCCESS;
+            if (retStatus == STATUS_HASH_KEY_NOT_PRESENT) {
+                // should be unreachable, this means hashTablContains() said we had it but hashTableGet() couldn't find it.
+                continue;
             } else {
-                CHK(FALSE, retStatus);
+                CHK_STATUS(retStatus);
             }
+            pCurPacket = (PRtpPacket) hashValue;
+            CHK(pCurPacket != NULL, STATUS_NULL_ARG);
             curTimestamp = pCurPacket->header.timestamp;
             // new timestamp on an RTP packet means new frame
             if (curTimestamp != pJitterBuffer->headTimestamp) {
@@ -315,7 +317,9 @@ STATUS jitterBufferDropBufferData(PJitterBuffer pJitterBuffer, UINT16 startIndex
         if (hasEntry) {
             CHK_STATUS(hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue));
             pCurPacket = (PRtpPacket) hashValue;
-            freeRtpPacket(&pCurPacket);
+            if (pCurPacket) {
+                freeRtpPacket(&pCurPacket);
+            }
             CHK_STATUS(hashTableRemove(pJitterBuffer->pPkgBufferHashTable, index));
         }
     }
@@ -344,13 +348,8 @@ STATUS jitterBufferFillFrameData(PJitterBuffer pJitterBuffer, PBYTE pFrame, UINT
     CHK(pJitterBuffer != NULL && pFrame != NULL && pFilledSize != NULL, STATUS_NULL_ARG);
     for (; UINT16_DEC(index) != endIndex; index++) {
         hashValue = 0;
-        retStatus = hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue);
+        CHK_STATUS(hashTableGet(pJitterBuffer->pPkgBufferHashTable, index, &hashValue));
         pCurPacket = (PRtpPacket) hashValue;
-        if (retStatus == STATUS_SUCCESS || retStatus == STATUS_HASH_KEY_NOT_PRESENT) {
-            retStatus = STATUS_SUCCESS;
-        } else {
-            CHK(FALSE, retStatus);
-        }
         CHK(pCurPacket != NULL, STATUS_NULL_ARG);
         partialFrameSize = remainingFrameSize;
         CHK_STATUS(pJitterBuffer->depayPayloadFn(pCurPacket->payload, pCurPacket->payloadLength, pCurPtrInFrame, &partialFrameSize, NULL));

diff --git a/tst/JitterBufferFunctionalityTest.cpp b/tst/JitterBufferFunctionalityTest.cpp
@@ -448,7 +448,7 @@ TEST_F(JitterBufferFunctionalityTest, fillDataReturnErrorWithImcompleteFrame)
         EXPECT_EQ(STATUS_SUCCESS, jitterBufferPush(mJitterBuffer, mPRtpPackets[i], nullptr));
     }
 
-    EXPECT_EQ(STATUS_NULL_ARG, jitterBufferFillFrameData(mJitterBuffer, buffer, 2, &filledSize, 0, 1));
+    EXPECT_EQ(STATUS_HASH_KEY_NOT_PRESENT, jitterBufferFillFrameData(mJitterBuffer, buffer, 2, &filledSize, 0, 1));
 
     clearJitterBufferForTest();
     MEMFREE(buffer);