Fix thumbnail store crashing on loading old files

chrome/browser/android/thumbnail/thumbnail_store.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/thumbnail/thumbnail_store.h"
 
 #include <algorithm>
 #include <cmath>
 
+#include "base/big_endian.h"
 #include "base/file_util.h"
 #include "base/files/file.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/time.h"
 #include "content/public/browser/android/ui_resource_provider.h"
 #include "content/public/browser/browser_thread.h"
 #include "third_party/android_opengl/etc1/etc1.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkData.h"
 #include "third_party/skia/include/core/SkMallocPixelRef.h"
 #include "third_party/skia/include/core/SkPixelRef.h"
 #include "ui/gfx/geometry/size_conversions.h"
 
 namespace {
 
 const float kApproximationScaleFactor = 4.f;
 const base::TimeDelta kCaptureMinRequestTimeMs(
     base::TimeDelta::FromMilliseconds(1000));
 
 const int kCompressedKey = 0xABABABAB;
+const int kCurrentExtraVersion = 1;
+const int kMaxDimensionSize = 2048;
 
 // Indicates whether we prefer to have more free CPU memory over GPU memory.
 const bool kPreferCPUMemory = true;
 
 // TODO(): ETC1 texture sizes should be multiples of four, but some drivers only
 // allow power-of-two ETC1 textures.  Find better work around.
 size_t NextPowerOfTwo(size_t x) {
   --x;
   x |= x >> 1;
   x |= x >> 2;
   x |= x >> 4;
   x |= x >> 8;
   x |= x >> 16;
   return x + 1;
 }
 
 gfx::Size GetEncodedSize(const gfx::Size& bitmap_size) {
   DCHECK(!bitmap_size.IsEmpty());
   return gfx::Size(NextPowerOfTwo(bitmap_size.width()),
                    NextPowerOfTwo(bitmap_size.height()));
 }
 
+template<typename T>
+bool ReadBigEndianFromFile(base::File& file, T* out) {
+  char buffer[sizeof(T)];
+  if (file.ReadAtCurrentPos(buffer, sizeof(T)) != sizeof(T))
+    return false;
+
+  base::ReadBigEndian(buffer, out);
+  return true;
+}
+
+template<typename T>
+bool WriteBigEndianToFile(base::File& file, T val) {
+  char buffer[sizeof(T)];
+  base::WriteBigEndian(buffer, val);
+  if (file.WriteAtCurrentPos(buffer, sizeof(T)) != sizeof(T))

[Ted C, 2014/08/19 16:51:09]

Could you also just do:

return file.WriteAtCurrentPos(buffer, sizeof(T)) == sizeof(T);

[David Trainor- moved to gerrit, 2014/08/19 18:31:07]

Good point.  Done

+    return false;
+  return true;
+}
+
 }  // anonymous namespace
 
 ThumbnailStore::ThumbnailStore(const std::string& disk_cache_path_str,
                                size_t default_cache_size,
                                size_t approximation_cache_size,
                                size_t compression_queue_max_size,
                                size_t write_queue_max_size,
                                bool use_approximation_thumbnail)
     : disk_cache_path_(disk_cache_path_str),
       compression_queue_max_size_(compression_queue_max_size),
@@ skipping 344 matching lines @@
                                skia::RefPtr<SkPixelRef> compressed_data,
                                float scale,
                                const gfx::Size& content_size,
                                const base::Callback<void()>& post_write_task) {
   DCHECK(compressed_data);
 
   base::File file(file_path,
                   base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
   DCHECK(file.IsValid());
 
-  compressed_data->lockPixels();
   bool success = true;
-  int content_width = content_size.width();
-  int content_height = content_size.height();
-  int data_width = compressed_data->info().width();
-  int data_height = compressed_data->info().height();
 
-  if (file.WriteAtCurrentPos(reinterpret_cast<const char*>(&kCompressedKey),
-                             sizeof(int)) < 0 ||
-      file.WriteAtCurrentPos(reinterpret_cast<const char*>(&content_width),
-                             sizeof(int)) < 0 ||
-      file.WriteAtCurrentPos(reinterpret_cast<const char*>(&content_height),
-                             sizeof(int)) < 0 ||
-      file.WriteAtCurrentPos(reinterpret_cast<const char*>(&data_width),
-                             sizeof(int)) < 0 ||
-      file.WriteAtCurrentPos(reinterpret_cast<const char*>(&data_height),
-                             sizeof(int)) < 0 ||
-      file.WriteAtCurrentPos(reinterpret_cast<const char*>(&scale),
-                             sizeof(float)) < 0) {
+  if (file.IsValid()) {

[Ted C, 2014/08/19 16:51:09]

Would it be possible to pull out a helper function that simply takes the file
ref, compressed_data, scale, and content_size and returns a boolean instead of
tracking success like this?

Would be nice to early out instead of having to continually check for success on
each step.

You would probably need to lockPixels/unlockPixels before entering the function,
but I think it should clarify this code.

[David Trainor- moved to gerrit, 2014/08/19 18:31:06]

Done.

+    if (!WriteBigEndianToFile(file, kCompressedKey))
+      success = false;
+
+    if (success && !WriteBigEndianToFile(file, content_size.width()))
+      success = false;
+
+    if (success && !WriteBigEndianToFile(file, content_size.height()))
+      success = false;
+
+    // Write ETC1 header.
+    compressed_data->lockPixels();
+
+    int bytes_written = 0;
+    unsigned char etc1_buffer[ETC_PKM_HEADER_SIZE];

[Ted C, 2014/08/19 16:51:09]

any reason these lines don't go into the conditional right below?  I would
probably redefine bytes_written in each one just for clarity as well if you do
move it.

[David Trainor- moved to gerrit, 2014/08/19 18:31:06]

Done.

+    etc1_pkm_format_header(etc1_buffer,
+                           compressed_data->info().width(),
+                           compressed_data->info().height());
+
+    if (success) {
+      bytes_written = file.WriteAtCurrentPos(
+          reinterpret_cast<char*>(etc1_buffer), ETC_PKM_HEADER_SIZE);
+      if (bytes_written != ETC_PKM_HEADER_SIZE)
+        success = false;
+    }
+
+    if (success) {
+      int data_size = etc1_get_encoded_data_size(
+          compressed_data->info().width(),
+          compressed_data->info().height());
+      bytes_written = file.WriteAtCurrentPos(
+          reinterpret_cast<char*>(compressed_data->pixels()),
+          data_size);
+      if (bytes_written != data_size)
+        success = false;
+    }
+
+    compressed_data->unlockPixels();
+
+    if (success && !WriteBigEndianToFile(file, kCurrentExtraVersion))
+      success = false;
+
+    if (success) {
+      scale = 1.f / scale;
+
+      char buffer[4];
+      memcpy(buffer, &scale, sizeof(buffer));
+
+      char tmp = buffer[0];
+      buffer[0] = buffer[3];
+      buffer[3] = tmp;
+
+      tmp = buffer[1];
+      buffer[1] = buffer[2];
+      buffer[2] = tmp;
+
+      if (file.WriteAtCurrentPos(buffer, sizeof(buffer)) != sizeof(buffer))
+        success = false;
+    }
+  } else {
     success = false;
   }
 
-  size_t compressed_bytes = etc1_get_encoded_data_size(data_width, data_height);
-  if (file.WriteAtCurrentPos(reinterpret_cast<char*>(compressed_data->pixels()),
-                             compressed_bytes) < 0)
-    success = false;
-
-  compressed_data->unlockPixels();
-
   file.Close();
 
   if (!success)
     base::DeleteFile(file_path, false);
 
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE, post_write_task);
 }
 
 void ThumbnailStore::PostWriteTask() {
@@ skipping 78 matching lines @@
         void(skia::RefPtr<SkPixelRef>, float, const gfx::Size&)>&
         post_read_task) {
   skia::RefPtr<SkPixelRef> compressed_data;
   float scale = 0.f;
   gfx::Size content_size;
 
   if (base::PathExists(file_path)) {
     base::File file(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
     DCHECK(file.IsValid());
 
-    int key;
     bool success = true;
-    if (file.ReadAtCurrentPos(reinterpret_cast<char*>(&key), sizeof(int)) < 0 ||
-        key != kCompressedKey)
+    int key = 0;

[Ted C, 2014/08/19 16:51:09]

same general comment about success early bail out

[David Trainor- moved to gerrit, 2014/08/19 18:31:06]

Done.

+    if (!ReadBigEndianFromFile(file, &key))
       success = false;
 
-    int width = 0;
-    int height = 0;
-    if (file.ReadAtCurrentPos(reinterpret_cast<char*>(&width), sizeof(int)) <
-            0 ||
-        file.ReadAtCurrentPos(reinterpret_cast<char*>(&height), sizeof(int)) <
-            0)
+    if (key != kCompressedKey)
       success = false;
 
-    content_size = gfx::Size(width, height);
-    if (file.ReadAtCurrentPos(reinterpret_cast<char*>(&width), sizeof(int)) <
-            0 ||
-        file.ReadAtCurrentPos(reinterpret_cast<char*>(&height), sizeof(int)) <
-            0)
+
+    int content_width = 0;
+    if (!ReadBigEndianFromFile(file, &content_width))
       success = false;
 
-    gfx::Size data_size(width, height);
-    if (file.ReadAtCurrentPos(reinterpret_cast<char*>(&scale), sizeof(float)) <
-        0)
+    int content_height = 0;
+    if (!ReadBigEndianFromFile(file, &content_height))
       success = false;
 
-    size_t compressed_bytes =
-        etc1_get_encoded_data_size(data_size.width(), data_size.height());
-    SkImageInfo info = {data_size.width(),
-                        data_size.height(),
-                        kUnknown_SkColorType,
-                        kUnpremul_SkAlphaType};
+    content_size.SetSize(content_width, content_height);
 
-    scoped_ptr<uint8_t[]> data(new uint8_t[compressed_bytes]);
-    if (file.ReadAtCurrentPos(reinterpret_cast<char*>(data.get()),
-                              compressed_bytes) < 0)
+    // Read ETC1 header.
+    int bytes_read = 0;
+    unsigned char etc1_buffer[ETC_PKM_HEADER_SIZE];
+    bytes_read = file.ReadAtCurrentPos(reinterpret_cast<char*>(etc1_buffer),
+                                       ETC_PKM_HEADER_SIZE);
+    if (bytes_read != ETC_PKM_HEADER_SIZE) {

[Ted C, 2014/08/19 16:51:09]

no need for parens here since it's inconsistent

[David Trainor- moved to gerrit, 2014/08/19 18:31:07]

Done.

       success = false;
+    }
+
+    if (!etc1_pkm_is_valid(etc1_buffer))
+      success = false;
+
+    int raw_width = 0;
+    if (success)
+      raw_width = etc1_pkm_get_width(etc1_buffer);
+
+    int raw_height = 0;
+    if (success)
+      raw_height = etc1_pkm_get_height(etc1_buffer);
+
+    // Do some simple sanity check validation.  This is for Android only, for

[Ted C, 2014/08/19 16:51:09]

this directory is under android, so I don't think you need this comment.

Also, I wonder if this could be smarter/future proof, by using 

std::max(
    DeviceDisplayInfo::GetDisplayHeight(),
    DeviceDisplayInfo::GetDisplayWidth());

[David Trainor- moved to gerrit, 2014/08/19 18:31:07]

Ah good point I forgot we could access those.  Thanks!
On 2014/08/19 16:51:09, Ted C wrote:

+    // other platforms we'll have to be a bit smarter.  Anything over 2048x2048
+    // won't load.
+    if (content_width > kMaxDimensionSize
+        || content_height > kMaxDimensionSize
+        || raw_width > kMaxDimensionSize
+        || raw_height > kMaxDimensionSize) {
+      success = false;
+    }
+
+    skia::RefPtr<SkData> etc1_pixel_data;
+    if (success) {
+      int data_size = etc1_get_encoded_data_size(raw_width, raw_height);
+      scoped_ptr<uint8_t[]> raw_data =
+          scoped_ptr<uint8_t[]>(new uint8_t[data_size]);
+
+      bytes_read = file.ReadAtCurrentPos(
+          reinterpret_cast<char*>(raw_data.get()),
+          data_size);
+
+      if (bytes_read == data_size) {
+        SkImageInfo info = {raw_width,
+                            raw_height,
+                            kUnknown_SkColorType,
+                            kUnpremul_SkAlphaType};
+
+        etc1_pixel_data = skia::AdoptRef(
+            SkData::NewFromMalloc(raw_data.release(), data_size));
+        compressed_data = skia::AdoptRef(
+            SkMallocPixelRef::NewWithData(info,
+                                          0,
+                                          NULL,
+                                          etc1_pixel_data.get()));
+      } else {
+        success = false;
+      }
+    }
+
+    int extra_data_version = 0;
+    if (!ReadBigEndianFromFile(file, &extra_data_version))
+      success = false;
+
+    scale = 1.f;
+    if (success && extra_data_version == 1) {
+      char buffer[4];
+      if (file.ReadAtCurrentPos(buffer, sizeof(buffer)) == sizeof(buffer)) {
+        char tmp = buffer[0];
+        buffer[0] = buffer[3];
+        buffer[3] = tmp;
+
+        tmp = buffer[1];
+        buffer[1] = buffer[2];
+        buffer[2] = tmp;
+
+        memcpy(&scale, buffer, sizeof(buffer));
+
+        if (scale == 0.f) {
+          success = false;
+        } else {
+          scale = 1.f / scale;
+        }
+      } else {
+        success = false;
+      }
+    }
 
     file.Close();
 
-    skia::RefPtr<SkData> etc1_pixel_data =
-        skia::AdoptRef(SkData::NewFromMalloc(data.release(), compressed_bytes));
-    compressed_data = skia::AdoptRef(
-        SkMallocPixelRef::NewWithData(info, 0, NULL, etc1_pixel_data.get()));
-
     if (!success) {
       compressed_data.clear();
       content_size = gfx::Size();
       scale = 0.f;
       base::DeleteFile(file_path, false);
     }
   }
 
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
@@ skipping 82 matching lines @@
   dst_bitmap.eraseColor(0);
   SkAutoLockPixels dst_bitmap_lock(dst_bitmap);
 
   SkCanvas canvas(dst_bitmap);
   canvas.scale(new_scale, new_scale);
   canvas.drawBitmap(bitmap, 0, 0, NULL);
   dst_bitmap.setImmutable();
 
   return std::make_pair(dst_bitmap, new_scale * scale);
 }