Issue 66056: openh264:decoder_fuzzer: Integer-overflow in WelsDec::ParseSliceHeaderSyntaxs

[BenzhengZhang]

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=66056&q=label%3AProj-openh264&can=2

[BenzhengZhang]

@tyan0 Recently fuzzy test reported two decoder bugs, another #3746 Could you pls take a look this issue? thank you.

[tyan0]

Could you please let me know how to reproduce the issues?

[BenzhengZhang]

@tyan0 you can refer to https://google.github.io/oss-fuzz/advanced-topics/reproducing/

[BenzhengZhang]

Could you please let me know how to reproduce the issues?
I remembered, you may not be able to open the bug report link. https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=66056&q=label%3AProj-openh264&can=2

Reproducer Testcase:
clusterfuzz-testcase-minimized-decoder_fuzzer-4680815741042688 (1).zip

[tyan0]

I'm very sorry for extremely long delay.

Finally, I could reproduce the problems with the testcases:
clusterfuzz-testcase-minimized-decoder_fuzzer-4680815741042688 (#3745)
clusterfuzz-testcase-minimized-decoder_fuzzer-6315387293597696 (#3746)

I'll look into these problems.

[tyan0]

As for #3745, the code in decoder_core.cpp expects that the arithmetic overflow in int32_t results into wrapped-around result. Oss-fuzz complains about that. C/C++ standard states the overflowed result is undefined for signed integer.

The following patch fixes the issue. However, it does not seem smart enough and is difficult to read the intent.

diff --git a/codec/decoder/core/src/decoder_core.cpp b/codec/decoder/core/src/decoder_core.cpp
index 9d8f5eaa..2e280dcb 100644
--- a/codec/decoder/core/src/decoder_core.cpp
+++ b/codec/decoder/core/src/decoder_core.cpp
@@ -1079,16 +1079,34 @@ int32_t ParseSliceHeaderSyntaxs (PWelsDecoderContext pCtx, PBitStringAux pBs, co
       pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb = 0;
       pCtx->pLastDecPicInfo->iPrevPicOrderCntLsb = 0;
     }
+    /* We shoud not expect that overflow in signed integer returns wrapped-
+       around result. C/C++ standard states the result is undefined. */
     int32_t pocMsb;
     if (pocLsb < pCtx->pLastDecPicInfo->iPrevPicOrderCntLsb
         && pCtx->pLastDecPicInfo->iPrevPicOrderCntLsb - pocLsb >= iMaxPocLsb / 2)
-      pocMsb = pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb + iMaxPocLsb;
+      {
+        if (INT32_MAX - iMaxPocLsb < pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb)
+          /* Will overflow. Emulate wraparound result. */
+          pocMsb = (pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb - INT32_MAX - 1) + (INT32_MIN + iMaxPocLsb);
+        else
+          pocMsb = pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb + iMaxPocLsb;
+      }
     else if (pocLsb > pCtx->pLastDecPicInfo->iPrevPicOrderCntLsb
              && pocLsb - pCtx->pLastDecPicInfo->iPrevPicOrderCntLsb > iMaxPocLsb / 2)
-      pocMsb = pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb - iMaxPocLsb;
+      {
+        if (INT32_MIN + iMaxPocLsb > pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb)
+          /* Will overflow. Emulate wraparound result. */
+          pocMsb = (pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb - INT32_MIN) + (INT32_MAX - iMaxPocLsb + 1);
+        else
+          pocMsb = pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb - iMaxPocLsb;
+      }
     else
       pocMsb = pCtx->pLastDecPicInfo->iPrevPicOrderCntMsb;
-    pSliceHead->iPicOrderCntLsb = pocMsb + pocLsb;
+    if (INT32_MAX - pocLsb < pocMsb)
+      /* Will overflow. Emulate wraparound result. */
+      pSliceHead->iPicOrderCntLsb = (pocMsb - INT32_MAX - 1) + (INT32_MIN + pocLsb);
+    else
+      pSliceHead->iPicOrderCntLsb = pocMsb + pocLsb;
 
     if (pPps->bPicOrderPresentFlag && !pSliceHead->bFieldPicFlag) {
       pSliceHead->iPicOrderCntLsb += pSliceHead->iDeltaPicOrderCntBottom;
diff --git a/codec/decoder/plus/src/welsDecoderExt.cpp b/codec/decoder/plus/src/welsDecoderExt.cpp
index 7753505d..6b8ae90d 100644
--- a/codec/decoder/plus/src/welsDecoderExt.cpp
+++ b/codec/decoder/plus/src/welsDecoderExt.cpp
@@ -1052,7 +1052,7 @@ void CWelsDecoder::ReleaseBufferedReadyPictureReorder (PWelsDecoderContext pCtx,
       int32_t iLastPOC = pCtx != NULL ? pCtx->pSliceHeader->iPicOrderCntLsb : m_sPictInfoList[m_iLastBufferedIdx].iPOC;
       int32_t iLastSeqNum = pCtx != NULL ? pCtx->iSeqNum : m_sPictInfoList[m_iLastBufferedIdx].iSeqNum;
       isReady = (m_sReoderingStatus.iLastWrittenPOC > IMinInt32
-        && m_sReoderingStatus.iMinPOC - m_sReoderingStatus.iLastWrittenPOC <= 1)
+        && m_sReoderingStatus.iMinPOC <= m_sReoderingStatus.iLastWrittenPOC + 1)
         || m_sReoderingStatus.iMinPOC < iLastPOC
         || m_sReoderingStatus.iMinSeqNum - iLastSeqNum < 0;
     }

[BenzhengZhang]

So this is a false positive, right? @tyan0

[tyan0]

No, it is not. Current code works as expected on the most systems (where two's complement is used for negative value) with gcc. However, C++ standard inhibit expecting the certain result on overflow on signed integer (the result is UNDEFINED).

Therefore, in some systems (https://stackoverflow.com/questions/12276957/are-there-any-non-twos-complement-implementations-of-c), the code does not work as expected. So, the current code is not portable. It depends on system.

I am not sure if openh264 should be portable even for such systems.

[fermay]

As for #3745, the code in decoder_core.cpp expects that the arithmetic overflow in int32_t results into wrapped-around result. Oss-fuzz complains about that. C/C++ standard states the overflowed result is undefined for signed integer.

i also think not need to modify this code, 2 reasons: 1. if modify it, it will cause the code more hard understand. 2. msb and lsb are hard to exceed int32_t in actually.