From 7e180fa92423795aaa986e7761883ad227e3a817 Mon Sep 17 00:00:00 2001 From: Wes Kocher Date: Tue, 21 Jun 2016 18:38:46 -0700 Subject: [PATCH] Backed out 2 changesets (bug 1249578) for build bustage Backed out changeset caddb604d934 (bug 1249578) Backed out changeset a2a645bf7ccf (bug 1249578) --- image/decoders/nsICODecoder.cpp | 88 ++++++++++-------- image/decoders/nsICODecoder.h | 18 ++-- image/test/gtest/Common.cpp | 16 ---- image/test/gtest/Common.h | 2 - image/test/gtest/TestDecoders.cpp | 20 ---- .../gtest/corrupt-with-bad-bmp-height.ico | Bin 41663 -> 0 bytes .../test/gtest/corrupt-with-bad-bmp-width.ico | Bin 41663 -> 0 bytes image/test/gtest/moz.build | 2 - 8 files changed, 55 insertions(+), 91 deletions(-) delete mode 100644 image/test/gtest/corrupt-with-bad-bmp-height.ico delete mode 100644 image/test/gtest/corrupt-with-bad-bmp-width.ico diff --git a/image/decoders/nsICODecoder.cpp b/image/decoders/nsICODecoder.cpp index 237a76f9c15d..46dbc6c46aa4 100644 --- a/image/decoders/nsICODecoder.cpp +++ b/image/decoders/nsICODecoder.cpp @@ -102,57 +102,58 @@ nsICODecoder::GetFinalStateFromContainedDecoder() MOZ_ASSERT(HasError() || !mCurrentFrame || mCurrentFrame->IsFinished()); } +// A BMP inside of an ICO has *2 height because of the AND mask +// that follows the actual bitmap. The BMP shouldn't know about +// this difference though. bool -nsICODecoder::CheckAndFixBitmapSize(int8_t* aBIH) +nsICODecoder::FixBitmapHeight(int8_t* bih) { - // Get the width from the BMP file information header. This is - // (unintuitively) a signed integer; see the documentation at: - // - // https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376(v=vs.85).aspx - // - // However, we reject negative widths since they aren't meaningful. - const int32_t width = LittleEndian::readInt32(aBIH + 4); - if (width <= 0 || width > 256) { - return false; - } - - // Verify that the BMP width matches the width we got from the ICO directory - // entry. If not, decoding fails, because if we were to allow it to continue - // the intrinsic size of the image wouldn't match the size of the decoded - // surface. - if (width != GetRealWidth()) { - return false; - } - - // Get the height from the BMP file information header. This is also signed, - // but in this case negative values are meaningful; see below. - int32_t height = LittleEndian::readInt32(aBIH + 8); - if (height == 0) { - return false; - } + // Get the height from the BMP file information header. + int32_t height = LittleEndian::readInt32(bih + 8); // BMPs can be stored inverted by having a negative height. - // XXX(seth): Should we really be writing the absolute value into the BIH - // below? Seems like this could be problematic for inverted BMPs. height = abs(height); - // The height field is double the actual height of the image to account for - // the AND mask. This is true even if the AND mask is not present. + // The bitmap height is by definition * 2 what it should be to account for + // the 'AND mask'. It is * 2 even if the `AND mask` is not present. height /= 2; + if (height > 256) { return false; } - // Verify that the BMP height matches the height we got from the ICO directory - // entry. If not, again, decoding fails. - if (height != GetRealHeight()) { + // We should always trust the height from the bitmap itself instead of + // the ICO height. So fix the ICO height. + if (height == 256) { + mDirEntry.mHeight = 0; + } else { + mDirEntry.mHeight = (int8_t)height; + } + + // Fix the BMP height in the BIH so that the BMP decoder can work properly + LittleEndian::writeInt32(bih + 8, height); + return true; +} + +// We should always trust the contained resource for the width +// information over our own information. +bool +nsICODecoder::FixBitmapWidth(int8_t* bih) +{ + // Get the width from the BMP file information header. + int32_t width = LittleEndian::readInt32(bih + 4); + + if (width > 256) { return false; } - // Fix the BMP height in the BIH so that the BMP decoder, which does not know - // about the AND mask that may follow the actual bitmap, can work properly. - LittleEndian::writeInt32(aBIH + 8, GetRealHeight()); - + // We should always trust the width from the bitmap itself instead of + // the ICO width. + if (width == 256) { + mDirEntry.mWidth = 0; + } else { + mDirEntry.mWidth = (int8_t)width; + } return true; } @@ -380,10 +381,15 @@ nsICODecoder::ReadBIH(const char* aData) } mContainedDecoder->Init(); - // Verify that the BIH width and height values match the ICO directory entry, - // and fix the BIH height value to compensate for the fact that the underlying - // BMP decoder doesn't know about AND masks. - if (!CheckAndFixBitmapSize(reinterpret_cast(mBIHraw))) { + // Fix the ICO height from the BIH. It needs to be halved so our BMP decoder + // will understand, because the BMP decoder doesn't expect the alpha mask that + // follows the BMP data in an ICO. + if (!FixBitmapHeight(reinterpret_cast(mBIHraw))) { + return Transition::TerminateFailure(); + } + + // Fix the ICO width from the BIH. + if (!FixBitmapWidth(reinterpret_cast(mBIHraw))) { return Transition::TerminateFailure(); } diff --git a/image/decoders/nsICODecoder.h b/image/decoders/nsICODecoder.h index 97217608aa23..8dfe527b6d21 100644 --- a/image/decoders/nsICODecoder.h +++ b/image/decoders/nsICODecoder.h @@ -85,16 +85,14 @@ private: // Gets decoder state from the contained decoder so it's visible externally. void GetFinalStateFromContainedDecoder(); - /** - * Verifies that the width and height values in @aBIH are valid and match the - * values we read from the ICO directory entry. If everything looks OK, the - * height value in @aBIH is updated to compensate for the AND mask, which the - * underlying BMP decoder doesn't know about. - * - * @return true if the width and height values in @aBIH are valid and correct. - */ - bool CheckAndFixBitmapSize(int8_t* aBIH); - + // Fixes the ICO height to match that of the BIH. + // and also fixes the BIH height to be /2 of what it was. + // See definition for explanation. + // Returns false if invalid information is contained within. + bool FixBitmapHeight(int8_t* bih); + // Fixes the ICO width to match that of the BIH. + // Returns false if invalid information is contained within. + bool FixBitmapWidth(int8_t* bih); // Obtains the number of colors from the BPP, mBPP must be filled in uint16_t GetNumColors(); diff --git a/image/test/gtest/Common.cpp b/image/test/gtest/Common.cpp index b098e26fccb4..2d6f59e31d48 100644 --- a/image/test/gtest/Common.cpp +++ b/image/test/gtest/Common.cpp @@ -485,22 +485,6 @@ ImageTestCase CorruptTestCase() TEST_CASE_HAS_ERROR); } -ImageTestCase CorruptICOWithBadBMPWidthTestCase() -{ - // This ICO contains a BMP icon which has a width that doesn't match the size - // listed in the corresponding ICO directory entry. - return ImageTestCase("corrupt-with-bad-bmp-width.ico", "image/x-icon", - IntSize(100, 100), TEST_CASE_HAS_ERROR); -} - -ImageTestCase CorruptICOWithBadBMPHeightTestCase() -{ - // This ICO contains a BMP icon which has a height that doesn't match the size - // listed in the corresponding ICO directory entry. - return ImageTestCase("corrupt-with-bad-bmp-height.ico", "image/x-icon", - IntSize(100, 100), TEST_CASE_HAS_ERROR); -} - ImageTestCase TransparentPNGTestCase() { return ImageTestCase("transparent.png", "image/png", IntSize(32, 32), diff --git a/image/test/gtest/Common.h b/image/test/gtest/Common.h index abeea385ec73..aa571d29cd4d 100644 --- a/image/test/gtest/Common.h +++ b/image/test/gtest/Common.h @@ -327,8 +327,6 @@ ImageTestCase GreenFirstFrameAnimatedGIFTestCase(); ImageTestCase GreenFirstFrameAnimatedPNGTestCase(); ImageTestCase CorruptTestCase(); -ImageTestCase CorruptICOWithBadBMPWidthTestCase(); -ImageTestCase CorruptICOWithBadBMPHeightTestCase(); ImageTestCase TransparentPNGTestCase(); ImageTestCase TransparentGIFTestCase(); diff --git a/image/test/gtest/TestDecoders.cpp b/image/test/gtest/TestDecoders.cpp index a6dbaca95788..49628c593267 100644 --- a/image/test/gtest/TestDecoders.cpp +++ b/image/test/gtest/TestDecoders.cpp @@ -350,26 +350,6 @@ TEST(ImageDecoders, CorruptMultiChunk) CheckDecoderMultiChunk(CorruptTestCase()); } -TEST(ImageDecoders, CorruptICOWithBadBMPWidthSingleChunk) -{ - CheckDecoderSingleChunk(CorruptICOWithBadBMPWidthTestCase()); -} - -TEST(ImageDecoders, CorruptICOWithBadBMPWidthMultiChunk) -{ - CheckDecoderMultiChunk(CorruptICOWithBadBMPWidthTestCase()); -} - -TEST(ImageDecoders, CorruptICOWithBadBMPHeightSingleChunk) -{ - CheckDecoderSingleChunk(CorruptICOWithBadBMPHeightTestCase()); -} - -TEST(ImageDecoders, CorruptICOWithBadBMPHeightMultiChunk) -{ - CheckDecoderMultiChunk(CorruptICOWithBadBMPHeightTestCase()); -} - TEST(ImageDecoders, AnimatedGIFWithExtraImageSubBlocks) { ImageTestCase testCase = ExtraImageSubBlocksAnimatedGIFTestCase(); diff --git a/image/test/gtest/corrupt-with-bad-bmp-height.ico b/image/test/gtest/corrupt-with-bad-bmp-height.ico deleted file mode 100644 index ee4a90fcd78ffea0c28ed23835983b6928206be2..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 41663 zcmeIup$&yl5Cp)*f+RsBe;E`*Stt~h&=&t5BzXMXWml}6q^n=oRL?o~WJ>a@)ReQ* z_IAsbjKh