diff options
-rw-r--r-- | base/file_util_win.cc | 9 | ||||
-rw-r--r-- | chrome/browser/extensions/convert_user_script.cc | 10 | ||||
-rw-r--r-- | chrome/browser/extensions/convert_web_app.cc | 10 | ||||
-rw-r--r-- | chrome/browser/extensions/crx_installer.cc | 11 | ||||
-rw-r--r-- | chrome/browser/extensions/sandboxed_extension_unpacker.cc | 42 | ||||
-rw-r--r-- | chrome/browser/extensions/sandboxed_extension_unpacker.h | 10 | ||||
-rw-r--r-- | chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc | 16 | ||||
-rw-r--r-- | chrome/common/chrome_paths.cc | 3 | ||||
-rw-r--r-- | chrome/common/chrome_paths.h | 5 | ||||
-rw-r--r-- | chrome/common/extensions/extension_file_util.cc | 72 | ||||
-rw-r--r-- | chrome/common/extensions/extension_file_util.h | 8 |
11 files changed, 151 insertions, 45 deletions
diff --git a/base/file_util_win.cc b/base/file_util_win.cc index 7476b53..51bcb4e 100644 --- a/base/file_util_win.cc +++ b/base/file_util_win.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -667,9 +667,10 @@ bool CreateDirectory(const FilePath& full_path) { if (!::CreateDirectory(full_path_str, NULL)) { DWORD error_code = ::GetLastError(); if (error_code == ERROR_ALREADY_EXISTS && DirectoryExists(full_path)) { - // This error code doesn't indicate whether we were racing with someone - // creating the same directory, or a file with the same path, therefore - // we check. + // This error code ERROR_ALREADY_EXISTS doesn't indicate whether we + // were racing with someone creating the same directory, or a file + // with the same path. If DirectoryExists() returns true, we lost the + // race to create the same directory. return true; } else { LOG(WARNING) << "Failed to create directory " << full_path_str diff --git a/chrome/browser/extensions/convert_user_script.cc b/chrome/browser/extensions/convert_user_script.cc index 7322c70..0b45da8 100644 --- a/chrome/browser/extensions/convert_user_script.cc +++ b/chrome/browser/extensions/convert_user_script.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -18,6 +18,7 @@ #include "chrome/common/chrome_paths.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_constants.h" +#include "chrome/common/extensions/extension_file_util.h" #include "chrome/common/extensions/user_script.h" #include "chrome/common/json_value_serializer.h" #include "googleurl/src/gurl.h" @@ -45,8 +46,11 @@ scoped_refptr<Extension> ConvertUserScriptToExtension( return NULL; } - FilePath user_data_temp_dir; - CHECK(PathService::Get(chrome::DIR_USER_DATA_TEMP, &user_data_temp_dir)); + FilePath user_data_temp_dir = extension_file_util::GetUserDataTempDir(); + if (user_data_temp_dir.empty()) { + *error = "Could not get path to profile temporary directory."; + return NULL; + } ScopedTempDir temp_dir; if (!temp_dir.CreateUniqueTempDirUnderPath(user_data_temp_dir)) { diff --git a/chrome/browser/extensions/convert_web_app.cc b/chrome/browser/extensions/convert_web_app.cc index e1c8c92..d410f2a 100644 --- a/chrome/browser/extensions/convert_web_app.cc +++ b/chrome/browser/extensions/convert_web_app.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -22,6 +22,7 @@ #include "chrome/common/chrome_paths.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_constants.h" +#include "chrome/common/extensions/extension_file_util.h" #include "chrome/common/json_value_serializer.h" #include "chrome/common/web_apps.h" #include "gfx/codec/png_codec.h" @@ -84,8 +85,11 @@ std::string ConvertTimeToExtensionVersion(const Time& create_time) { scoped_refptr<Extension> ConvertWebAppToExtension( const WebApplicationInfo& web_app, const Time& create_time) { - FilePath user_data_temp_dir; - CHECK(PathService::Get(chrome::DIR_USER_DATA_TEMP, &user_data_temp_dir)); + FilePath user_data_temp_dir = extension_file_util::GetUserDataTempDir(); + if (user_data_temp_dir.empty()) { + LOG(ERROR) << "Could not get path to profile temporary directory."; + return NULL; + } ScopedTempDir temp_dir; if (!temp_dir.CreateUniqueTempDirUnderPath(user_data_temp_dir)) { diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc index 1593aea..f62c983 100644 --- a/chrome/browser/extensions/crx_installer.cc +++ b/chrome/browser/extensions/crx_installer.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -117,18 +117,9 @@ CrxInstaller::~CrxInstaller() { void CrxInstaller::InstallCrx(const FilePath& source_file) { source_file_ = source_file; - FilePath user_data_temp_dir; - { - // We shouldn't be doing disk IO on the UI thread. - // http://code.google.com/p/chromium/issues/detail?id=60634 - base::ThreadRestrictions::ScopedAllowIO allow_io; - CHECK(PathService::Get(chrome::DIR_USER_DATA_TEMP, &user_data_temp_dir)); - } - scoped_refptr<SandboxedExtensionUnpacker> unpacker( new SandboxedExtensionUnpacker( source_file, - user_data_temp_dir, g_browser_process->resource_dispatcher_host(), this)); diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.cc b/chrome/browser/extensions/sandboxed_extension_unpacker.cc index 6ae14a2..c43b83d 100644 --- a/chrome/browser/extensions/sandboxed_extension_unpacker.cc +++ b/chrome/browser/extensions/sandboxed_extension_unpacker.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -12,12 +12,15 @@ #include "base/file_util.h" #include "base/file_util_proxy.h" #include "base/message_loop.h" +#include "base/metrics/histogram.h" +#include "base/path_service.h" #include "base/scoped_handle.h" #include "base/task.h" #include "base/utf_string_conversions.h" // TODO(viettrungluu): delete me. #include "chrome/browser/browser_thread.h" #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/renderer_host/resource_dispatcher_host.h" +#include "chrome/common/chrome_paths.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_constants.h" @@ -33,28 +36,44 @@ const char SandboxedExtensionUnpacker::kExtensionHeaderMagic[] = "Cr24"; SandboxedExtensionUnpacker::SandboxedExtensionUnpacker( const FilePath& crx_path, - const FilePath& temp_path, ResourceDispatcherHost* rdh, SandboxedExtensionUnpackerClient* client) - : crx_path_(crx_path), temp_path_(temp_path), + : crx_path_(crx_path), thread_identifier_(BrowserThread::ID_COUNT), rdh_(rdh), client_(client), got_response_(false) { } -void SandboxedExtensionUnpacker::Start() { - // We assume that we are started on the thread that the client wants us to do - // file IO on. +bool SandboxedExtensionUnpacker::CreateTempDirectory() { CHECK(BrowserThread::GetCurrentThreadIdentifier(&thread_identifier_)); - // Create a temporary directory to work in. - if (!temp_dir_.CreateUniqueTempDirUnderPath(temp_path_)) { - // Could not create temporary directory. + FilePath user_data_temp_dir = extension_file_util::GetUserDataTempDir(); + if (user_data_temp_dir.empty()) { + // TODO(skerner): This should have its own string. + // Using an existing string so that the change can be merged. ReportFailure(l10n_util::GetStringFUTF8( IDS_EXTENSION_PACKAGE_INSTALL_ERROR, ASCIIToUTF16("COULD_NOT_CREATE_TEMP_DIRECTORY"))); - return; + return false; + } + + if (!temp_dir_.CreateUniqueTempDirUnderPath(user_data_temp_dir)) { + ReportFailure(l10n_util::GetStringFUTF8( + IDS_EXTENSION_PACKAGE_INSTALL_ERROR, + ASCIIToUTF16("COULD_NOT_CREATE_TEMP_DIRECTORY"))); + return false; } + return true; +} + +void SandboxedExtensionUnpacker::Start() { + // We assume that we are started on the thread that the client wants us to do + // file IO on. + CHECK(BrowserThread::GetCurrentThreadIdentifier(&thread_identifier_)); + + if (!CreateTempDirectory()) + return; // ReportFailure() already called. + // Initialize the path that will eventually contain the unpacked extension. extension_root_ = temp_dir_.path().AppendASCII( extension_filenames::kTempExtensionName); @@ -311,10 +330,13 @@ bool SandboxedExtensionUnpacker::ValidateSignature() { } void SandboxedExtensionUnpacker::ReportFailure(const std::string& error) { + UMA_HISTOGRAM_COUNTS("Extensions.SandboxUnpackFailure", 1); client_->OnUnpackFailure(error); } void SandboxedExtensionUnpacker::ReportSuccess() { + UMA_HISTOGRAM_COUNTS("Extensions.SandboxUnpackSuccess", 1); + // Client takes ownership of temporary directory and extension. client_->OnUnpackSuccess(temp_dir_.Take(), extension_root_, extension_); extension_ = NULL; diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.h b/chrome/browser/extensions/sandboxed_extension_unpacker.h index e47b26c..455e3f6 100644 --- a/chrome/browser/extensions/sandboxed_extension_unpacker.h +++ b/chrome/browser/extensions/sandboxed_extension_unpacker.h @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -93,7 +93,6 @@ class SandboxedExtensionUnpacker : public UtilityProcessHost::Client { // |client| with the result. If |rdh| is provided, unpacking is done in a // sandboxed subprocess. Otherwise, it is done in-process. SandboxedExtensionUnpacker(const FilePath& crx_path, - const FilePath& temp_path, ResourceDispatcherHost* rdh, SandboxedExtensionUnpackerClient* cilent); @@ -107,6 +106,10 @@ class SandboxedExtensionUnpacker : public UtilityProcessHost::Client { virtual ~SandboxedExtensionUnpacker(); + // Set |temp_dir_| as a temporary directory to unpack the extension in. + // Return true on success. + virtual bool CreateTempDirectory(); + // Validates the signature of the extension and extract the key to // |public_key_|. Returns true if the signature validates, false otherwise. // @@ -141,9 +144,6 @@ class SandboxedExtensionUnpacker : public UtilityProcessHost::Client { // The path to the CRX to unpack. FilePath crx_path_; - // A path to a temp dir to unpack in. - FilePath temp_path_; - // Our client's thread. This is the thread we respond on. BrowserThread::ID thread_identifier_; diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc b/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc index 6246a6f..ecc521c 100644 --- a/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc +++ b/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -22,12 +22,16 @@ namespace keys = extension_manifest_keys; using testing::_; using testing::Invoke; +namespace { + void OnUnpackSuccess(const FilePath& temp_dir, const FilePath& extension_root, const Extension* extension) { // Don't delete temp_dir here, we need to do some post op checking. } +} // namespace + class MockSandboxedExtensionUnpackerClient : public SandboxedExtensionUnpackerClient { public: @@ -78,7 +82,7 @@ class SandboxedExtensionUnpackerTest : public testing::Test { // a temp folder to play in. ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &install_dir_)); install_dir_ = - install_dir_.AppendASCII("sandboxed_extension_unpacker_test"); + install_dir_.AppendASCII("sandboxed_extension_unpacker_test"); file_util::Delete(install_dir_, true); file_util::CreateDirectory(install_dir_); @@ -89,14 +93,14 @@ class SandboxedExtensionUnpackerTest : public testing::Test { unpacker_.reset(new ExtensionUnpacker(crx_path)); - // Build a temp area where the extension will be unpacked. ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &temp_dir_)); temp_dir_ = temp_dir_.AppendASCII("sandboxed_extension_unpacker_test_Temp"); - file_util::CreateDirectory(temp_dir_); + ASSERT_TRUE(file_util::CreateDirectory(temp_dir_)); sandboxed_unpacker_ = - new SandboxedExtensionUnpacker(crx_path, temp_dir_, NULL, client_); + new SandboxedExtensionUnpacker(crx_path, NULL, client_); + // Hack since SandboxedExtensionUnpacker gets its background thread id from // the Start call, but we don't call it here. sandboxed_unpacker_->thread_identifier_ = BrowserThread::FILE; @@ -159,6 +163,7 @@ class SandboxedExtensionUnpackerTest : public testing::Test { TEST_F(SandboxedExtensionUnpackerTest, NoCatalogsSuccess) { EXPECT_CALL(*client_, OnUnpackSuccess(_, _, _)); + EXPECT_CALL(*client_, OnUnpackFailure(_)).Times(0); SetupUnpacker("no_l10n.crx"); ASSERT_TRUE(unpacker_->Run()); @@ -180,6 +185,7 @@ TEST_F(SandboxedExtensionUnpackerTest, NoCatalogsSuccess) { TEST_F(SandboxedExtensionUnpackerTest, WithCatalogsSuccess) { EXPECT_CALL(*client_, OnUnpackSuccess(_, _, _)); + EXPECT_CALL(*client_, OnUnpackFailure(_)).Times(0); SetupUnpacker("good_l10n.crx"); ASSERT_TRUE(unpacker_->Run()); diff --git a/chrome/common/chrome_paths.cc b/chrome/common/chrome_paths.cc index 344db91..51aa019 100644 --- a/chrome/common/chrome_paths.cc +++ b/chrome/common/chrome_paths.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2006-2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -182,7 +182,6 @@ bool PathProvider(int key, FilePath* result) { if (!PathService::Get(chrome::DIR_USER_DATA, &cur)) return false; cur = cur.Append(FILE_PATH_LITERAL("Temp")); - create_dir = true; break; case chrome::DIR_INTERNAL_PLUGINS: if (!GetInternalPluginsDirectory(&cur)) diff --git a/chrome/common/chrome_paths.h b/chrome/common/chrome_paths.h index 21867bd..09fe1b6 100644 --- a/chrome/common/chrome_paths.h +++ b/chrome/common/chrome_paths.h @@ -1,4 +1,4 @@ -// Copyright (c) 2006-2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -35,6 +35,9 @@ enum { // this when a temporary file or directory will // be moved into the profile, to avoid issues // moving across volumes. See crbug.com/13044 . + // Getting this path does not create it. Users + // should check that the path exists before + // using it. DIR_INTERNAL_PLUGINS, // Directory where internal plugins reside. #if !defined(OS_MACOSX) && defined(OS_POSIX) DIR_POLICY_FILES, // Directory for system-wide read-only diff --git a/chrome/common/extensions/extension_file_util.cc b/chrome/common/extensions/extension_file_util.cc index e8636be..9472f03 100644 --- a/chrome/common/extensions/extension_file_util.cc +++ b/chrome/common/extensions/extension_file_util.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -10,8 +10,12 @@ #include "app/l10n_util.h" #include "base/file_util.h" #include "base/logging.h" +#include "base/metrics/histogram.h" +#include "base/path_service.h" #include "base/scoped_temp_dir.h" +#include "base/threading/thread_restrictions.h" #include "base/utf_string_conversions.h" +#include "chrome/common/chrome_paths.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_action.h" #include "chrome/common/extensions/extension_l10n_util.h" @@ -517,4 +521,70 @@ FilePath ExtensionURLToRelativeFilePath(const GURL& url) { return path; } +FilePath GetUserDataTempDir() { + // We do file IO in this function, but only when the current profile's + // Temp directory has never been used before, or in a rare error case. + // Developers are not likely to see these situations often, so do an + // explicit thread check. + base::ThreadRestrictions::AssertIOAllowed(); + + // Getting chrome::DIR_USER_DATA_TEMP is failing. Use histogram to see why. + // TODO(skerner): Fix the problem, and remove this code. crbug.com/70056 + enum DirectoryCreationResult { + SUCCESS = 0, + + CANT_GET_PARENT_PATH, + CANT_GET_UDT_PATH, + NOT_A_DIRECTORY, + CANT_CREATE_DIR, + CANT_WRITE_TO_PATH, + + UNSET, + NUM_DIRECTORY_CREATION_RESULTS + }; + + // All paths should set |result|. + DirectoryCreationResult result = UNSET; + + FilePath temp_path; + if (!PathService::Get(chrome::DIR_USER_DATA_TEMP, &temp_path)) { + FilePath parent_path; + if (!PathService::Get(chrome::DIR_USER_DATA, &parent_path)) + result = CANT_GET_PARENT_PATH; + else + result = CANT_GET_UDT_PATH; + + } else if (file_util::PathExists(temp_path)) { + + // Path exists. Check that it is a directory we can write to. + if (!file_util::DirectoryExists(temp_path)) { + result = NOT_A_DIRECTORY; + + } else if (!file_util::PathIsWritable(temp_path)) { + result = CANT_WRITE_TO_PATH; + + } else { + // Temp is a writable directory. + result = SUCCESS; + } + + } else if (!file_util::CreateDirectory(temp_path)) { + // Path doesn't exist, and we failed to create it. + result = CANT_CREATE_DIR; + + } else { + // Successfully created the Temp directory. + result = SUCCESS; + } + + UMA_HISTOGRAM_ENUMERATION("Extensions.GetUserDataTempDir", + result, + NUM_DIRECTORY_CREATION_RESULTS); + + if (result == SUCCESS) + return temp_path; + + return FilePath(); +} + } // namespace extension_file_util diff --git a/chrome/common/extensions/extension_file_util.h b/chrome/common/extensions/extension_file_util.h index f6a72bc..b26b564 100644 --- a/chrome/common/extensions/extension_file_util.h +++ b/chrome/common/extensions/extension_file_util.h @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 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. @@ -76,6 +76,12 @@ bool CheckForIllegalFilenames(const FilePath& extension_path, // Get a relative file path from a chrome-extension:// URL. FilePath ExtensionURLToRelativeFilePath(const GURL& url); +// Get a path to a temp directory for unpacking an extension. +// This is essentially PathService::Get(chrome::DIR_USER_DATA_TEMP, ...), +// with a histogram that allows us to understand why it is failing. +// Return an empty file path on failure. +FilePath GetUserDataTempDir(); + } // namespace extension_file_util #endif // CHROME_COMMON_EXTENSIONS_EXTENSION_FILE_UTIL_H_ |