Validate image files before writing them to media galleries.

chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc

Index: chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
diff --git a/chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc b/chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..361d0b1a0c85f997713848e6efd6d9154931c261
--- /dev/null
+++ b/chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
@@ -0,0 +1,166 @@
+// Copyright 2013 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/media_galleries/fileapi/supported_image_type_validator.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/memory/scoped_generic_obj.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "base/threading/thread_restrictions.h"
+#include "chrome/browser/image_decoder.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace chrome {
+
+namespace {
+
+const int kMaxImageFileSize = 50*1014*1024;

[Greg Billock, 2013/05/22 16:21:59]

Do we have this limit available somewhere lower in the analysis code to use?

[vandebo (ex-Chrome), 2013/05/22 18:41:17]

Not sure what you mean.

[Greg Billock, 2013/05/23 18:18:29]

Meaning is there an ImageDecoder::MAX_FILE_SIZE or something.

[vandebo (ex-Chrome), 2013/05/24 01:13:05]

This isn't a limit of ImageDecoder, it's an arbitrary limit imposed her to
ensure that we don't spend too much time/memory trying to decode a potentially
malicious image. 

+
+class PlatformFileCloser {
+ public:
+  void operator()(base::PlatformFile file) const {
+    if (file != base::kInvalidPlatformFileValue)
+      base::ClosePlatformFile(file);
+  }
+};
+
+typedef ScopedGenericObj<base::PlatformFile, PlatformFileCloser>
+    ScopedPlatformFile;
+
+scoped_ptr<std::string> ReadOnFileThread(const base::FilePath& path) {

[Greg Billock, 2013/05/22 16:21:59]

Would it be better to pass the ScopedPlatformFile back and have the validator
read it? I could see us only needing to read a very small portion of the file
sometimes, but sometimes needing larger portions. And being able to consume less
memory seems nice.

nm, I see ImageDecoder needs the whole image.

+  base::ThreadRestrictions::AssertIOAllowed();
+  scoped_ptr<std::string> empty_result;
+
+  ScopedPlatformFile file(base::CreatePlatformFile(
+      path, base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, NULL, NULL));
+  if (file == base::kInvalidPlatformFileValue)
+    return empty_result.Pass();
+
+  base::PlatformFileInfo file_info;
+  if (!base::GetPlatformFileInfo(file.get(), &file_info) ||
+      file_info.size > kMaxImageFileSize) {

[Greg Billock, 2013/05/22 16:21:59]

Do we just want to read the first kMaxImageFileSize bytes? I think this supports
returning the file handle.

[vandebo (ex-Chrome), 2013/05/22 18:41:17]

ImageDecoder needs the whole file.  GetPlatformFileInfo is effectively stat().

[Greg Billock, 2013/05/23 18:18:29]

Yeah. I should have put this under ReadPlatformFile I guess. Anyway, if the
decoder needs the whole thing, then...

+    return empty_result.Pass();
+  }
+
+  scoped_ptr<char[]> data(new char[file_info.size]);
+  if (base::ReadPlatformFile(file, 0, data.get(),
+                             file_info.size) != file_info.size) {
+    return empty_result.Pass();
+  }
+
+  return make_scoped_ptr(new std::string(data.get(), file_info.size)).Pass();
+}
+
+class ImageDecoderDelegateAdapter : public ImageDecoder::Delegate {
+ public:
+  ImageDecoderDelegateAdapter(
+      scoped_ptr<std::string> data,
+      const fileapi::CopyOrMoveFileValidator::ResultCallback& callback)
+      : data_(data.Pass()),
+        callback_(callback) {
+    DCHECK(data_);
+  }
+
+  const std::string& data() {
+    return *data_;
+  }
+
+  // ImageDecoder::Delegate methods.
+  virtual void OnImageDecoded(const ImageDecoder* /*decoder*/,
+                              const SkBitmap& /*decoded_image*/) {
+    // Using base::Owned causes |this| to be deleted after the callback runs.
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&ImageDecoderDelegateAdapter::OnDecodeDoneOnIOThread,

[Greg Billock, 2013/05/22 16:21:59]

Can we just run the callback_ directly from here?
base::Bind(callback_, base::PLATFORM_FILE_???).

Then you could just call delete(this) or scoped_ptr(this) instead of using
base::Owned, as well, which is a bit more clear.

[vandebo (ex-Chrome), 2013/05/22 18:41:17]

I thought I tried that before and it didn't work.  Seems to work now.  Done. 
Done.

+                   base::Owned(this),
+                   base::PLATFORM_FILE_OK));
+  }
+
+  virtual void OnDecodeImageFailed(const ImageDecoder* /*decoder*/) {
+    // Using base::Owned causes |this| to be deleted after the callback runs.
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&ImageDecoderDelegateAdapter::OnDecodeDoneOnIOThread,
+                   base::Owned(this),
+                   base::PLATFORM_FILE_ERROR_SECURITY));
+  }
+
+ private:
+  void OnDecodeDoneOnIOThread(base::PlatformFileError result) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    callback_.Run(result);
+  }
+
+  scoped_ptr<std::string> data_;
+  fileapi::CopyOrMoveFileValidator::ResultCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(ImageDecoderDelegateAdapter);
+};
+
+}  // namespace
+
+SupportedImageTypeValidator::~SupportedImageTypeValidator() {}
+
+// static
+bool SupportedImageTypeValidator::SupportsFileType(const base::FilePath& path) {
+  base::FilePath::StringType extension = path.Extension();
+  return extension == FILE_PATH_LITERAL(".gif") ||

[Greg Billock, 2013/05/22 16:21:59]

Is this list going to grow big enough that we ought to put the extentions into a
list and iterate?

[vandebo (ex-Chrome), 2013/05/22 18:41:17]

I do not expect this list to grow.  Since this is a static method, I'd have to
use a lazy instance or something similar to construct the list, so I don't think
it's worthwhile.

+         extension == FILE_PATH_LITERAL(".png") ||
+         extension == FILE_PATH_LITERAL(".webp") ||
+         extension == FILE_PATH_LITERAL(".bmp") ||
+         extension == FILE_PATH_LITERAL(".jpg") ||

[Greg Billock, 2013/05/22 16:21:59]

How about sorting the list?

[vandebo (ex-Chrome), 2013/05/22 18:41:17]

Done.

+         extension == FILE_PATH_LITERAL(".jpeg") ||
+         extension == FILE_PATH_LITERAL(".jfif") ||
+         extension == FILE_PATH_LITERAL(".pjpeg") ||
+         extension == FILE_PATH_LITERAL(".pjp");
+}
+
+void SupportedImageTypeValidator::StartValidation(
+    const fileapi::CopyOrMoveFileValidator::ResultCallback& result_callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  callback_ = result_callback;
+
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&ReadOnFileThread, path_),
+      base::Bind(&SupportedImageTypeValidator::OnValidateData,
+                 weak_factory_.GetWeakPtr()));
+}
+
+SupportedImageTypeValidator::SupportedImageTypeValidator(
+    const base::FilePath& path)
+    : path_(path),
+      weak_factory_(this) {
+}
+
+void SupportedImageTypeValidator::OnValidateData(scoped_ptr<std::string> data) {

[Greg Billock, 2013/05/22 16:21:59]

I think this should be "OnOpenFile" or "OnReadFile". Given the name I'm
expecting the validation to be run above, not here.

[vandebo (ex-Chrome), 2013/05/22 18:41:17]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (!data.get()) {
+    callback_.Run(base::PLATFORM_FILE_ERROR_SECURITY);
+    return;
+  }
+
+  // |adapter| will delete itself after a completion message is received.
+  ImageDecoderDelegateAdapter* adapter =

[Greg Billock, 2013/05/22 16:21:59]

Is there a way we can hook this up earlier, so that the filesystem api runs the
validator while it is streaming the bytes out, instead of after they're already
received?

[vandebo (ex-Chrome), 2013/05/22 18:41:17]

I'm not sure what you mean.  This is validating a file before it is copied or
moved.  It is already on disk and we're going to copy or move it to a different
location.

[Greg Billock, 2013/05/23 18:18:29]

Right. If there were a hook we could insert this as it's getting written, that'd
be nice -- we already have it in memory at that point, so we wouldn't have to
work through the disk again.

[vandebo (ex-Chrome), 2013/05/24 01:13:05]

No, there isn't really a hook to do it while copying, but that would kind of
defeat the point, since we'd have to delete it if the validation failed. 
However, the OS buffer cache will save us in this case, the disk will probably
only get hit once.

+      new ImageDecoderDelegateAdapter(data.Pass(), callback_);
+  decoder_ = new ImageDecoder(adapter, adapter->data(),
+                              ImageDecoder::DEFAULT_CODEC);
+  base::SequencedWorkerPool* workerpool = BrowserThread::GetBlockingPool();
+  decoder_->Start(workerpool->GetSequencedTaskRunnerWithShutdownBehavior(
+        workerpool->GetSequenceToken(),
+        base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN));
+}
+
+}  // namespace chrome