From 866aee4a5fe7a25fe6be2ddc562e3072348a978c Mon Sep 17 00:00:00 2001
From: jdelapla <delaplan@amazon.com>
Date: Tue, 5 Apr 2022 09:42:28 -0700
Subject: [PATCH] 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
---
 src/source/PeerConnection/JitterBuffer.c | 21 ++++++++++-----------
 tst/JitterBufferFunctionalityTest.cpp    |  2 +-
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/src/source/PeerConnection/JitterBuffer.c b/src/source/PeerConnection/JitterBuffer.c
index 2d919d07c6..aea092edc0 100644
--- 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
index d88717b8da..c857bea78a 100644
--- 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);