From a3742637f501306f0ec3ffec73dbda8f22790863 Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Wed, 4 Oct 2017 14:44:21 -0400 Subject: Make GIF decoder more standards complaint Fixed issue with unit tests that was causing raw data to be backwards and reverted related LSB -> MSB change that was introduced due to this. If global palette not set then the background colour index should be 0. Check that background colour index is valid when global palette exists. Check if transparency index is valid for the palette of the frame it is being applied to. BUG=chromium:770337 Change-Id: I5d9b648f45bbb4c93218664a7035e4d01dbeb627 Reviewed-on: https://pdfium-review.googlesource.com/15453 Commit-Queue: Ryan Harrison Reviewed-by: dsinclair --- core/fxcodec/gif/cfx_gifcontext.cpp | 31 ++++++++--- core/fxcodec/gif/cfx_gifcontext_unittest.cpp | 83 +++++++++++----------------- 2 files changed, 54 insertions(+), 60 deletions(-) diff --git a/core/fxcodec/gif/cfx_gifcontext.cpp b/core/fxcodec/gif/cfx_gifcontext.cpp index 28bcad9ceb..ca2e7c3c88 100644 --- a/core/fxcodec/gif/cfx_gifcontext.cpp +++ b/core/fxcodec/gif/cfx_gifcontext.cpp @@ -390,9 +390,14 @@ CFX_GifDecodeStatus CFX_GifContext::ReadLogicalScreenDescriptor() { } if (lsd->global_flags.global_pal) { - uint32_t global_pal_size = unsigned(2 << lsd->global_flags.pal_bits) * 3u; - uint8_t* global_pal = nullptr; - if (!ReadData(&global_pal, global_pal_size)) { + uint32_t palette_count = unsigned(2 << lsd->global_flags.pal_bits); + if (lsd->bc_index >= palette_count) + return CFX_GifDecodeStatus::Error; + bc_index_ = lsd->bc_index; + + uint32_t palette_size = palette_count * 3u; + uint8_t* palette = nullptr; + if (!ReadData(&palette, palette_size)) { skip_size_ = skip_size_org; return CFX_GifDecodeStatus::Unfinished; } @@ -400,15 +405,15 @@ CFX_GifDecodeStatus CFX_GifContext::ReadLogicalScreenDescriptor() { global_pal_exp_ = lsd->global_flags.pal_bits; global_sort_flag_ = lsd->global_flags.sort_flag; global_color_resolution_ = lsd->global_flags.color_resolution; - global_palette_.resize(global_pal_size / 3); - memcpy(global_palette_.data(), global_pal, global_pal_size); + global_palette_.resize(palette_count); + memcpy(global_palette_.data(), palette, palette_size); } width_ = static_cast( - FXWORD_GET_MSBFIRST(reinterpret_cast(&lsd->width))); + FXWORD_GET_LSBFIRST(reinterpret_cast(&lsd->width))); height_ = static_cast( - FXWORD_GET_MSBFIRST(reinterpret_cast(&lsd->height))); - bc_index_ = lsd->bc_index; + FXWORD_GET_LSBFIRST(reinterpret_cast(&lsd->height))); + pixel_aspect_ = lsd->pixel_aspect; return CFX_GifDecodeStatus::Success; } @@ -544,9 +549,19 @@ CFX_GifDecodeStatus CFX_GifContext::DecodeImageInfo() { gif_image->data_pos += skip_size_; gif_image->image_GCE = nullptr; if (graphic_control_extension_.get()) { + if (graphic_control_extension_->gce_flags.transparency) { + // Need to test that the color that is going to be transparent is actually + // in the palette being used. + if (graphic_control_extension_->trans_index >= + 2 << (gif_image->local_palettes.empty() + ? global_pal_exp_ + : gif_image->local_pallette_exp)) + return CFX_GifDecodeStatus::Error; + } gif_image->image_GCE = std::move(graphic_control_extension_); graphic_control_extension_ = nullptr; } + images_.push_back(std::move(gif_image)); SaveDecodingStatus(GIF_D_STATUS_IMG_DATA); return CFX_GifDecodeStatus::Success; diff --git a/core/fxcodec/gif/cfx_gifcontext_unittest.cpp b/core/fxcodec/gif/cfx_gifcontext_unittest.cpp index 98b8ec5cd0..c5d3ac90c7 100644 --- a/core/fxcodec/gif/cfx_gifcontext_unittest.cpp +++ b/core/fxcodec/gif/cfx_gifcontext_unittest.cpp @@ -168,10 +168,9 @@ TEST(CFX_GifContext, ReadLocalScreenDescriptor) { } // LSD with all the values zero'd { - CFX_GifLocalScreenDescriptor lsd; + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)]; memset(&lsd, 0, sizeof(CFX_GifLocalScreenDescriptor)); - context.SetInputBuffer(reinterpret_cast(&lsd), - sizeof(CFX_GifLocalScreenDescriptor)); + context.SetInputBuffer(lsd, sizeof(CFX_GifLocalScreenDescriptor)); EXPECT_EQ(CFX_GifDecodeStatus::Success, context.ReadLogicalScreenDescriptor()); @@ -184,9 +183,9 @@ TEST(CFX_GifContext, ReadLocalScreenDescriptor) { } // LSD with no global palette { - CFX_GifLocalScreenDescriptor lsd{0x0A00, 0x000F, {0, 0, 0, 0}, 1, 2}; - context.SetInputBuffer(reinterpret_cast(&lsd), - sizeof(CFX_GifLocalScreenDescriptor)); + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)] = {0x0A, 0x00, 0x00, 0x0F, + 0x00, 0x01, 0x02}; + context.SetInputBuffer(lsd, sizeof(CFX_GifLocalScreenDescriptor)); EXPECT_EQ(CFX_GifDecodeStatus::Success, context.ReadLogicalScreenDescriptor()); @@ -194,14 +193,14 @@ TEST(CFX_GifContext, ReadLocalScreenDescriptor) { EXPECT_EQ(sizeof(CFX_GifLocalScreenDescriptor), context.skip_size_); EXPECT_EQ(0x000A, context.width_); EXPECT_EQ(0x0F00, context.height_); - EXPECT_EQ(1u, context.bc_index_); + EXPECT_EQ(0u, context.bc_index_); // bc_index_ is 0 if no global palette EXPECT_EQ(2u, context.pixel_aspect_); } // LSD with global palette bit set, but no global palette { - CFX_GifLocalScreenDescriptor lsd{0x0A00, 0x000F, {0, 0, 0, 1}, 1, 2}; - context.SetInputBuffer(reinterpret_cast(&lsd), - sizeof(CFX_GifLocalScreenDescriptor)); + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)] = {0x0A, 0x00, 0x00, 0x0F, + 0x80, 0x01, 0x02}; + context.SetInputBuffer(lsd, sizeof(CFX_GifLocalScreenDescriptor)); EXPECT_EQ(CFX_GifDecodeStatus::Unfinished, context.ReadLogicalScreenDescriptor()); @@ -210,14 +209,11 @@ TEST(CFX_GifContext, ReadLocalScreenDescriptor) { } // LSD with global palette { - CFX_GifLocalScreenDescriptor lsd = {0x0A00, 0x000F, {1, 1, 2, 1}, 1, 2}; - CFX_GifPalette palette[4] = {{0, 0, 0}, {1, 1, 1}, {1, 0, 0}, {0, 0, 1}}; struct { - CFX_GifLocalScreenDescriptor lsd; - CFX_GifPalette palette[4]; - } data; - memcpy(&data.lsd, &lsd, sizeof(data.lsd)); - memcpy(&data.palette, &palette, sizeof(data.lsd)); + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)]; + uint8_t palette[4 * sizeof(CFX_GifPalette)]; + } data = {{0x0A, 0x00, 0x00, 0x0F, 0xA9, 0x01, 0x02}, + {0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 1}}; context.SetInputBuffer(reinterpret_cast(&data), sizeof(data)); EXPECT_EQ(CFX_GifDecodeStatus::Success, @@ -228,6 +224,7 @@ TEST(CFX_GifContext, ReadLocalScreenDescriptor) { EXPECT_EQ(0x0F00, context.height_); EXPECT_EQ(1u, context.bc_index_); EXPECT_EQ(2u, context.pixel_aspect_); + EXPECT_EQ(1u, context.global_pal_exp_); EXPECT_EQ(1, context.global_sort_flag_); EXPECT_EQ(2, context.global_color_resolution_); @@ -240,19 +237,16 @@ TEST(CFX_GifContext, ReadHeader) { CFX_GifContextForTest context(nullptr, nullptr); // Bad signature { - uint8_t signature[] = {'N', 'O', 'T', 'G', 'I', 'F'}; - CFX_GifLocalScreenDescriptor lsd{0x0A00, 0x000F, {0, 0, 0, 0}, 1, 2}; struct { uint8_t signature[6]; - CFX_GifLocalScreenDescriptor lsd; - } data; - memcpy(&data.signature, signature, sizeof(data.signature)); - memcpy(&data.lsd, &lsd, sizeof(data.lsd)); + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)]; + } data = {{'N', 'O', 'T', 'G', 'I', 'F'}, + {0x0A, 0x00, 0x00, 0x0F, 0x00, 0x01, 0x02}}; context.SetInputBuffer(reinterpret_cast(&data), sizeof(data)); EXPECT_EQ(CFX_GifDecodeStatus::Error, context.ReadHeader()); - EXPECT_EQ(sizeof(signature), context.skip_size_); + EXPECT_EQ(sizeof(data.signature), context.skip_size_); } // Short after signature { @@ -266,15 +260,11 @@ TEST(CFX_GifContext, ReadHeader) { } // Success without global palette { - uint8_t signature[] = {'G', 'I', 'F', '8', '7', 'a'}; - CFX_GifLocalScreenDescriptor lsd{0x0A00, 0x000F, {0, 0, 0, 0}, 1, 2}; struct { uint8_t signature[6]; - CFX_GifLocalScreenDescriptor lsd; - } data; - memcpy(&data.signature, signature, sizeof(data.signature)); - memcpy(&data.lsd, &lsd, sizeof(data.lsd)); - + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)]; + } data = {{'G', 'I', 'F', '8', '7', 'a'}, + {0x0A, 0x00, 0x00, 0x0F, 0x00, 0x01, 0x02}}; context.SetInputBuffer(reinterpret_cast(&data), sizeof(data)); EXPECT_EQ(CFX_GifDecodeStatus::Success, context.ReadHeader()); @@ -282,42 +272,31 @@ TEST(CFX_GifContext, ReadHeader) { EXPECT_EQ(sizeof(data), context.skip_size_); EXPECT_EQ(0x000A, context.width_); EXPECT_EQ(0x0F00, context.height_); - EXPECT_EQ(1u, context.bc_index_); + EXPECT_EQ(0u, context.bc_index_); // bc_index_ is 0 if no global palette EXPECT_EQ(2u, context.pixel_aspect_); } // Missing Global Palette { - uint8_t signature[] = {'G', 'I', 'F', '8', '7', 'a'}; - CFX_GifLocalScreenDescriptor lsd{0x0A00, 0x000F, {0, 0, 0, 1}, 1, 2}; - struct { uint8_t signature[6]; - CFX_GifLocalScreenDescriptor lsd; - } data; - memcpy(&data.signature, signature, sizeof(data.signature)); - memcpy(&data.lsd, &lsd, sizeof(data.lsd)); - + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)]; + } data = {{'G', 'I', 'F', '8', '7', 'a'}, + {0x0A, 0x00, 0x00, 0x0F, 0x80, 0x01, 0x02}}; context.SetInputBuffer(reinterpret_cast(&data), sizeof(data)); EXPECT_EQ(CFX_GifDecodeStatus::Unfinished, context.ReadHeader()); - EXPECT_EQ(sizeof(signature), context.skip_size_); + EXPECT_EQ(sizeof(data.signature), context.skip_size_); } // Success with global palette { - uint8_t signature[] = {'G', 'I', 'F', '8', '7', 'a'}; - CFX_GifLocalScreenDescriptor lsd = {0x0A00, 0x000F, {1, 1, 2, 1}, 1, 2}; - CFX_GifPalette palette[4] = {{0, 0, 0}, {1, 1, 1}, {1, 0, 0}, {0, 0, 1}}; - struct { uint8_t signature[6]; - CFX_GifLocalScreenDescriptor lsd; - CFX_GifPalette palette[4]; - } data; - memcpy(&data.signature, signature, sizeof(data.signature)); - memcpy(&data.lsd, &lsd, sizeof(data.lsd)); - memcpy(&data.palette, &palette, sizeof(data.lsd)); - + uint8_t lsd[sizeof(CFX_GifLocalScreenDescriptor)]; + uint8_t palette[4 * sizeof(CFX_GifPalette)]; + } data = {{'G', 'I', 'F', '8', '7', 'a'}, + {0x0A, 0x00, 0x00, 0x0F, 0xA9, 0x01, 0x02}, + {0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 1}}; context.SetInputBuffer(reinterpret_cast(&data), sizeof(data)); EXPECT_EQ(CFX_GifDecodeStatus::Success, context.ReadHeader()); -- cgit v1.2.3