Fix thumbnail store crashing on loading old files

chrome/browser/android/thumbnail/thumbnail_store.cc

Index: chrome/browser/android/thumbnail/thumbnail_store.cc
diff --git a/chrome/browser/android/thumbnail/thumbnail_store.cc b/chrome/browser/android/thumbnail/thumbnail_store.cc
index 1df7faf7eaf416de2be596aff1482bc22bacb2c1..6e61458c12b06000bc4d398a7c643b5a110f206b 100644
--- a/chrome/browser/android/thumbnail/thumbnail_store.cc
+++ b/chrome/browser/android/thumbnail/thumbnail_store.cc
@@ -7,6 +7,7 @@
 #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"
@@ -31,6 +32,8 @@ 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;
@@ -53,6 +56,25 @@ gfx::Size GetEncodedSize(const gfx::Size& bitmap_size) {
                    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,
@@ -417,34 +439,70 @@ void ThumbnailStore::WriteTask(const base::FilePath& 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) {
-    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;
+  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;
 
-  compressed_data->unlockPixels();
+    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;
+  }
 
   file.Close();
 
@@ -544,50 +602,112 @@ void ThumbnailStore::ReadTask(
     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;
+    }
 
-    file.Close();
+    if (!etc1_pkm_is_valid(etc1_buffer))
+      success = false;
 
-    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()));
+    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();
 
     if (!success) {
       compressed_data.clear();