Support asynchronous patching operations in the component updater.

chrome/browser/component_updater/component_unpacker.cc

 // Copyright (c) 2012 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/component_updater/component_unpacker.h"
 
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/file_util.h"
+#include "base/files/file_path.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_handle.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/component_updater/component_patcher.h"
 #include "chrome/browser/component_updater/component_updater_service.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "content/public/browser/browser_thread.h"
 #include "crypto/secure_hash.h"
 #include "crypto/signature_verifier.h"
 #include "extensions/common/crx_file.h"
 #include "third_party/zlib/google/zip.h"
 
 using crypto::SecureHash;
 
 namespace component_updater {
 
 namespace {
 
 // This class makes sure that the CRX digital signature is valid
 // and well formed.
 class CRXValidator {
  public:
-  explicit CRXValidator(FILE* crx_file) : valid_(false), delta_(false) {
+  explicit CRXValidator(FILE* crx_file) : valid_(false), is_delta_(false) {
     extensions::CrxFile::Header header;
     size_t len = fread(&header, 1, sizeof(header), crx_file);
     if (len < sizeof(header))
       return;
 
     extensions::CrxFile::Error error;
     scoped_ptr<extensions::CrxFile> crx(
         extensions::CrxFile::Parse(header, &error));
     if (!crx.get())
       return;
-    delta_ = extensions::CrxFile::HeaderIsDelta(header);
+    is_delta_ = extensions::CrxFile::HeaderIsDelta(header);
 
     std::vector<uint8> key(header.key_size);
     len = fread(&key[0], sizeof(uint8), header.key_size, crx_file);
     if (len < header.key_size)
       return;
 
     std::vector<uint8> signature(header.signature_size);
     len = fread(&signature[0], sizeof(uint8), header.signature_size, crx_file);
     if (len < header.signature_size)
       return;
@@ skipping 15 matching lines @@
 
     if (!verifier.VerifyFinal())
       return;
 
     public_key_.swap(key);
     valid_ = true;
   }
 
   bool valid() const { return valid_; }
 
-  bool delta() const { return delta_; }
+  bool is_delta() const { return is_delta_; }
 
   const std::vector<uint8>& public_key() const { return public_key_; }
 
  private:
   bool valid_;
-  bool delta_;
+  bool is_delta_;
   std::vector<uint8> public_key_;
 };
 
-}  // namespace.
+}  // namespace
+
+ComponentUnpacker::ComponentUnpacker(
+    const std::vector<uint8>& pk_hash,
+    const base::FilePath& path,
+    const std::string& fingerprint,
+    ComponentPatcher* patcher,
+    ComponentInstaller* installer,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : pk_hash_(pk_hash),
+      path_(path),
+      is_delta_(false),
+      fingerprint_(fingerprint),
+      patcher_(patcher),
+      installer_(installer),
+      error_(kNone),
+      extended_error_(0),
+      ptr_factory_(this),
+      task_runner_(task_runner) {
+}
 
 // TODO(cpu): add a specific attribute check to a component json that the
 // extension unpacker will reject, so that a component cannot be installed
 // as an extension.
 scoped_ptr<base::DictionaryValue> ReadManifest(
     const base::FilePath& unpack_path) {
   base::FilePath manifest =
       unpack_path.Append(FILE_PATH_LITERAL("manifest.json"));
   if (!base::PathExists(manifest))
     return scoped_ptr<base::DictionaryValue>();
   JSONFileValueSerializer serializer(manifest);
   std::string error;
   scoped_ptr<base::Value> root(serializer.Deserialize(NULL, &error));
   if (!root.get())
     return scoped_ptr<base::DictionaryValue>();
   if (!root->IsType(base::Value::TYPE_DICTIONARY))
     return scoped_ptr<base::DictionaryValue>();
   return scoped_ptr<base::DictionaryValue>(
       static_cast<base::DictionaryValue*>(root.release())).Pass();
 }
 
-ComponentUnpacker::ComponentUnpacker(const std::vector<uint8>& pk_hash,
-                                     const base::FilePath& path,
-                                     const std::string& fingerprint,
-                                     ComponentPatcher* patcher,
-                                     ComponentInstaller* installer)
-    : error_(kNone),
-      extended_error_(0) {
-  if (pk_hash.empty() || path.empty()) {
+// At the end of this execution, either Finish() will be called or
+// DonePatching() will be called asynchronously.
+void ComponentUnpacker::Unpack(

[Sorin Jianu, 2014/01/25 02:30:21]

We could have a function like:

bool UnpackInternal(...) {
if (!Verify())
  return false;
if (!Unzip())
  return false;
....
}

And then:
void ComponentUnpacker::Unpack(...) {
  if(!UnpackInternal())
    Finish();
}

Small fry...

[waffles, 2014/01/28 21:06:31]

Done.

+    const base::Callback<void(Error, int)>& callback) {
+  callback_ = callback;
+  if (!Verify()) {

[Sorin Jianu, 2014/01/25 02:30:21]

I knew it Verify would be the first step.

[waffles, 2014/01/28 21:06:31]

Acknowledged.

+    Finish();
+    return;
+  }
+  if (!Unzip()) {
+    Finish();
+    return;
+  }
+  if (!BeginPatching())
+    Finish();
+}
+
+bool ComponentUnpacker::Verify() {
+  if (pk_hash_.empty() || path_.empty()) {
     error_ = kInvalidParams;
-    return;
+    return false;
   }
   // First, validate the CRX header and signature. As of today
   // this is SHA1 with RSA 1024.
-  ScopedStdioHandle file(base::OpenFile(path, "rb"));
+  ScopedStdioHandle file(base::OpenFile(path_, "rb"));
   if (!file.get()) {
     error_ = kInvalidFile;
-    return;
+    return false;
   }
   CRXValidator validator(file.get());
+  file.Close();
   if (!validator.valid()) {
     error_ = kInvalidFile;
-    return;
+    return false;
   }
-  file.Close();
+  is_delta_ = validator.is_delta();
 
   // File is valid and the digital signature matches. Now make sure
   // the public key hash matches the expected hash. If they do we fully
   // trust this CRX.
-  uint8 hash[32];
+  uint8 hash[32] = {};
   scoped_ptr<SecureHash> sha256(SecureHash::Create(SecureHash::SHA256));
   sha256->Update(&(validator.public_key()[0]), validator.public_key().size());
   sha256->Finish(hash, arraysize(hash));
 
-  if (!std::equal(pk_hash.begin(), pk_hash.end(), hash)) {
+  if (!std::equal(pk_hash_.begin(), pk_hash_.end(), hash)) {
     error_ = kInvalidId;
+    return false;
+  }
+  return true;
+}
+
+bool ComponentUnpacker::Unzip() {
+  base::FilePath& destination = is_delta_ ? unpack_diff_path_ : unpack_path_;
+  if (!base::CreateNewTempDirectory(base::FilePath::StringType(),
+                                    &destination)) {
+    error_ = kUnzipPathError;
+    return false;
+  }
+  if (!zip::Unzip(path_, destination))
+    error_ = kUnzipFailed;

[Sorin Jianu, 2014/01/25 02:30:21]

is the code missing a return false here?

[waffles, 2014/01/28 21:06:31]

Yes, thanks. Done.

+  return true;
+}
+
+
+bool ComponentUnpacker::BeginPatching() {
+  if (is_delta_) {  // Package is a diff package.
+    // We want a different temp directory to put the patch output files into.

[Sorin Jianu, 2014/01/25 02:30:21]

I am in the definitely pet peeve territory here but I have nothing else but to
nit pick. I've been avoiding comments in the first person plural since I've come
to Google at the suggestion of Mark Mentovai. Sometimes is wierd to formulate
the comment but in this case it would sound like:
// Use a different temp directory for the patch output files.

[waffles, 2014/01/28 21:06:31]

Done. I agree, this one slipped in.

+    if (!base::CreateNewTempDirectory(base::FilePath::StringType(),
+                                      &unpack_path_)) {
+      error_ = kUnzipPathError;
+      return false;
+    }
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&DifferentialUpdatePatch,

[Sorin Jianu, 2014/01/25 02:30:21]

Can this function create the unpack_path and return it to the caller? It seems
something that can be encapsulated inside this function. And then, BeginPatching
becomes nice and simple: if this call that else call nop.

[waffles, 2014/01/28 21:06:31]

Let's discuss. I like Unpacker being responsible for all the paths, though I
agree that such a change is possible. Do you just want this function to be
simplified?

[Sorin Jianu, 2014/01/31 00:32:16]

I'll leave it up to you to structure the code. In general, I agree with a
certain symmetry when resources are acquired and released. At the same time, the
non-blocking aspect of the code can result in split ownership since it makes
certain c++ idioms impractical, such as scoping of resources.

I can certainly see how DifferentialUpdatePatch can create the path just like a
function can allocate memory and return it tp the caller.

Streamlining functions is a good thing.

+                              unpack_diff_path_,
+                              unpack_path_,
+                              patcher_,
+                              installer_,
+                              base::Bind(&ComponentUnpacker::DonePatching,
+                                         GetWeakPtr())));
+  } else {
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&ComponentUnpacker::DonePatching,
+                              GetWeakPtr(),
+                              kNone,
+                              0));
+  }
+  return true;
+}
+
+void ComponentUnpacker::DonePatching(Error error, int extended_error) {
+  error_ = error;
+  extended_error_ = extended_error;
+  if (error_ != kNone) {
+    Finish();
     return;
   }
-  if (!base::CreateNewTempDirectory(base::FilePath::StringType(),
-                                    &unpack_path_)) {
-    error_ = kUnzipPathError;
+  // Optimization: clean up patch files early, in case we're too low on disk to
+  // install otherwise.
+  if (!unpack_diff_path_.empty()) {

[Sorin Jianu, 2014/01/25 02:30:21]

Arguably, we can have DifferentialUpdatePatch delete the patch files upon
return. Then DonePatching becomes prettier and there would be no need for the
comment.

[waffles, 2014/01/28 21:06:31]

We could do that; I like having the ComponentUnpacker responsible for deleting
the path it creates, however.

+    base::DeleteFile(unpack_diff_path_, true);
+    unpack_diff_path_.clear();
+  }
+  Install();
+  Finish();
+}
+
+void ComponentUnpacker::Install() {
+  // Write the fingerprint to disk.

[Sorin Jianu, 2014/01/25 02:30:21]

Is the semantics of the Install that it copies/installs everything in the
unpack_path_ as the manifest.fingerprint would have been part of the original
CRX?

If yes, would it be more appropriate to have writing the fingerprint as a
separate step before Install()?

[waffles, 2014/01/28 21:06:31]

Personally I consider manifest.fingerprint a piece of metadata that we happen to
store alongside the contents of the CRX. I don't mind making an additional step
for it, though, if you prefer that.

[Sorin Jianu, 2014/01/31 00:32:16]

I have no preference, I am just offering an idea.

+  if (static_cast<int>(fingerprint_.size()) !=
+      file_util::WriteFile(
+          unpack_path_.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
+          fingerprint_.c_str(),
+          fingerprint_.size())) {
+    error_ = kFingerprintWriteFailed;
     return;
   }
-  if (validator.delta()) {  // Package is a diff package.
-    // We want a different temp directory for the delta files; we'll put the
-    // patch output into unpack_path_.
-    base::FilePath unpack_diff_path;
-    if (!base::CreateNewTempDirectory(base::FilePath::StringType(),
-                                      &unpack_diff_path)) {
-      error_ = kUnzipPathError;
+  if (error_ == kNone) {

[Sorin Jianu, 2014/01/25 02:30:21]

in which case error_ != kNone would be true at this point?

[waffles, 2014/01/28 21:06:31]

No cases, as far as I can tell. I've removed it.

[Sorin Jianu, 2014/01/31 00:32:16]

Seems a good place to assert that errors were handled as a preconditions to
calling this function.

+    scoped_ptr<base::DictionaryValue> manifest(ReadManifest(unpack_path_));
+    if (!manifest.get()) {
+      error_ = kBadManifest;
       return;
     }
-    if (!zip::Unzip(path, unpack_diff_path)) {
-      error_ = kUnzipFailed;
-      return;
-    }
-    ComponentUnpacker::Error result = DifferentialUpdatePatch(unpack_diff_path,
-                                                              unpack_path_,
-                                                              patcher,
-                                                              installer,
-                                                              &extended_error_);
-    base::DeleteFile(unpack_diff_path, true);
-    unpack_diff_path.clear();
-    error_ = result;
-    if (error_ != kNone) {
-      return;
-    }
-  } else {
-    // Package is a normal update/install; unzip it into unpack_path_ directly.
-    if (!zip::Unzip(path, unpack_path_)) {
-      error_ = kUnzipFailed;
+    if (!installer_->Install(*manifest, unpack_path_)) {
+      error_ = kInstallerError;
       return;
     }
   }
-  scoped_ptr<base::DictionaryValue> manifest(ReadManifest(unpack_path_));
-  if (!manifest.get()) {
-    error_ = kBadManifest;
-    return;
-  }
-  // Write the fingerprint to disk.
-  if (static_cast<int>(fingerprint.size()) !=
-      file_util::WriteFile(
-          unpack_path_.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
-          fingerprint.c_str(),
-          fingerprint.size())) {
-    error_ = kFingerprintWriteFailed;
-    return;
-  }
-  if (!installer->Install(*manifest, unpack_path_)) {
-    error_ = kInstallerError;
-    return;
-  }
-  // Installation successful. The directory is not our concern now.
-  unpack_path_.clear();
+}
+
+void ComponentUnpacker::Finish() {
+  if (!unpack_diff_path_.empty())
+    base::DeleteFile(unpack_diff_path_, true);
+  if (!unpack_path_.empty())
+    base::DeleteFile(unpack_path_, true);
+  callback_.Run(error_, extended_error_);
+}
+
+base::WeakPtr<ComponentUnpacker> ComponentUnpacker::GetWeakPtr() {
+  return ptr_factory_.GetWeakPtr();
 }
 
 ComponentUnpacker::~ComponentUnpacker() {
-  if (!unpack_path_.empty())
-    base::DeleteFile(unpack_path_, true);
 }
 
 }  // namespace component_updater
-