Fix crash when rendering invalid GIF

The core fix in this CL is a change to how LWZ decompression works, so
that when the min code table size and the color palette size are
different, color codes after the end of the defined color palette are
considered errors.

This CL also introduces a bunch of tweaks to the call return path,
since there were multiple locations where the GIF decode failing
status was being dropped on the floor, so the end widget would have a
bitmap with the default colour in it, instead of nothing.

BUG=chromium:616671

Change-Id: Id6f40d552dc24650c91e9903f710ff2fa63bc774
Reviewed-on: https://pdfium-review.googlesource.com/14630
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

8 files changed, 55 insertions, 37 deletions

diff --git a/core/fxcodec/codec/ccodec_gifmodule.cpp b/core/fxcodec/codec/ccodec_gifmodule.cpp
index f7e3546a8f..911323c3a1 100644
--- a/core/fxcodec/codec/ccodec_gifmodule.cpp
+++ b/core/fxcodec/codec/ccodec_gifmodule.cpp
@@ -36,7 +36,7 @@ GifDecodeStatus CCodec_GifModule::ReadHeader(Context* pContext,
 
   *width = context->width;
   *height = context->height;
-  *pal_num = context->global_pal_num;
+  *pal_num = (2 << context->global_pal_exp);
   *pal_pp = context->m_GlobalPalette.empty() ? nullptr
                                              : context->m_GlobalPalette.data();
   *bg_index = context->bc_index;
diff --git a/core/fxcodec/codec/ccodec_progressivedecoder.h b/core/fxcodec/codec/ccodec_progressivedecoder.h
index 87bed08fcf..05b7c92078 100644
--- a/core/fxcodec/codec/ccodec_progressivedecoder.h
+++ b/core/fxcodec/codec/ccodec_progressivedecoder.h
@@ -56,7 +56,7 @@ class CCodec_ProgressiveDecoder : public CCodec_BmpModule::Delegate,
   int32_t GetBPC() const { return m_SrcBPC; }
   void SetClipBox(FX_RECT* clip);
 
-  FXCODEC_STATUS GetFrames(int32_t& frames);
+  FXCODEC_STATUS GetFrames(int32_t* frames);
   FXCODEC_STATUS StartDecode(const RetainPtr<CFX_DIBitmap>& pDIBitmap,
                              int start_x,
                              int start_y,
diff --git a/core/fxcodec/codec/fx_codec_progress.cpp b/core/fxcodec/codec/fx_codec_progress.cpp
index 8b90d3ab84..f10523cc39 100644
--- a/core/fxcodec/codec/fx_codec_progress.cpp
+++ b/core/fxcodec/codec/fx_codec_progress.cpp
@@ -1791,7 +1791,7 @@ void CCodec_ProgressiveDecoder::Resample(
   }
 }
 
-FXCODEC_STATUS CCodec_ProgressiveDecoder::GetFrames(int32_t& frames) {
+FXCODEC_STATUS CCodec_ProgressiveDecoder::GetFrames(int32_t* frames) {
   if (!(m_status == FXCODEC_STATUS_FRAME_READY ||
         m_status == FXCODEC_STATUS_FRAME_TOBECONTINUE)) {
     return FXCODEC_STATUS_ERROR;
@@ -1801,7 +1801,8 @@ FXCODEC_STATUS CCodec_ProgressiveDecoder::GetFrames(int32_t& frames) {
     case FXCODEC_IMAGE_BMP:
     case FXCODEC_IMAGE_PNG:
     case FXCODEC_IMAGE_TIF:
-      frames = m_FrameNumber = 1;
+      *frames = 1;
+      m_FrameNumber = 1;
       m_status = FXCODEC_STATUS_DECODE_READY;
       return m_status;
     case FXCODEC_IMAGE_GIF: {
@@ -1822,7 +1823,7 @@ FXCODEC_STATUS CCodec_ProgressiveDecoder::GetFrames(int32_t& frames) {
               pGifModule->LoadFrameInfo(m_pGifContext.get(), &m_FrameNumber);
         }
         if (readResult == GifDecodeStatus::Success) {
-          frames = m_FrameNumber;
+          *frames = m_FrameNumber;
           m_status = FXCODEC_STATUS_DECODE_READY;
           return m_status;
         }
@@ -2108,34 +2109,37 @@ FXCODEC_STATUS CCodec_ProgressiveDecoder::ContinueDecode() {
     case FXCODEC_IMAGE_GIF: {
       CCodec_GifModule* pGifModule = m_pCodecMgr->GetGifModule();
       if (!pGifModule) {
+        m_pDeviceBitmap = nullptr;
+        m_pFile = nullptr;
         m_status = FXCODEC_STATUS_ERR_MEMORY;
         return m_status;
       }
-      while (true) {
-        GifDecodeStatus readRes =
-            pGifModule->LoadFrame(m_pGifContext.get(), m_FrameCur, nullptr);
-        while (readRes == GifDecodeStatus::Unfinished) {
-          FXCODEC_STATUS error_status = FXCODEC_STATUS_DECODE_FINISH;
-          if (!GifReadMoreData(pGifModule, error_status)) {
-            m_pDeviceBitmap = nullptr;
-            m_pFile = nullptr;
-            m_status = error_status;
-            return m_status;
-          }
-          readRes =
-              pGifModule->LoadFrame(m_pGifContext.get(), m_FrameCur, nullptr);
-        }
-        if (readRes == GifDecodeStatus::Success) {
+
+      GifDecodeStatus readRes =
+          pGifModule->LoadFrame(m_pGifContext.get(), m_FrameCur, nullptr);
+      while (readRes == GifDecodeStatus::Unfinished) {
+        FXCODEC_STATUS error_status = FXCODEC_STATUS_DECODE_FINISH;
+        if (!GifReadMoreData(pGifModule, error_status)) {
           m_pDeviceBitmap = nullptr;
           m_pFile = nullptr;
-          m_status = FXCODEC_STATUS_DECODE_FINISH;
+          m_status = error_status;
           return m_status;
         }
+        readRes =
+            pGifModule->LoadFrame(m_pGifContext.get(), m_FrameCur, nullptr);
+      }
+
+      if (readRes == GifDecodeStatus::Success) {
         m_pDeviceBitmap = nullptr;
         m_pFile = nullptr;
-        m_status = FXCODEC_STATUS_ERROR;
+        m_status = FXCODEC_STATUS_DECODE_FINISH;
         return m_status;
       }
+
+      m_pDeviceBitmap = nullptr;
+      m_pFile = nullptr;
+      m_status = FXCODEC_STATUS_ERROR;
+      return m_status;
     }
     case FXCODEC_IMAGE_BMP: {
       CCodec_BmpModule* pBmpModule = m_pCodecMgr->GetBmpModule();
diff --git a/core/fxcodec/fx_codec_def.h b/core/fxcodec/fx_codec_def.h
index fd23d78fa5..04696a11dd 100644
--- a/core/fxcodec/fx_codec_def.h
+++ b/core/fxcodec/fx_codec_def.h
@@ -22,6 +22,7 @@ enum FXCODEC_STATUS {
   FXCODEC_STATUS_ERR_FORMAT,
   FXCODEC_STATUS_ERR_PARAMS
 };
+
 #define JP2_SPACE_INVALID -1
 #define JPX_SPACE_INVALID -1
 #define JP2_SPACE_bilevel1 0
diff --git a/core/fxcodec/lgif/cgifcontext.cpp b/core/fxcodec/lgif/cgifcontext.cpp
index e57e978218..4dc663644c 100644
--- a/core/fxcodec/lgif/cgifcontext.cpp
+++ b/core/fxcodec/lgif/cgifcontext.cpp
@@ -17,7 +17,7 @@ CGifContext::CGifContext(CCodec_GifModule* gif_module,
                          CCodec_GifModule::Delegate* pDelegate)
     : m_pModule(gif_module),
       m_pDelegate(pDelegate),
-      global_pal_num(0),
+      global_pal_exp(0),
       img_row_offset(0),
       img_row_avail_size(0),
       avail_in(0),
diff --git a/core/fxcodec/lgif/cgifcontext.h b/core/fxcodec/lgif/cgifcontext.h
index d85243711d..71cf2bc75e 100644
--- a/core/fxcodec/lgif/cgifcontext.h
+++ b/core/fxcodec/lgif/cgifcontext.h
@@ -40,7 +40,7 @@ class CGifContext : public CCodec_GifModule::Context {
   UnownedPtr<CCodec_GifModule> m_pModule;
   UnownedPtr<CCodec_GifModule::Delegate> m_pDelegate;
   std::vector<GifPalette> m_GlobalPalette;
-  int32_t global_pal_num;
+  uint8_t global_pal_exp;
   uint32_t img_row_offset;
   uint32_t img_row_avail_size;
   uint32_t avail_in;
diff --git a/core/fxcodec/lgif/fx_gif.cpp b/core/fxcodec/lgif/fx_gif.cpp
index 18385c3bdd..ea78259420 100644
--- a/core/fxcodec/lgif/fx_gif.cpp
+++ b/core/fxcodec/lgif/fx_gif.cpp
@@ -147,6 +147,7 @@ GifDecodeStatus gif_decode_image_info(CGifContext* context) {
   }
   GifLF* gif_img_info_lf_ptr = (GifLF*)&gif_img_info_ptr->local_flag;
   if (gif_img_info_lf_ptr->local_pal) {
+    gif_image->local_pallette_exp = gif_img_info_lf_ptr->pal_bits;
     int32_t loc_pal_size = (2 << gif_img_info_lf_ptr->pal_bits) * 3;
     uint8_t* loc_pal_ptr = nullptr;
     if (!gif_read_data(context, &loc_pal_ptr, loc_pal_size)) {
@@ -162,7 +163,7 @@ GifDecodeStatus gif_decode_image_info(CGifContext* context) {
     context->skip_size = skip_size_org;
     return GifDecodeStatus::Unfinished;
   }
-  gif_image->image_code_size = *code_size_ptr;
+  gif_image->image_code_exp = *code_size_ptr;
   context->RecordCurrentPosition(&gif_image->image_data_pos);
   gif_image->image_data_pos += context->skip_size;
   gif_image->m_ImageGCE = nullptr;
@@ -214,10 +215,13 @@ CGifLZWDecoder::CGifLZWDecoder(char* error_ptr)
 
 CGifLZWDecoder::~CGifLZWDecoder() {}
 
-void CGifLZWDecoder::InitTable(uint8_t code_len) {
-  code_size = code_len;
-  ASSERT(code_size < 13);
-  code_clear = 1 << code_size;
+void CGifLZWDecoder::InitTable(uint8_t color_exp, uint8_t code_exp) {
+  // TODO(rharrison): Refactor all of this, so that initializing the table with
+  // bad values will kill the decompress.
+  ASSERT(code_exp <= GIF_MAX_LZW_EXP);
+  code_color_end = std::min(2 << color_exp, 1 << code_exp);
+  code_size = code_exp;
+  code_clear = 1 << code_exp;
   code_end = code_clear + 1;
   bits_left = 0;
   code_store = 0;
@@ -247,6 +251,9 @@ bool CGifLZWDecoder::DecodeString(uint16_t code) {
     stack[GIF_MAX_LZW_CODE - 1 - stack_size++] = code_table[code].suffix;
     code = code_table[code].prefix;
   }
+  if (code >= code_color_end)
+    return false;
+
   stack[GIF_MAX_LZW_CODE - 1 - stack_size++] = static_cast<uint8_t>(code);
   code_first = static_cast<uint8_t>(code);
   return true;
@@ -282,7 +289,7 @@ GifDecodeStatus CGifLZWDecoder::Decode(uint8_t* des_buf, uint32_t* des_size) {
   }
   ASSERT(err_msg_ptr);
   while (i <= *des_size && (avail_in > 0 || bits_left >= code_size_cur)) {
-    if (code_size_cur > 12) {
+    if (code_size_cur > GIF_MAX_LZW_EXP) {
       strncpy(err_msg_ptr, "Code Length Out Of Range", GIF_MAX_ERROR_SIZE - 1);
       return GifDecodeStatus::Error;
     }
@@ -383,9 +390,9 @@ GifDecodeStatus gif_read_header(CGifContext* context) {
     return GifDecodeStatus::Unfinished;
   }
   if (reinterpret_cast<GifGF*>(&gif_lsd_ptr->global_flag)->global_pal) {
-    context->global_pal_num =
-        2 << reinterpret_cast<GifGF*>(&gif_lsd_ptr->global_flag)->pal_bits;
-    int32_t global_pal_size = context->global_pal_num * 3;
+    context->global_pal_exp =
+        reinterpret_cast<GifGF*>(&gif_lsd_ptr->global_flag)->pal_bits;
+    int32_t global_pal_size = (2 << context->global_pal_exp) * 3;
     uint8_t* global_pal_ptr = nullptr;
     if (!gif_read_data(context, &global_pal_ptr, global_pal_size)) {
       context->skip_size = skip_size_org;
@@ -558,7 +565,7 @@ GifDecodeStatus gif_load_frame(CGifContext* context, int32_t frame_num) {
         return GifDecodeStatus::Error;
       }
     }
-    if (gif_image_ptr->image_code_size >= 13) {
+    if (gif_image_ptr->image_code_exp > GIF_MAX_LZW_EXP) {
       gif_image_ptr->m_ImageRowBuf.clear();
       context->AddError("Error Invalid Code Size");
       return GifDecodeStatus::Error;
@@ -566,7 +573,10 @@ GifDecodeStatus gif_load_frame(CGifContext* context, int32_t frame_num) {
     if (!context->m_ImgDecoder.get())
       context->m_ImgDecoder =
           pdfium::MakeUnique<CGifLZWDecoder>(context->m_szLastError);
-    context->m_ImgDecoder->InitTable(gif_image_ptr->image_code_size);
+    context->m_ImgDecoder->InitTable(!gif_image_ptr->m_LocalPalettes.empty()
+                                         ? gif_image_ptr->local_pallette_exp
+                                         : context->global_pal_exp,
+                                     gif_image_ptr->image_code_exp);
     context->img_row_offset = 0;
     context->img_row_avail_size = 0;
     context->img_pass_num = 0;
diff --git a/core/fxcodec/lgif/fx_gif.h b/core/fxcodec/lgif/fx_gif.h
index fd95aba17a..bddb22b30c 100644
--- a/core/fxcodec/lgif/fx_gif.h
+++ b/core/fxcodec/lgif/fx_gif.h
@@ -21,6 +21,7 @@ class CGifContext;
 #define GIF_BLOCK_CE 0xFE
 #define GIF_BLOCK_AE 0xFF
 #define GIF_BLOCK_TERMINAL 0x00
+#define GIF_MAX_LZW_EXP 12
 #define GIF_MAX_LZW_CODE 4096
 #define GIF_DATA_BLOCK 255
 #define GIF_MAX_ERROR_SIZE 256
@@ -116,7 +117,8 @@ class GifImage {
   std::vector<GifPalette> m_LocalPalettes;
   std::vector<uint8_t> m_ImageRowBuf;
   GifImageInfo m_ImageInfo;
-  uint8_t image_code_size;
+  uint8_t local_pallette_exp;
+  uint8_t image_code_exp;
   uint32_t image_data_pos;
   int32_t image_row_num;
 };
@@ -131,7 +133,7 @@ class CGifLZWDecoder {
   explicit CGifLZWDecoder(char* error_ptr);
   ~CGifLZWDecoder();
 
-  void InitTable(uint8_t code_len);
+  void InitTable(uint8_t color_exp, uint8_t code_exp);
   GifDecodeStatus Decode(uint8_t* des_buf, uint32_t* des_size);
   void Input(uint8_t* src_buf, uint32_t src_size);
   uint32_t GetAvailInput();
@@ -143,6 +145,7 @@ class CGifLZWDecoder {
 
   uint8_t code_size;
   uint8_t code_size_cur;
+  uint16_t code_color_end;
   uint16_t code_clear;
   uint16_t code_end;
   uint16_t code_next;