From: Nick Terrell Date: Fri, 8 Oct 2021 18:45:30 +0000 (-0700) Subject: [binary-tree] Fix underflow of nbCompares X-Git-Tag: v1.4.10~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c0c38ba1db6a245b944b9c226647c00e5d657c4b;p=thirdparty%2Fzstd.git [binary-tree] Fix underflow of nbCompares Fix underflow of `nbCompares` by switching to an `int` and comparing `nbCompares > 0`. This is a minimal fix, because I don't want to change the logic. These loops seem to be doing `nbCompares + 1` comparisons. The bug was reported by Dan Carpenter and found by Smatch static checker. https://lore.kernel.org/all/20211008063704.GA5370@kili/ --- diff --git a/lib/compress/zstd_lazy.c b/lib/compress/zstd_lazy.c index c5fe6dd47..c71a2c9e6 100644 --- a/lib/compress/zstd_lazy.c +++ b/lib/compress/zstd_lazy.c @@ -93,7 +93,7 @@ ZSTD_insertDUBT1(ZSTD_matchState_t* ms, assert(curr >= btLow); assert(ip < iend); /* condition for ZSTD_count */ - while (nbCompares-- && (matchIndex > windowLow)) { + for (; nbCompares && (matchIndex > windowLow); --nbCompares) { U32* const nextPtr = bt + 2*(matchIndex & btMask); size_t matchLength = MIN(commonLengthSmaller, commonLengthLarger); /* guaranteed minimum nb of common bytes */ assert(matchIndex < curr); @@ -185,7 +185,7 @@ ZSTD_DUBT_findBetterDictMatch ( (void)dictMode; assert(dictMode == ZSTD_dictMatchState); - while (nbCompares-- && (dictMatchIndex > dictLowLimit)) { + for (; nbCompares && (dictMatchIndex > dictLowLimit); --nbCompares) { U32* const nextPtr = dictBt + 2*(dictMatchIndex & btMask); size_t matchLength = MIN(commonLengthSmaller, commonLengthLarger); /* guaranteed minimum nb of common bytes */ const BYTE* match = dictBase + dictMatchIndex; @@ -309,7 +309,7 @@ ZSTD_DUBT_findBestMatch(ZSTD_matchState_t* ms, matchIndex = hashTable[h]; hashTable[h] = curr; /* Update Hash Table */ - while (nbCompares-- && (matchIndex > windowLow)) { + for (; nbCompares && (matchIndex > windowLow); --nbCompares) { U32* const nextPtr = bt + 2*(matchIndex & btMask); size_t matchLength = MIN(commonLengthSmaller, commonLengthLarger); /* guaranteed minimum nb of common bytes */ const BYTE* match; @@ -357,6 +357,7 @@ ZSTD_DUBT_findBestMatch(ZSTD_matchState_t* ms, *smallerPtr = *largerPtr = 0; + assert(nbCompares <= (1U << ZSTD_SEARCHLOG_MAX)); /* Check we haven't underflowed. */ if (dictMode == ZSTD_dictMatchState && nbCompares) { bestLength = ZSTD_DUBT_findBetterDictMatch( ms, ip, iend, @@ -660,6 +661,7 @@ size_t ZSTD_HcFindBestMatch_generic ( matchIndex = NEXT_IN_CHAIN(matchIndex, chainMask); } + assert(nbAttempts <= (1U << ZSTD_SEARCHLOG_MAX)); /* Check we haven't underflowed. */ if (dictMode == ZSTD_dedicatedDictSearch) { const U32 ddsLowestIndex = dms->window.dictLimit; const BYTE* const ddsBase = dms->window.base; diff --git a/lib/compress/zstd_opt.c b/lib/compress/zstd_opt.c index 402a7e5c7..d9fbfef1f 100644 --- a/lib/compress/zstd_opt.c +++ b/lib/compress/zstd_opt.c @@ -408,7 +408,7 @@ static U32 ZSTD_insertBt1( hashTable[h] = curr; /* Update Hash Table */ assert(windowLow > 0); - while (nbCompares-- && (matchIndex >= windowLow)) { + for (; nbCompares && (matchIndex >= windowLow); --nbCompares) { U32* const nextPtr = bt + 2*(matchIndex & btMask); size_t matchLength = MIN(commonLengthSmaller, commonLengthLarger); /* guaranteed minimum nb of common bytes */ assert(matchIndex < curr); @@ -639,7 +639,7 @@ U32 ZSTD_insertBtAndGetAllMatches ( hashTable[h] = curr; /* Update Hash Table */ - while (nbCompares-- && (matchIndex >= matchLow)) { + for (; nbCompares && (matchIndex >= matchLow); --nbCompares) { U32* const nextPtr = bt + 2*(matchIndex & btMask); const BYTE* match; size_t matchLength = MIN(commonLengthSmaller, commonLengthLarger); /* guaranteed minimum nb of common bytes */ @@ -692,12 +692,13 @@ U32 ZSTD_insertBtAndGetAllMatches ( *smallerPtr = *largerPtr = 0; + assert(nbCompares <= (1U << ZSTD_SEARCHLOG_MAX)); /* Check we haven't underflowed. */ if (dictMode == ZSTD_dictMatchState && nbCompares) { size_t const dmsH = ZSTD_hashPtr(ip, dmsHashLog, mls); U32 dictMatchIndex = dms->hashTable[dmsH]; const U32* const dmsBt = dms->chainTable; commonLengthSmaller = commonLengthLarger = 0; - while (nbCompares-- && (dictMatchIndex > dmsLowLimit)) { + for (; nbCompares && (dictMatchIndex > dmsLowLimit); --nbCompares) { const U32* const nextPtr = dmsBt + 2*(dictMatchIndex & dmsBtMask); size_t matchLength = MIN(commonLengthSmaller, commonLengthLarger); /* guaranteed minimum nb of common bytes */ const BYTE* match = dmsBase + dictMatchIndex; diff --git a/tests/regression/results.csv b/tests/regression/results.csv index 0fb1d160a..f0edea222 100644 --- a/tests/regression/results.csv +++ b/tests/regression/results.csv @@ -171,7 +171,7 @@ github, level 7 with dict, zstdcli, github, level 9, zstdcli, 137122 github, level 9 with dict, zstdcli, 41332 github, level 13, zstdcli, 136064 -github, level 13 with dict, zstdcli, 41743 +github, level 13 with dict, zstdcli, 41900 github, level 16, zstdcli, 136064 github, level 16 with dict, zstdcli, 39577 github, level 19, zstdcli, 136064 @@ -303,7 +303,7 @@ github, level 7 with dict, advanced github, level 9, advanced one pass, 135122 github, level 9 with dict, advanced one pass, 39332 github, level 13, advanced one pass, 134064 -github, level 13 with dict, advanced one pass, 39743 +github, level 13 with dict, advanced one pass, 39900 github, level 16, advanced one pass, 134064 github, level 16 with dict, advanced one pass, 37577 github, level 19, advanced one pass, 134064 @@ -437,7 +437,7 @@ github, level 7 with dict, advanced github, level 9, advanced one pass small out, 135122 github, level 9 with dict, advanced one pass small out, 39332 github, level 13, advanced one pass small out, 134064 -github, level 13 with dict, advanced one pass small out, 39743 +github, level 13 with dict, advanced one pass small out, 39900 github, level 16, advanced one pass small out, 134064 github, level 16 with dict, advanced one pass small out, 37577 github, level 19, advanced one pass small out, 134064 @@ -571,7 +571,7 @@ github, level 7 with dict, advanced github, level 9, advanced streaming, 135122 github, level 9 with dict, advanced streaming, 39332 github, level 13, advanced streaming, 134064 -github, level 13 with dict, advanced streaming, 39743 +github, level 13 with dict, advanced streaming, 39900 github, level 16, advanced streaming, 134064 github, level 16 with dict, advanced streaming, 37577 github, level 19, advanced streaming, 134064 @@ -689,7 +689,7 @@ github, level 7 with dict, old stre github, level 9, old streaming, 135122 github, level 9 with dict, old streaming, 39332 github, level 13, old streaming, 134064 -github, level 13 with dict, old streaming, 39743 +github, level 13 with dict, old streaming, 39900 github, level 16, old streaming, 134064 github, level 16 with dict, old streaming, 37577 github, level 19, old streaming, 134064 @@ -877,7 +877,7 @@ github, level 5 with dict, old stre github, level 6 with dict, old streaming cdcit, 38632 github, level 7 with dict, old streaming cdcit, 38771 github, level 9 with dict, old streaming cdcit, 39332 -github, level 13 with dict, old streaming cdcit, 39743 +github, level 13 with dict, old streaming cdcit, 39900 github, level 16 with dict, old streaming cdcit, 37577 github, level 19 with dict, old streaming cdcit, 37576 github, no source size with dict, old streaming cdcit, 40654