From: Nick Terrell Date: Fri, 8 Oct 2021 18:45:30 +0000 (-0700) Subject: [binary-tree] Fix underflow of nbCompares X-Git-Tag: v1.5.1~1^2~79^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F2820%2Fhead;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 c76fa840f..7e44b8282 100644 --- a/lib/compress/zstd_lazy.c +++ b/lib/compress/zstd_lazy.c @@ -93,7 +93,7 @@ ZSTD_insertDUBT1(const 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, @@ -758,6 +759,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) { ml = ZSTD_dedicatedDictSearch_lazy_search(offsetPtr, ml, nbAttempts, dms, ip, iLimit, prefixStart, curr, dictLimit, ddsIdx); @@ -1056,7 +1058,7 @@ FORCE_INLINE_TEMPLATE void ZSTD_row_update_internal(ZSTD_matchState_t* ms, const const U32 kMaxMatchEndPositionsToUpdate = 32; if (useCache) { - /* Only skip positions when using hash cache, i.e. + /* Only skip positions when using hash cache, i.e. * if we are loading a dict, don't skip anything. * If we decide to skip, then we only update a set number * of positions at the beginning and end of the match. @@ -1356,6 +1358,7 @@ size_t ZSTD_RowFindBestMatch_generic ( } } + assert(nbAttempts <= (1U << ZSTD_SEARCHLOG_MAX)); /* Check we haven't underflowed. */ if (dictMode == ZSTD_dedicatedDictSearch) { ml = ZSTD_dedicatedDictSearch_lazy_search(offsetPtr, ml, nbAttempts + ddsExtraAttempts, dms, ip, iLimit, prefixStart, curr, dictLimit, ddsIdx); diff --git a/lib/compress/zstd_opt.c b/lib/compress/zstd_opt.c index 4c765128a..909e06d05 100644 --- a/lib/compress/zstd_opt.c +++ b/lib/compress/zstd_opt.c @@ -442,7 +442,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); @@ -673,7 +673,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 */ @@ -725,12 +725,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 2538728c8..ec4a28025 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, 41393 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 @@ -391,9 +391,9 @@ github, level 12 row 2 with dict dds, advanced github, level 12 row 2 with dict copy, advanced one pass, 39677 github, level 12 row 2 with dict load, advanced one pass, 41166 github, level 13, advanced one pass, 134064 -github, level 13 with dict, advanced one pass, 39743 -github, level 13 with dict dms, advanced one pass, 39743 -github, level 13 with dict dds, advanced one pass, 39743 +github, level 13 with dict, advanced one pass, 39900 +github, level 13 with dict dms, advanced one pass, 39900 +github, level 13 with dict dds, advanced one pass, 39900 github, level 13 with dict copy, advanced one pass, 39948 github, level 13 with dict load, advanced one pass, 42626 github, level 16, advanced one pass, 134064 @@ -517,8 +517,8 @@ github.tar, level 12 row 2 with dict copy, advanced github.tar, level 12 row 2 with dict load, advanced one pass, 36459 github.tar, level 13, advanced one pass, 35501 github.tar, level 13 with dict, advanced one pass, 37130 -github.tar, level 13 with dict dms, advanced one pass, 37267 -github.tar, level 13 with dict dds, advanced one pass, 37267 +github.tar, level 13 with dict dms, advanced one pass, 37220 +github.tar, level 13 with dict dds, advanced one pass, 37220 github.tar, level 13 with dict copy, advanced one pass, 37130 github.tar, level 13 with dict load, advanced one pass, 36010 github.tar, level 16, advanced one pass, 40471 @@ -709,9 +709,9 @@ github, level 12 row 2 with dict dds, advanced github, level 12 row 2 with dict copy, advanced one pass small out, 39677 github, level 12 row 2 with dict load, advanced one pass small out, 41166 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 dms, advanced one pass small out, 39743 -github, level 13 with dict dds, advanced one pass small out, 39743 +github, level 13 with dict, advanced one pass small out, 39900 +github, level 13 with dict dms, advanced one pass small out, 39900 +github, level 13 with dict dds, advanced one pass small out, 39900 github, level 13 with dict copy, advanced one pass small out, 39948 github, level 13 with dict load, advanced one pass small out, 42626 github, level 16, advanced one pass small out, 134064 @@ -835,8 +835,8 @@ github.tar, level 12 row 2 with dict copy, advanced github.tar, level 12 row 2 with dict load, advanced one pass small out, 36459 github.tar, level 13, advanced one pass small out, 35501 github.tar, level 13 with dict, advanced one pass small out, 37130 -github.tar, level 13 with dict dms, advanced one pass small out, 37267 -github.tar, level 13 with dict dds, advanced one pass small out, 37267 +github.tar, level 13 with dict dms, advanced one pass small out, 37220 +github.tar, level 13 with dict dds, advanced one pass small out, 37220 github.tar, level 13 with dict copy, advanced one pass small out, 37130 github.tar, level 13 with dict load, advanced one pass small out, 36010 github.tar, level 16, advanced one pass small out, 40471 @@ -1027,9 +1027,9 @@ github, level 12 row 2 with dict dds, advanced github, level 12 row 2 with dict copy, advanced streaming, 39677 github, level 12 row 2 with dict load, advanced streaming, 41166 github, level 13, advanced streaming, 134064 -github, level 13 with dict, advanced streaming, 39743 -github, level 13 with dict dms, advanced streaming, 39743 -github, level 13 with dict dds, advanced streaming, 39743 +github, level 13 with dict, advanced streaming, 39900 +github, level 13 with dict dms, advanced streaming, 39900 +github, level 13 with dict dds, advanced streaming, 39900 github, level 13 with dict copy, advanced streaming, 39948 github, level 13 with dict load, advanced streaming, 42626 github, level 16, advanced streaming, 134064 @@ -1153,8 +1153,8 @@ github.tar, level 12 row 2 with dict copy, advanced github.tar, level 12 row 2 with dict load, advanced streaming, 36459 github.tar, level 13, advanced streaming, 35501 github.tar, level 13 with dict, advanced streaming, 37130 -github.tar, level 13 with dict dms, advanced streaming, 37267 -github.tar, level 13 with dict dds, advanced streaming, 37267 +github.tar, level 13 with dict dms, advanced streaming, 37220 +github.tar, level 13 with dict dds, advanced streaming, 37220 github.tar, level 13 with dict copy, advanced streaming, 37130 github.tar, level 13 with dict load, advanced streaming, 36010 github.tar, level 16, advanced streaming, 40471 @@ -1241,7 +1241,7 @@ github, level 7 with dict, old stre github, level 9, old streaming, 135122 github, level 9 with dict, old streaming, 39437 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 @@ -1359,7 +1359,7 @@ github, level 7 with dict, old stre github, level 9, old streaming advanced, 138676 github, level 9 with dict, old streaming advanced, 38981 github, level 13, old streaming advanced, 138676 -github, level 13 with dict, old streaming advanced, 39721 +github, level 13 with dict, old streaming advanced, 39725 github, level 16, old streaming advanced, 138676 github, level 16 with dict, old streaming advanced, 40789 github, level 19, old streaming advanced, 134064 @@ -1429,7 +1429,7 @@ github, level 5 with dict, old stre github, level 6 with dict, old streaming cdict, 38671 github, level 7 with dict, old streaming cdict, 38758 github, level 9 with dict, old streaming cdict, 39437 -github, level 13 with dict, old streaming cdict, 39743 +github, level 13 with dict, old streaming cdict, 39900 github, level 16 with dict, old streaming cdict, 37577 github, level 19 with dict, old streaming cdict, 37576 github, no source size with dict, old streaming cdict, 40654 @@ -1459,7 +1459,7 @@ github, level 5 with dict, old stre github, level 6 with dict, old streaming advanced cdict, 39363 github, level 7 with dict, old streaming advanced cdict, 38924 github, level 9 with dict, old streaming advanced cdict, 38981 -github, level 13 with dict, old streaming advanced cdict, 39721 +github, level 13 with dict, old streaming advanced cdict, 39725 github, level 16 with dict, old streaming advanced cdict, 40789 github, level 19 with dict, old streaming advanced cdict, 37576 github, no source size with dict, old streaming advanced cdict, 40608