From 3cd642df5f6c274c762c2b1388bdccc9d74f1db2 Mon Sep 17 00:00:00 2001 From: Rostislav Skudnov Date: Wed, 1 Feb 2017 18:35:13 +0000 Subject: [PATCH 1/2] Replace int -> uint to avoid signed integer overflow An example of such an error (should be compiled with DEBUG_SANITIZE): runtime error: left shift of 1 by 31 places cannot be represented in type 'int' Signed-off-by: Rostislav Skudnov Signed-off-by: Denys Vlasenko --- archival/libarchive/decompress_bunzip2.c | 6 +++--- libbb/crc32.c | 2 +- libbb/getopt32.c | 4 ++-- libbb/pw_encrypt.c | 2 +- miscutils/rx.c | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/archival/libarchive/decompress_bunzip2.c b/archival/libarchive/decompress_bunzip2.c index fe5953da2..4fb989c29 100644 --- a/archival/libarchive/decompress_bunzip2.c +++ b/archival/libarchive/decompress_bunzip2.c @@ -134,7 +134,7 @@ static unsigned get_bits(bunzip_data *bd, int bits_wanted) /* Avoid 32-bit overflow (dump bit buffer to top of output) */ if (bit_count >= 24) { - bits = bd->inbufBits & ((1 << bit_count) - 1); + bits = bd->inbufBits & ((1U << bit_count) - 1); bits_wanted -= bit_count; bits <<= bits_wanted; bit_count = 0; @@ -158,11 +158,11 @@ static int get_next_block(bunzip_data *bd) { struct group_data *hufGroup; int dbufCount, dbufSize, groupCount, *base, *limit, selector, - i, j, t, runPos, symCount, symTotal, nSelectors, byteCount[256]; + i, j, runPos, symCount, symTotal, nSelectors, byteCount[256]; int runCnt = runCnt; /* for compiler */ uint8_t uc, symToByte[256], mtfSymbol[256], *selectors; uint32_t *dbuf; - unsigned origPtr; + unsigned origPtr, t; dbuf = bd->dbuf; dbufSize = bd->dbufSize; diff --git a/libbb/crc32.c b/libbb/crc32.c index ac9836cc9..0711ca84e 100644 --- a/libbb/crc32.c +++ b/libbb/crc32.c @@ -24,7 +24,7 @@ uint32_t* FAST_FUNC crc32_filltable(uint32_t *crc_table, int endian) { uint32_t polynomial = endian ? 0x04c11db7 : 0xedb88320; uint32_t c; - int i, j; + unsigned i, j; if (!crc_table) crc_table = xmalloc(256 * sizeof(uint32_t)); diff --git a/libbb/getopt32.c b/libbb/getopt32.c index 15b6efc09..497fc016f 100644 --- a/libbb/getopt32.c +++ b/libbb/getopt32.c @@ -404,7 +404,7 @@ getopt32(char **argv, const char *applet_opts, ...) if (c >= 32) break; on_off->opt_char = *s; - on_off->switch_on = (1 << c); + on_off->switch_on = (1U << c); if (*++s == ':') { on_off->optarg = va_arg(p, void **); if (s[1] == '+' || s[1] == '*') { @@ -454,7 +454,7 @@ getopt32(char **argv, const char *applet_opts, ...) if (c >= 32) break; on_off->opt_char = l_o->val; - on_off->switch_on = (1 << c); + on_off->switch_on = (1U << c); if (l_o->has_arg != no_argument) on_off->optarg = va_arg(p, void **); c++; diff --git a/libbb/pw_encrypt.c b/libbb/pw_encrypt.c index 4cdc2de76..fe06a8fe6 100644 --- a/libbb/pw_encrypt.c +++ b/libbb/pw_encrypt.c @@ -30,7 +30,7 @@ static int i64c(int i) int FAST_FUNC crypt_make_salt(char *p, int cnt /*, int x */) { /* was: x += ... */ - int x = getpid() + monotonic_us(); + unsigned x = getpid() + monotonic_us(); do { /* x = (x*1664525 + 1013904223) % 2^32 generator is lame * (low-order bit is not "random", etc...), diff --git a/miscutils/rx.c b/miscutils/rx.c index 660f66a89..86627e1b5 100644 --- a/miscutils/rx.c +++ b/miscutils/rx.c @@ -94,7 +94,7 @@ static int receive(/*int read_fd, */int file_fd) int blockBegin; int blockNo, blockNoOnesCompl; int cksum_or_crc; - int expected; + unsigned expected; int i, j; blockBegin = read_byte(timeout); -- 2.15.0 From 2be3fc2e5407081a597a99e3a71d55fd673de50f Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Sun, 22 Oct 2017 18:23:23 +0200 Subject: [PATCH 2/2] bunzip2: fix runCnt overflow from bug 10431 This particular corrupted file can be dealth with by using "unsigned". If there will be cases where it genuinely overflows, there is a disabled code to deal with that too. function old new delta get_next_block 1678 1667 -11 Signed-off-by: Denys Vlasenko --- archival/libarchive/decompress_bunzip2.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/archival/libarchive/decompress_bunzip2.c b/archival/libarchive/decompress_bunzip2.c index 4fb989c29..2da5d59ac 100644 --- a/archival/libarchive/decompress_bunzip2.c +++ b/archival/libarchive/decompress_bunzip2.c @@ -157,15 +157,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 */ @@ -188,7 +188,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 @@ -436,7 +436,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; } @@ -446,14 +453,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; } @@ -467,7 +475,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]; -- 2.15.0