Skip to content

Commit

Permalink
Fix decoding bug in bit reader
Browse files Browse the repository at this point in the history 
This bug could cause a wrong numeric value reading a PNG in the rare
case of long huffman symbol for a distance with many extra bits

Also add tests for the bit reader
  • Loading branch information
@lvandeve
lvandeve committed Dec 19, 2019
1 parent 2e541f5 commit 9652b36
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 10 deletions.
36 changes: 28 additions & 8 deletions lodepng.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
LodePNG version 20191109
LodePNG version 20191219
Copyright (c) 2005-2019 Lode Vandevenne
Expand Down Expand Up @@ -44,7 +44,7 @@ Rename this file to lodepng.cpp to use it for C++, or to lodepng.c to use it for
#pragma warning( disable : 4996 ) /*VS does not like fopen, but fopen_s is not standard C so unusable here*/
#endif /*_MSC_VER */

const char* LODEPNG_VERSION_STRING = "20191109";
const char* LODEPNG_VERSION_STRING = "20191219";

/*
This source file is built up in the following large parts. The code sections
Expand Down Expand Up @@ -535,7 +535,7 @@ static unsigned ensureBits9(LodePNGBitReader* reader, size_t nbits) {
reader->buffer = 0;
if(start + 0u < size) reader->buffer |= reader->data[start + 0];
reader->buffer >>= (reader->bp & 7u);
return reader->bp + nbits < reader->bitsize;
return reader->bp + nbits <= reader->bitsize;
}
}

Expand All @@ -553,7 +553,7 @@ static unsigned ensureBits17(LodePNGBitReader* reader, size_t nbits) {
if(start + 0u < size) reader->buffer |= reader->data[start + 0];
if(start + 1u < size) reader->buffer |= ((unsigned)reader->data[start + 1] << 8u);
reader->buffer >>= (reader->bp & 7u);
return reader->bp + nbits < reader->bitsize;
return reader->bp + nbits <= reader->bitsize;
}
}

Expand All @@ -572,7 +572,7 @@ static LODEPNG_INLINE unsigned ensureBits25(LodePNGBitReader* reader, size_t nbi
if(start + 1u < size) reader->buffer |= ((unsigned)reader->data[start + 1] << 8u);
if(start + 2u < size) reader->buffer |= ((unsigned)reader->data[start + 2] << 16u);
reader->buffer >>= (reader->bp & 7u);
return reader->bp + nbits < reader->bitsize;
return reader->bp + nbits <= reader->bitsize;
}
}

Expand All @@ -584,7 +584,7 @@ static LODEPNG_INLINE unsigned ensureBits32(LodePNGBitReader* reader, size_t nbi
reader->buffer = (unsigned)reader->data[start + 0] | ((unsigned)reader->data[start + 1] << 8u) |
((unsigned)reader->data[start + 2] << 16u) | ((unsigned)reader->data[start + 3] << 24u);
reader->buffer >>= (reader->bp & 7u);
reader->buffer |= (((unsigned)reader->data[start + 4] << 24u) << (7u - (reader->bp & 7u)));
reader->buffer |= (((unsigned)reader->data[start + 4] << 24u) << (8u - (reader->bp & 7u)));
return 1;
} else {
reader->buffer = 0;
Expand All @@ -593,12 +593,13 @@ static LODEPNG_INLINE unsigned ensureBits32(LodePNGBitReader* reader, size_t nbi
if(start + 2u < size) reader->buffer |= ((unsigned)reader->data[start + 2] << 16u);
if(start + 3u < size) reader->buffer |= ((unsigned)reader->data[start + 3] << 24u);
reader->buffer >>= (reader->bp & 7u);
return reader->bp + nbits < reader->bitsize;
return reader->bp + nbits <= reader->bitsize;
}
}

/* Get bits without advancing the bit pointer. Must have enough bits available with ensureBits */
/* Get bits without advancing the bit pointer. Must have enough bits available with ensureBits. Max nbits is 31. */
static unsigned peekBits(LodePNGBitReader* reader, size_t nbits) {
/* The shift allows nbits to be only up to 31. */
return reader->buffer & ((1u << nbits) - 1u);
}

Expand All @@ -614,6 +615,25 @@ static unsigned readBits(LodePNGBitReader* reader, size_t nbits) {
advanceBits(reader, nbits);
return result;
}

/* Public for testing only. steps and result must have numsteps values. */
unsigned lode_png_test_bitreader(const unsigned char* data, size_t size,
size_t numsteps, const size_t* steps, unsigned* result) {
size_t i;
LodePNGBitReader reader;
LodePNGBitReader_init(&reader, data, size);
for(i = 0; i < numsteps; i++) {
size_t step = steps[i];
unsigned ok;
if(step > 25) ok = ensureBits32(&reader, step);
else if(step > 17) ok = ensureBits25(&reader, step);
else if(step > 9) ok = ensureBits17(&reader, step);
else ok = ensureBits9(&reader, step);
if(!ok) return 0;
result[i] = readBits(&reader, step);
}
return 1;
}
#endif /*LODEPNG_COMPILE_DECODER*/

static unsigned reverseBits(unsigned bits, unsigned num) {
Expand Down
2 changes: 1 addition & 1 deletion lodepng.h
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
LodePNG version 20191109
LodePNG version 20191219
Copyright (c) 2005-2019 Lode Vandevenne
Expand Down
113 changes: 112 additions & 1 deletion lodepng_unittest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3504,6 +3504,117 @@ void testErrorImages() {
testBase64Image("iVBORw0KGgoAAAANSUhEUgAAAQAAAAEAAgMAAAAhHED1AAAAU0lEQVR4Ae3MwQAAAAxFoXnM3/NDvGsBdB8JBAKBQCAQCAQCgUAgEAgEAoFAIBAIBAKBQCAQCAQCgUAgEAgEAoFAIBAIBAKBQCAQCAQCgUAgEEQDHGPAW1eyhK0AAAAASUVORK5CYII=", true, 256, 256, "");
}

// defined in lodepng.cpp
unsigned lode_png_test_bitreader(const unsigned char* data, size_t size, size_t numsteps, const size_t* steps, unsigned* result);

void testBitReaderCase(const std::string& bits, const std::vector<size_t>& steps, bool expect_error, bool silent = false) {
if(!silent) std::cout << "testBitReaderCase: " << bits << ", #steps: " << steps.size() << std::endl;
std::vector<unsigned char> data;
std::vector<bool> bits0;
size_t bitcount = 0;
for(size_t i = 0; i < bits.size(); i++) {
char c = bits[i];
if(c != '0' && c != '1') continue;
if((bitcount & 7) == 0) data.push_back(0);
int bit = (c == '1');
data.back() |= (bit << (bitcount & 7));
bits0.push_back(bit);
bitcount++;
}
std::vector<unsigned> result(steps.size());
unsigned ok = lode_png_test_bitreader(data.data(), data.size(), steps.size(), steps.data(), result.data());
if(expect_error) {
assertEquals(0, ok, "expected it would give an error");
return;
}
assertEquals(1, ok, "expected there would be no error");

std::vector<bool> bits1;
for(size_t i = 0; i < steps.size(); i++) {
size_t step = steps[i];
size_t value = result[i];
for(size_t j = 0; j < step; j++) {
bits1.push_back((value >> j) & 1);
}
}
bits0.resize(bits1.size()); // only test those bits that were actually read for the test

assertEquals(bits0, bits1);
}

// This is still using C++98 for lodepng for compatibility, even though I'd love to use C++11 vector initializer lists here.
// TODO: use modern C++ at least for the unit test
#define V(arr) std::vector<size_t>(arr, arr + sizeof(arr) / sizeof(*arr))

void testBitReader() {
std::string zeros = "0000000000000000000000000000000000000000000000000000000000000000";
testBitReaderCase("", std::vector<size_t>(0), false);
{ size_t arr[] = {1}; testBitReaderCase("0", V(arr), false); }
{ size_t arr[] = {1}; testBitReaderCase("1", V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("00", V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("01", V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("10", V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("11", V(arr), false); }
{ size_t arr[] = {3}; testBitReaderCase("111", V(arr), false); }
{ size_t arr[] = {4}; testBitReaderCase("1111", V(arr), false); }
{ size_t arr[] = {8}; testBitReaderCase("11111111", V(arr), false); }
{ size_t arr[] = {9}; testBitReaderCase("111111111", V(arr), false); }
{ size_t arr[] = {9}; testBitReaderCase("11111111", V(arr), true); }
{ size_t arr[] = {16}; testBitReaderCase("1111111111111111", V(arr), false); }
{ size_t arr[] = {17}; testBitReaderCase("11111111111111111", V(arr), false); }
{ size_t arr[] = {17}; testBitReaderCase("1111111111111111", V(arr), true); }
{ size_t arr[] = {24}; testBitReaderCase("111111111111111111111111", V(arr), false); }
{ size_t arr[] = {25}; testBitReaderCase("1111111111111111111111111", V(arr), false); }
{ size_t arr[] = {25}; testBitReaderCase("111111111111111111111111", V(arr), true); }
{ size_t arr[] = {31}; testBitReaderCase("1111111111111111111111111111111", V(arr), false); }
{ size_t arr[] = {1, 31}; testBitReaderCase("11111111111111111111111111111111", V(arr), false); }
{ size_t arr[] = {31}; testBitReaderCase("111111111111111111111111", V(arr), true); }
{ size_t arr[] = {16, 16}; testBitReaderCase("11111111111111111111111111111111", V(arr), false); }
{ size_t arr[] = {1}; testBitReaderCase("0" + zeros, V(arr), false); }
{ size_t arr[] = {1}; testBitReaderCase("1" + zeros, V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("00" + zeros, V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("01" + zeros, V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("10" + zeros, V(arr), false); }
{ size_t arr[] = {2}; testBitReaderCase("11" + zeros, V(arr), false); }
{ size_t arr[] = {3}; testBitReaderCase("111" + zeros, V(arr), false); }
{ size_t arr[] = {4}; testBitReaderCase("1111" + zeros, V(arr), false); }
{ size_t arr[] = {8}; testBitReaderCase("11111111" + zeros, V(arr), false); }
{ size_t arr[] = {9}; testBitReaderCase("111111111" + zeros, V(arr), false); }
{ size_t arr[] = {16}; testBitReaderCase("1111111111111111" + zeros, V(arr), false); }
{ size_t arr[] = {17}; testBitReaderCase("11111111111111111" + zeros, V(arr), false); }
{ size_t arr[] = {24}; testBitReaderCase("111111111111111111111111" + zeros, V(arr), false); }
{ size_t arr[] = {25}; testBitReaderCase("1111111111111111111111111" + zeros, V(arr), false); }
{ size_t arr[] = {31}; testBitReaderCase("1111111111111111111111111111111" + zeros, V(arr), false); }
{ size_t arr[] = {16, 16}; testBitReaderCase("11111111111111111111111111111111" + zeros, V(arr), false); }

// 128 arbitrary bits
std::string test = "10101011110000101110010010000101000100100000111000010010010010010010111100000100100100100000001010111111110001111010101011011001";
for(size_t i = 0; i < 32; i++) {
std::cout << "testBitReader loop " << i << std::endl;
{ size_t arr[] = {i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {i, i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {i, i, i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {i, i, i, i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {1, i, i, i, i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {2, i, i, i, i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {3, i, i, i, i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {31, 31, 31, i}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {i, i, i, i, i, i, i, i}; testBitReaderCase(test + test, V(arr), false, true); }
for(size_t j = 0; j < 32; j++) {
{ size_t arr[] = {i, j}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {i, j, i, j}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {i, i, j, j}; testBitReaderCase(test, V(arr), false, true); }
{ size_t arr[] = {31, 31, i, j}; testBitReaderCase(test, V(arr), false, true); }
}
}
{ size_t arr[] = {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1}; testBitReaderCase(test, V(arr), false); }
{ size_t arr[] = {3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3}; testBitReaderCase(test, V(arr), false); }
{ size_t arr[] = {1,1,1,1,1,1,1,1,1,1,1,1,1,31,1,1,1,1,1,1,1,1,1,1,1}; testBitReaderCase(test, V(arr), false); }
{ size_t arr[] = {16,1,2,4,7,3,6,28,28,31,1,17,12,1,3,8,3,3,14,21,25,24,1,8,7}; testBitReaderCase(test + test + test, V(arr), false); }
{ size_t arr[] = {5,7,17,15,6,8,4,5,3,11,1,4,4,8,6,4,5,1,6,5,13,8,18,1,1,8,7,2}; testBitReaderCase(test + test + test, V(arr), false); }

}

void doMain() {
//PNG
testPngSuite();
Expand Down Expand Up @@ -3546,8 +3657,8 @@ void doMain() {
testCustomDeflate();
testCustomZlibDecompress();
testCustomInflate();
testBitReader();
// TODO: add test for huffman code with exactly 0 and 1 symbols present
// TODO: add case where ensureBits25 and ensureBits32 do left shift of 24 with value >= 128

//lodepng_util
testChunkUtil();
Expand Down

0 comments on commit 9652b36

Please sign in to comment.