From 7a20c2cdfcf3e45f31063b4eaff94f1c0776b593 Mon Sep 17 00:00:00 2001 From: Florian Kainz Date: Wed, 29 Jul 2009 02:18:59 +0000 Subject: [PATCH] Bug fix: crashes when reading damaged images, caused by buffer overruns, or by deleting uninitialized pointers (found by Apple). --- ChangeLog | 11 +++ IlmImf/ImfB44Compressor.cpp | 15 +++-- IlmImf/ImfB44Compressor.h | 4 +- IlmImf/ImfCheckedArithmetic.h | 161 +++++++++++++++++++++++++++++++++++++++++ IlmImf/ImfCompressor.cpp | 9 ++- IlmImf/ImfCompressor.h | 7 +- IlmImf/ImfHuf.cpp | 20 +++++- IlmImf/ImfPizCompressor.cpp | 18 ++++- IlmImf/ImfPizCompressor.h | 5 +- IlmImf/ImfPreviewImage.cpp | 4 +- IlmImf/ImfPxr24Compressor.cpp | 15 +++- IlmImf/ImfPxr24Compressor.h | 5 +- IlmImf/ImfRleCompressor.cpp | 5 +- IlmImf/ImfRleCompressor.h | 2 +- IlmImf/ImfZipCompressor.cpp | 17 ++++- IlmImf/ImfZipCompressor.h | 5 +- IlmImf/Makefile.am | 6 +- 17 files changed, 271 insertions(+), 38 deletions(-) create mode 100644 IlmImf/ImfCheckedArithmetic.h diff --git a/ChangeLog b/ChangeLog index a2bb502..9b6c4da 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,14 @@ + * Bug fix: crash when reading a damaged image file (found + by Apple). An exception thrown inside the PIZ Huffman + decoder bypasses initialization of an array of pointers. + The uninitialized pointers are later passed to operator + delete. + (Florian Kainz) + * Bug fix: crash when reading a damaged image file (found by + Apple). Computing the size of input certain buffers may + overflow and wrap around to a small number, later causing + writes beyond the end of the buffer. + (Florian Kainz) * In the "Technical Introduction" document, added Premultiplied vs. Un-Premulitiplied Color section: states explicitly that pixels with zero alpha and non-zero diff --git a/IlmImf/ImfB44Compressor.cpp b/IlmImf/ImfB44Compressor.cpp index 7771e6e..231c18c 100644 --- a/IlmImf/ImfB44Compressor.cpp +++ b/IlmImf/ImfB44Compressor.cpp @@ -101,6 +101,7 @@ #include #include #include +#include #include #include #include @@ -465,8 +466,8 @@ struct B44Compressor::ChannelData B44Compressor::B44Compressor (const Header &hdr, - int maxScanLineSize, - int numScanLines, + size_t maxScanLineSize, + size_t numScanLines, bool optFlatFields) : Compressor (hdr), @@ -487,7 +488,9 @@ B44Compressor::B44Compressor // if uncompressed pixel data should be in native or Xdr format. // - _tmpBuffer = new unsigned short [maxScanLineSize * numScanLines]; + _tmpBuffer = new unsigned short + [checkArraySize (uiMult (maxScanLineSize, numScanLines), + sizeof (unsigned short))]; const ChannelList &channels = header().channels(); int numHalfChans = 0; @@ -507,9 +510,11 @@ B44Compressor::B44Compressor // Compressed data may be larger than the input data // - int padding = 12 * numHalfChans * (numScanLines + 3) / 4; + size_t padding = 12 * numHalfChans * (numScanLines + 3) / 4; + + _outBuffer = new char + [uiAdd (uiMult (maxScanLineSize, numScanLines), padding)]; - _outBuffer = new char [maxScanLineSize * numScanLines + padding]; _channelData = new ChannelData[_numChans]; int i = 0; diff --git a/IlmImf/ImfB44Compressor.h b/IlmImf/ImfB44Compressor.h index b426927..32b3713 100644 --- a/IlmImf/ImfB44Compressor.h +++ b/IlmImf/ImfB44Compressor.h @@ -54,8 +54,8 @@ class B44Compressor: public Compressor public: B44Compressor (const Header &hdr, - int maxScanLineSize, - int numScanLines, + size_t maxScanLineSize, + size_t numScanLines, bool optFlatFields); virtual ~B44Compressor (); diff --git a/IlmImf/ImfCheckedArithmetic.h b/IlmImf/ImfCheckedArithmetic.h new file mode 100644 index 0000000..00eb93d --- /dev/null +++ b/IlmImf/ImfCheckedArithmetic.h @@ -0,0 +1,161 @@ +/////////////////////////////////////////////////////////////////////////// +// +// Copyright (c) 2009, Industrial Light & Magic, a division of Lucas +// Digital Ltd. LLC +// +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Industrial Light & Magic nor the names of +// its contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// +/////////////////////////////////////////////////////////////////////////// + +#ifndef INCLUDED_IMF_CHECKED_ARITHMETIC_H +#define INCLUDED_IMF_CHECKED_ARITHMETIC_H + +//----------------------------------------------------------------------------- +// +// Integer arithmetic operations that throw exceptions +// on overflow, underflow or division by zero. +// +//----------------------------------------------------------------------------- + +#include +#include + +namespace Imf { + +template struct StaticAssertionFailed; +template <> struct StaticAssertionFailed {}; + +#define IMF_STATIC_ASSERT(x) \ + do {StaticAssertionFailed staticAssertionFailed;} while (false) + + +template +T +uiMult (T a, T b) +{ + // + // Unsigned integer multiplication + // + + IMF_STATIC_ASSERT (!std::numeric_limits::is_signed && + std::numeric_limits::is_integer); + + if (a > 0 && b > std::numeric_limits::max() / a) + throw Iex::OverflowExc ("Integer multiplication overflow."); + + return a * b; +} + + +template +T +uiDiv (T a, T b) +{ + // + // Unsigned integer division + // + + IMF_STATIC_ASSERT (!std::numeric_limits::is_signed && + std::numeric_limits::is_integer); + + if (b == 0) + throw Iex::DivzeroExc ("Integer division by zero."); + + return a / b; +} + + +template +T +uiAdd (T a, T b) +{ + // + // Unsigned integer addition + // + + IMF_STATIC_ASSERT (!std::numeric_limits::is_signed && + std::numeric_limits::is_integer); + + if (a > std::numeric_limits::max() - b) + throw Iex::OverflowExc ("Integer addition overflow."); + + return a + b; +} + + +template +T +uiSub (T a, T b) +{ + // + // Unsigned integer subtraction + // + + IMF_STATIC_ASSERT (!std::numeric_limits::is_signed && + std::numeric_limits::is_integer); + + if (a < b) + throw Iex::UnderflowExc ("Integer subtraction underflow."); + + return a - b; +} + + +template +size_t +checkArraySize (T n, size_t s) +{ + // + // Verify that the size, in bytes, of an array with n elements + // of size s can be computed without overflowing: + // + // If computing + // + // size_t (n) * s + // + // would overflow, then throw an Iex::OverflowExc exception. + // Otherwise return + // + // size_t (n). + // + + IMF_STATIC_ASSERT (!std::numeric_limits::is_signed && + std::numeric_limits::is_integer); + + IMF_STATIC_ASSERT (sizeof (T) <= sizeof (size_t)); + + if (size_t (n) > std::numeric_limits::max() / s) + throw Iex::OverflowExc ("Integer multiplication overflow."); + + return size_t (n); +} + + +} // namespace Imf + +#endif diff --git a/IlmImf/ImfCompressor.cpp b/IlmImf/ImfCompressor.cpp index 17dbab9..96bde8f 100644 --- a/IlmImf/ImfCompressor.cpp +++ b/IlmImf/ImfCompressor.cpp @@ -46,6 +46,7 @@ #include #include #include +#include namespace Imf { @@ -109,7 +110,7 @@ isValidCompression (Compression c) Compressor * -newCompressor (Compression c, int maxScanLineSize, const Header &hdr) +newCompressor (Compression c, size_t maxScanLineSize, const Header &hdr) { switch (c) { @@ -150,15 +151,15 @@ newCompressor (Compression c, int maxScanLineSize, const Header &hdr) Compressor * newTileCompressor (Compression c, - int tileLineSize, - int numTileLines, + size_t tileLineSize, + size_t numTileLines, const Header &hdr) { switch (c) { case RLE_COMPRESSION: - return new RleCompressor (hdr, tileLineSize * numTileLines); + return new RleCompressor (hdr, uiMult (tileLineSize, numTileLines)); case ZIPS_COMPRESSION: case ZIP_COMPRESSION: diff --git a/IlmImf/ImfCompressor.h b/IlmImf/ImfCompressor.h index 7439fd3..6039396 100644 --- a/IlmImf/ImfCompressor.h +++ b/IlmImf/ImfCompressor.h @@ -45,6 +45,7 @@ #include #include "ImathBox.h" +#include namespace Imf { @@ -219,7 +220,7 @@ bool isValidCompression (Compression c); //----------------------------------------------------------------- Compressor * newCompressor (Compression c, - int maxScanLineSize, + size_t maxScanLineSize, const Header &hdr); @@ -241,8 +242,8 @@ Compressor * newCompressor (Compression c, //----------------------------------------------------------------- Compressor * newTileCompressor (Compression c, - int tileLineSize, - int numTileLines, + size_t tileLineSize, + size_t numTileLines, const Header &hdr); diff --git a/IlmImf/ImfHuf.cpp b/IlmImf/ImfHuf.cpp index 4aa60e3..0de7d34 100644 --- a/IlmImf/ImfHuf.cpp +++ b/IlmImf/ImfHuf.cpp @@ -579,6 +579,19 @@ hufUnpackEncTable // // +// Clear a newly allocated decoding table so that it contains only zeroes. +// + +void +hufClearDecTable + (HufDec * hdecod) // io: (allocated by caller) + // decoding table [HUF_DECSIZE] +{ + memset (hdecod, 0, sizeof (HufDec) * HUF_DECSIZE); +} + + +// // Build a decoding hash table based on the encoding table hcode: // - short codes (<= HUF_DECBITS) are resolved with a single table access; // - long code entry allocations are not optimized, because long codes are @@ -595,11 +608,10 @@ hufBuildDecTable // decoding table [HUF_DECSIZE] { // - // Init hashtable & loop on all codes + // Init hashtable & loop on all codes. + // Assumes that hufClearDecTable(hdecod) has already been called. // - memset (hdecod, 0, sizeof (HufDec) * HUF_DECSIZE); - for (; im <= iM; im++) { Int64 c = hufCode (hcode[im]); @@ -1049,6 +1061,8 @@ hufUncompress (const char compressed[], AutoArray freq; AutoArray hdec; + hufClearDecTable (hdec); + hufUnpackEncTable (&ptr, nCompressed - (ptr - compressed), im, iM, freq); try diff --git a/IlmImf/ImfPizCompressor.cpp b/IlmImf/ImfPizCompressor.cpp index 7c84919..9ceb8c6 100644 --- a/IlmImf/ImfPizCompressor.cpp +++ b/IlmImf/ImfPizCompressor.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include #include @@ -168,8 +169,8 @@ struct PizCompressor::ChannelData PizCompressor::PizCompressor (const Header &hdr, - int maxScanLineSize, - int numScanLines) + size_t maxScanLineSize, + size_t numScanLines) : Compressor (hdr), _maxScanLineSize (maxScanLineSize), @@ -181,8 +182,17 @@ PizCompressor::PizCompressor _channels (hdr.channels()), _channelData (0) { - _tmpBuffer = new unsigned short [maxScanLineSize * numScanLines / 2]; - _outBuffer = new char [maxScanLineSize * numScanLines + 65536 + 8192]; + size_t tmpBufferSize = + uiMult (maxScanLineSize, numScanLines) / 2; + + size_t outBufferSize = + uiAdd (uiMult (maxScanLineSize, numScanLines), + size_t (65536 + 8192)); + + _tmpBuffer = new unsigned short + [checkArraySize (tmpBufferSize, sizeof (unsigned short))]; + + _outBuffer = new char [outBufferSize]; const ChannelList &channels = header().channels(); bool onlyHalfChannels = true; diff --git a/IlmImf/ImfPizCompressor.h b/IlmImf/ImfPizCompressor.h index f9ac7f4..0945a8c 100644 --- a/IlmImf/ImfPizCompressor.h +++ b/IlmImf/ImfPizCompressor.h @@ -53,7 +53,10 @@ class PizCompressor: public Compressor { public: - PizCompressor (const Header &hdr, int maxScanLineSize, int numScanLines); + PizCompressor (const Header &hdr, + size_t maxScanLineSize, + size_t numScanLines); + virtual ~PizCompressor (); virtual int numScanLines () const; diff --git a/IlmImf/ImfPreviewImage.cpp b/IlmImf/ImfPreviewImage.cpp index dae5227..d47d931 100644 --- a/IlmImf/ImfPreviewImage.cpp +++ b/IlmImf/ImfPreviewImage.cpp @@ -40,6 +40,7 @@ //----------------------------------------------------------------------------- #include +#include #include "Iex.h" namespace Imf { @@ -51,7 +52,8 @@ PreviewImage::PreviewImage (unsigned int width, { _width = width; _height = height; - _pixels = new PreviewRgba [_width * _height]; + _pixels = new PreviewRgba + [checkArraySize (uiMult (_width, _height), sizeof (PreviewRgba))]; if (pixels) { diff --git a/IlmImf/ImfPxr24Compressor.cpp b/IlmImf/ImfPxr24Compressor.cpp index a8edaad..d483abb 100644 --- a/IlmImf/ImfPxr24Compressor.cpp +++ b/IlmImf/ImfPxr24Compressor.cpp @@ -67,6 +67,7 @@ #include #include #include +#include #include #include #include @@ -175,8 +176,8 @@ tooMuchData () Pxr24Compressor::Pxr24Compressor (const Header &hdr, - int maxScanLineSize, - int numScanLines) + size_t maxScanLineSize, + size_t numScanLines) : Compressor (hdr), _maxScanLineSize (maxScanLineSize), @@ -185,10 +186,16 @@ Pxr24Compressor::Pxr24Compressor (const Header &hdr, _outBuffer (0), _channels (hdr.channels()) { - int maxInBytes = maxScanLineSize * numScanLines; + size_t maxInBytes = + uiMult (maxScanLineSize, numScanLines); + + size_t maxOutBytes = + uiAdd (uiAdd (maxInBytes, + size_t (ceil (maxInBytes * 0.01))), + size_t (100)); _tmpBuffer = new unsigned char [maxInBytes]; - _outBuffer = new char [int (ceil (maxInBytes * 1.01)) + 100]; + _outBuffer = new char [maxOutBytes]; const Box2i &dataWindow = hdr.dataWindow(); diff --git a/IlmImf/ImfPxr24Compressor.h b/IlmImf/ImfPxr24Compressor.h index ff168ae..71e6109 100644 --- a/IlmImf/ImfPxr24Compressor.h +++ b/IlmImf/ImfPxr24Compressor.h @@ -51,7 +51,10 @@ class Pxr24Compressor: public Compressor { public: - Pxr24Compressor (const Header &hdr, int maxScanLineSize, int numScanLines); + Pxr24Compressor (const Header &hdr, + size_t maxScanLineSize, + size_t numScanLines); + virtual ~Pxr24Compressor (); virtual int numScanLines () const; diff --git a/IlmImf/ImfRleCompressor.cpp b/IlmImf/ImfRleCompressor.cpp index cd79609..90619b4 100644 --- a/IlmImf/ImfRleCompressor.cpp +++ b/IlmImf/ImfRleCompressor.cpp @@ -41,6 +41,7 @@ //----------------------------------------------------------------------------- #include +#include #include "Iex.h" namespace Imf { @@ -158,14 +159,14 @@ rleUncompress (int inLength, int maxLength, const signed char in[], char out[]) } // namespace -RleCompressor::RleCompressor (const Header &hdr, int maxScanLineSize): +RleCompressor::RleCompressor (const Header &hdr, size_t maxScanLineSize): Compressor (hdr), _maxScanLineSize (maxScanLineSize), _tmpBuffer (0), _outBuffer (0) { _tmpBuffer = new char [maxScanLineSize]; - _outBuffer = new char [maxScanLineSize * 3 / 2]; + _outBuffer = new char [uiMult (maxScanLineSize, size_t (3)) / 2]; } diff --git a/IlmImf/ImfRleCompressor.h b/IlmImf/ImfRleCompressor.h index 455c640..e272381 100644 --- a/IlmImf/ImfRleCompressor.h +++ b/IlmImf/ImfRleCompressor.h @@ -52,7 +52,7 @@ class RleCompressor: public Compressor { public: - RleCompressor (const Header &hdr, int maxScanLineSize); + RleCompressor (const Header &hdr, size_t maxScanLineSize); virtual ~RleCompressor (); virtual int numScanLines () const; diff --git a/IlmImf/ImfZipCompressor.cpp b/IlmImf/ImfZipCompressor.cpp index 0d98096..125e2fb 100644 --- a/IlmImf/ImfZipCompressor.cpp +++ b/IlmImf/ImfZipCompressor.cpp @@ -41,6 +41,7 @@ //----------------------------------------------------------------------------- #include +#include #include "Iex.h" #include @@ -49,8 +50,8 @@ namespace Imf { ZipCompressor::ZipCompressor (const Header &hdr, - int maxScanLineSize, - int numScanLines) + size_t maxScanLineSize, + size_t numScanLines) : Compressor (hdr), _maxScanLineSize (maxScanLineSize), @@ -58,11 +59,19 @@ ZipCompressor::ZipCompressor _tmpBuffer (0), _outBuffer (0) { + size_t maxInBytes = + uiMult (maxScanLineSize, numScanLines); + + size_t maxOutBytes = + uiAdd (uiAdd (maxInBytes, + size_t (ceil (maxInBytes * 0.01))), + size_t (100)); + _tmpBuffer = - new char [maxScanLineSize * numScanLines]; + new char [maxInBytes]; _outBuffer = - new char [int (ceil (maxScanLineSize * numScanLines * 1.01)) + 100]; + new char [maxOutBytes]; } diff --git a/IlmImf/ImfZipCompressor.h b/IlmImf/ImfZipCompressor.h index f4de96b..a515348 100644 --- a/IlmImf/ImfZipCompressor.h +++ b/IlmImf/ImfZipCompressor.h @@ -52,7 +52,10 @@ class ZipCompressor: public Compressor { public: - ZipCompressor (const Header &hdr, int maxScanLineSize, int numScanLines); + ZipCompressor (const Header &hdr, + size_t maxScanLineSize, + size_t numScanLines); + virtual ~ZipCompressor (); virtual int numScanLines () const; diff --git a/IlmImf/Makefile.am b/IlmImf/Makefile.am index 2bffa65..d5b2b1c 100644 --- a/IlmImf/Makefile.am +++ b/IlmImf/Makefile.am @@ -59,7 +59,8 @@ libIlmImf_la_SOURCES = ImfAttribute.cpp ImfBoxAttribute.cpp ImfCRgbaFile.cpp \ ImfTestFile.cpp ImfTestFile.h ImfThreading.h \ ImfStringVectorAttribute.cpp ImfStringVectorAttribute.h \ ImfMultiView.cpp ImfMultiView.h \ - ImfAcesFile.cpp ImfAcesFile.h + ImfAcesFile.cpp ImfAcesFile.h \ + ImfCheckedArithmetic.h libIlmImf_la_LDFLAGS = @ILMBASE_LDFLAGS@ -version-info @LIBTOOL_VERSION@ \ -no-undefined @@ -104,7 +105,8 @@ libIlmImfinclude_HEADERS = ImfAttribute.h ImfBoxAttribute.h \ noinst_HEADERS = ImfCompressor.h ImfRleCompressor.h ImfZipCompressor.h \ ImfPizCompressor.h ImfMisc.h ImfAutoArray.h ImfTiledMisc.h \ - ImfTileOffsets.h ImfScanLineInputFile.h ImfPxr24Compressor.h + ImfTileOffsets.h ImfScanLineInputFile.h ImfPxr24Compressor.h \ + ImfCheckedArithmetic.h EXTRA_DIST = $(noinst_HEADERS) b44ExpLogTable.cpp b44ExpLogTable.h -- 1.6.3.3