Fix underflow in ProcessFdatChunkAsIdat

Fail on an fdAT that is too small to have a sequence number. This
prevents an underflow in ProcessFdatChunkAsIdat.

In addition, rename fd_at to fdat. This variable got renamed in the
automatic conversion to Chromium coding style. "fdat" is more clear and
consistent with other variables (e.g. idat).

Change-Id: I4045fc13e7ef9e916d78db124720fe80bc817567
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1593852Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656354}

third_party/blink/renderer/platform/image-decoders/png/png_image_decoder_test.cc

@@ -575,6 +575,40 @@ TEST(AnimatedPNGTests, IdatSizeMismatch) {
   ExpectStatic(decoder.get());
 }
 
+TEST(AnimatedPNGTests, EmptyFdatFails) {
+  const char* png_file =
+      "/images/resources/"
+      "png-animated-idat-part-of-animation.png";
+  scoped_refptr<SharedBuffer> data = ReadFile(png_file);
+  ASSERT_FALSE(data->IsEmpty());
+
+  // Modify the third fdAT to be empty.
+  constexpr size_t kOffsetThirdFdat = 352;
+  scoped_refptr<SharedBuffer> modified_data =
+      SharedBuffer::Create(data->Data(), kOffsetThirdFdat);
+  png_byte four_bytes[4u];
+  WriteUint32(0, four_bytes);
+  modified_data->Append(reinterpret_cast<char*>(four_bytes), 4u);
+
+  // fdAT tag
+  modified_data->Append(data->Data() + kOffsetThirdFdat + 4u, 4u);
+
+  // crc computed from modified fdAT chunk
+  WriteUint32(4122214294, four_bytes);
+  modified_data->Append(reinterpret_cast<char*>(four_bytes), 4u);
+
+  // IEND
+  constexpr size_t kIENDOffset = 422u;
+  modified_data->Append(data->Data() + kIENDOffset, 12u);
+
+  auto decoder = CreatePNGDecoder();
+  decoder->SetData(std::move(modified_data), true);
+  for (size_t i = 0; i < decoder->FrameCount(); i++) {
+    decoder->DecodeFrameBufferAtIndex(i);
+  }
+  ASSERT_TRUE(decoder->Failed());
+}
+
 // Originally, the third frame has an offset of (1,2) and a size of (3,2). By
 // changing the offset to (4,4), the frame rect is no longer within the image
 // size of 5x5. This results in a failure.
@@ -714,11 +748,11 @@ TEST(AnimatedPNGTests, MixedDataChunks) {
       SharedBuffer::Create(full_data->Data(), kPostIDAT);
   const size_t kFcTLSize = 38u;
   const size_t kFdATSize = 31u;
-  png_byte fd_at[kFdATSize];
-  memcpy(fd_at, full_data->Data() + kPostIDAT + kFcTLSize, kFdATSize);
+  png_byte fdat[kFdATSize];
+  memcpy(fdat, full_data->Data() + kPostIDAT + kFcTLSize, kFdATSize);
   // Modify the sequence number
-  WriteUint32(1u, fd_at + 8);
-  data->Append((const char*)fd_at, kFdATSize);
+  WriteUint32(1u, fdat + 8);
+  data->Append((const char*)fdat, kFdATSize);
   const size_t kIENDOffset = 422u;
   data->Append(full_data->Data() + kIENDOffset,
                full_data->size() - kIENDOffset);

third_party/blink/renderer/platform/image-decoders/png/png_image_reader.cc

@@ -288,7 +288,7 @@ bool PNGImageReader::ProgressivelyDecodeFirstFrame(
 }
 
 void PNGImageReader::ProcessFdatChunkAsIdat(png_uint_32 fdat_length) {
-  // An fdAT chunk is build up as follows:
+  // An fdAT chunk is built as follows:
   // - |length| (4B)
   // - fdAT tag (4B)
   // - sequence number (4B)
@@ -300,7 +300,8 @@ void PNGImageReader::ProcessFdatChunkAsIdat(png_uint_32 fdat_length) {
   // - change the tag to IDAT.
   // - omit the sequence number from the data part of the chunk.
   png_byte chunk_idat[] = {0, 0, 0, 0, 'I', 'D', 'A', 'T'};
-  png_save_uint_32(chunk_idat, fdat_length - 4);
+  DCHECK_GE(fdat_length, 4u);
+  png_save_uint_32(chunk_idat, fdat_length - 4u);
   // The CRC is incorrect when applied to the modified fdAT.
   png_set_crc_action(png_, PNG_CRC_QUIET_USE, PNG_CRC_QUIET_USE);
   png_process_data(png_, info_, chunk_idat, 8);
@@ -409,11 +410,11 @@ bool PNGImageReader::Parse(SegmentReader& data, ParseQuery query) {
     if (idat && !expect_idats_)
       return false;
 
-    const bool fd_at = IsChunk(chunk, "fdAT");
-    if (fd_at && expect_idats_)
+    const bool fdat = IsChunk(chunk, "fdAT");
+    if (fdat && expect_idats_)
       return false;
 
-    if (fd_at || (idat && idat_is_part_of_animation_)) {
+    if (fdat || (idat && idat_is_part_of_animation_)) {
       fctl_needs_dat_chunk_ = false;
       if (!new_frame_.start_offset) {
         // Beginning of a new frame's data.
@@ -427,7 +428,13 @@ bool PNGImageReader::Parse(SegmentReader& data, ParseQuery query) {
         }
       }
 
-      if (fd_at) {
+      if (fdat) {
+        if (length < 4) {
+          // The sequence number requires 4 bytes. Further,
+          // ProcessFdatChunkAsIdat expects to be able to create an IDAT with
+          // |newLength| = length - 4. Prevent underflow in that calculation.
+          return false;
+        }
         if (reader.size() < read_offset_ + 8 + 4)
           return true;
         const png_byte* sequence_position =