0402cb32d
Denys Vlasenko
2017-10-22
Subject
bunzip2: fix runCnt overflow from bug 10431
Binary
No binary (build failed)
Architecture & Compiler Flags
—
Build Log
(log yok)
Commit Diff GitHub →
archival/libarchive/decompress_bunzip2.c+19 -11
| @@ -156,15 +156,15 @@ static unsigned get_bits(bunzip_data *bd, int bits_wanted) | |
| static int get_next_block(bunzip_data *bd) | |
| { | |
| struct group_data *hufGroup; | |
| - | int dbufCount, dbufSize, groupCount, *base, *limit, selector, |
| - | i, j, runPos, symCount, symTotal, nSelectors, byteCount[256]; |
| - | int runCnt = runCnt; /* for compiler */ |
| + | int groupCount, *base, *limit, selector, |
| + | i, j, symCount, symTotal, nSelectors, byteCount[256]; |
| uint8_t uc, symToByte[256], mtfSymbol[256], *selectors; | |
| uint32_t *dbuf; | |
| unsigned origPtr, t; | |
| + | unsigned dbufCount, runPos; |
| + | unsigned runCnt = runCnt; /* for compiler */ |
| dbuf = bd->dbuf; | |
| - | dbufSize = bd->dbufSize; |
| selectors = bd->selectors; | |
| /* In bbox, we are ok with aborting through setjmp which is set up in start_bunzip */ | |
| @@ -187,7 +187,7 @@ static int get_next_block(bunzip_data *bd) | |
| it didn't actually work. */ | |
| if (get_bits(bd, 1)) return RETVAL_OBSOLETE_INPUT; | |
| origPtr = get_bits(bd, 24); | |
| - | if ((int)origPtr > dbufSize) return RETVAL_DATA_ERROR; |
| + | if (origPtr > bd->dbufSize) return RETVAL_DATA_ERROR; |
| /* mapping table: if some byte values are never used (encoding things | |
| like ascii text), the compression code removes the gaps to have fewer | |
| @@ -435,7 +435,14 @@ static int get_next_block(bunzip_data *bd) | |
| symbols, but a run of length 0 doesn't mean anything in this | |
| context). Thus space is saved. */ | |
| runCnt += (runPos << nextSym); /* +runPos if RUNA; +2*runPos if RUNB */ | |
| - | if (runPos < dbufSize) runPos <<= 1; |
| + | //The 32-bit overflow of runCnt wasn't yet seen, but probably can happen. |
| + | //This would be the fix (catches too large count way before it can overflow): |
| + | // if (runCnt > bd->dbufSize) { |
| + | // dbg("runCnt:%u > dbufSize:%u RETVAL_DATA_ERROR", |
| + | // runCnt, bd->dbufSize); |
| + | // return RETVAL_DATA_ERROR; |
| + | // } |
| + | if (runPos < bd->dbufSize) runPos <<= 1; |
| goto end_of_huffman_loop; | |
| } | |
| @@ -445,14 +452,15 @@ static int get_next_block(bunzip_data *bd) | |
| literal used is the one at the head of the mtfSymbol array.) */ | |
| if (runPos != 0) { | |
| uint8_t tmp_byte; | |
| - | if (dbufCount + runCnt > dbufSize) { |
| - | dbg("dbufCount:%d+runCnt:%d %d > dbufSize:%d RETVAL_DATA_ERROR", |
| - | dbufCount, runCnt, dbufCount + runCnt, dbufSize); |
| + | if (dbufCount + runCnt > bd->dbufSize) { |
| + | dbg("dbufCount:%u+runCnt:%u %u > dbufSize:%u RETVAL_DATA_ERROR", |
| + | dbufCount, runCnt, dbufCount + runCnt, bd->dbufSize); |
| return RETVAL_DATA_ERROR; | |
| } | |
| tmp_byte = symToByte[mtfSymbol[0]]; | |
| byteCount[tmp_byte] += runCnt; | |
| - | while (--runCnt >= 0) dbuf[dbufCount++] = (uint32_t)tmp_byte; |
| + | while ((int)--runCnt >= 0) |
| + | dbuf[dbufCount++] = (uint32_t)tmp_byte; |
| runPos = 0; | |
| } | |
| @@ -466,7 +474,7 @@ static int get_next_block(bunzip_data *bd) | |
| first symbol in the mtf array, position 0, would have been handled | |
| as part of a run above. Therefore 1 unused mtf position minus | |
| 2 non-literal nextSym values equals -1.) */ | |
| - | if (dbufCount >= dbufSize) return RETVAL_DATA_ERROR; |
| + | if (dbufCount >= bd->dbufSize) return RETVAL_DATA_ERROR; |
| i = nextSym - 1; | |
| uc = mtfSymbol[i]; | |