Don't use ::GetFileVersionInfo() in CreateFileVersionInfoForModule()
Currently, base::FileVersionInfo::CreateFileVersionInfoForModule()
calls ::GetModuleFileName and ::GetFileVersionInfo, grabs the loader
lock and potentially touches the disk to obtain the VS_VERSION_INFO
of the module. This is gratuitous for a module that is already loaded.
With this CL, base::FileVersionInfo::CreateFileVersionInfoForModule()
uses base::win::GetResourceFromModule() to get the VS_VERSION_INFO
resource from memory.
First version of this CL: https://codereview.chromium.org/2046583002/
TBR=thestig@chromium.org
BUG=609709
Review-Url: https://codereview.chromium.org/2111613002
Cr-Commit-Position: refs/heads/master@{#402985}
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 4b498e6..afba0ab 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -1752,7 +1752,7 @@
"deferred_sequenced_task_runner_unittest.cc",
"environment_unittest.cc",
"feature_list_unittest.cc",
- "file_version_info_unittest.cc",
+ "file_version_info_win_unittest.cc",
"files/dir_reader_posix_unittest.cc",
"files/file_locking_unittest.cc",
"files/file_path_unittest.cc",
@@ -2058,8 +2058,6 @@
}
if (is_linux) {
- sources -= [ "file_version_info_unittest.cc" ]
-
if (is_desktop_linux) {
sources += [ "nix/xdg_util_unittest.cc" ]
}
diff --git a/base/base.gyp b/base/base.gyp
index ecc3384f..7e98715 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -407,7 +407,7 @@
'deferred_sequenced_task_runner_unittest.cc',
'environment_unittest.cc',
'feature_list_unittest.cc',
- 'file_version_info_unittest.cc',
+ 'file_version_info_win_unittest.cc',
'files/dir_reader_posix_unittest.cc',
'files/file_locking_unittest.cc',
'files/file_path_unittest.cc',
@@ -684,9 +684,6 @@
'defines': [
'USE_SYMBOLIZE',
],
- 'sources!': [
- 'file_version_info_unittest.cc',
- ],
'conditions': [
[ 'desktop_linux==1', {
'sources': [
diff --git a/base/file_version_info_unittest.cc b/base/file_version_info_unittest.cc
deleted file mode 100644
index ac5320f..0000000
--- a/base/file_version_info_unittest.cc
+++ /dev/null
@@ -1,143 +0,0 @@
-// 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.
-
-#include "base/file_version_info.h"
-
-#include <stddef.h>
-
-#include <memory>
-
-#include "base/files/file_path.h"
-#include "base/macros.h"
-#include "base/path_service.h"
-#include "build/build_config.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-#if defined(OS_WIN)
-#include "base/file_version_info_win.h"
-#endif
-
-using base::FilePath;
-
-namespace {
-
-#if defined(OS_WIN)
-FilePath GetTestDataPath() {
- FilePath path;
- PathService::Get(base::DIR_SOURCE_ROOT, &path);
- path = path.AppendASCII("base");
- path = path.AppendASCII("test");
- path = path.AppendASCII("data");
- path = path.AppendASCII("file_version_info_unittest");
- return path;
-}
-#endif
-
-} // namespace
-
-#if defined(OS_WIN)
-TEST(FileVersionInfoTest, HardCodedProperties) {
- const wchar_t kDLLName[] = {L"FileVersionInfoTest1.dll"};
-
- const wchar_t* const kExpectedValues[15] = {
- // FileVersionInfoTest.dll
- L"Goooooogle", // company_name
- L"Google", // company_short_name
- L"This is the product name", // product_name
- L"This is the product short name", // product_short_name
- L"The Internal Name", // internal_name
- L"4.3.2.1", // product_version
- L"Private build property", // private_build
- L"Special build property", // special_build
- L"This is a particularly interesting comment", // comments
- L"This is the original filename", // original_filename
- L"This is my file description", // file_description
- L"1.2.3.4", // file_version
- L"This is the legal copyright", // legal_copyright
- L"This is the legal trademarks", // legal_trademarks
- L"This is the last change", // last_change
- };
-
- FilePath dll_path = GetTestDataPath();
- dll_path = dll_path.Append(kDLLName);
-
- std::unique_ptr<FileVersionInfo> version_info(
- FileVersionInfo::CreateFileVersionInfo(dll_path));
-
- int j = 0;
- EXPECT_EQ(kExpectedValues[j++], version_info->company_name());
- EXPECT_EQ(kExpectedValues[j++], version_info->company_short_name());
- EXPECT_EQ(kExpectedValues[j++], version_info->product_name());
- EXPECT_EQ(kExpectedValues[j++], version_info->product_short_name());
- EXPECT_EQ(kExpectedValues[j++], version_info->internal_name());
- EXPECT_EQ(kExpectedValues[j++], version_info->product_version());
- EXPECT_EQ(kExpectedValues[j++], version_info->private_build());
- EXPECT_EQ(kExpectedValues[j++], version_info->special_build());
- EXPECT_EQ(kExpectedValues[j++], version_info->comments());
- EXPECT_EQ(kExpectedValues[j++], version_info->original_filename());
- EXPECT_EQ(kExpectedValues[j++], version_info->file_description());
- EXPECT_EQ(kExpectedValues[j++], version_info->file_version());
- EXPECT_EQ(kExpectedValues[j++], version_info->legal_copyright());
- EXPECT_EQ(kExpectedValues[j++], version_info->legal_trademarks());
- EXPECT_EQ(kExpectedValues[j++], version_info->last_change());
-}
-#endif
-
-#if defined(OS_WIN)
-TEST(FileVersionInfoTest, IsOfficialBuild) {
- const wchar_t* kDLLNames[] = {
- L"FileVersionInfoTest1.dll",
- L"FileVersionInfoTest2.dll"
- };
-
- const bool kExpected[] = {
- true,
- false,
- };
-
- // Test consistency check.
- ASSERT_EQ(arraysize(kDLLNames), arraysize(kExpected));
-
- for (size_t i = 0; i < arraysize(kDLLNames); ++i) {
- FilePath dll_path = GetTestDataPath();
- dll_path = dll_path.Append(kDLLNames[i]);
-
- std::unique_ptr<FileVersionInfo> version_info(
- FileVersionInfo::CreateFileVersionInfo(dll_path));
-
- EXPECT_EQ(kExpected[i], version_info->is_official_build());
- }
-}
-#endif
-
-#if defined(OS_WIN)
-TEST(FileVersionInfoTest, CustomProperties) {
- FilePath dll_path = GetTestDataPath();
- dll_path = dll_path.AppendASCII("FileVersionInfoTest1.dll");
-
- std::unique_ptr<FileVersionInfo> version_info(
- FileVersionInfo::CreateFileVersionInfo(dll_path));
-
- // Test few existing properties.
- std::wstring str;
- FileVersionInfoWin* version_info_win =
- static_cast<FileVersionInfoWin*>(version_info.get());
- EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 1", &str));
- EXPECT_EQ(L"Un", str);
- EXPECT_EQ(L"Un", version_info_win->GetStringValue(L"Custom prop 1"));
-
- EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 2", &str));
- EXPECT_EQ(L"Deux", str);
- EXPECT_EQ(L"Deux", version_info_win->GetStringValue(L"Custom prop 2"));
-
- EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 3", &str));
- EXPECT_EQ(L"1600 Amphitheatre Parkway Mountain View, CA 94043", str);
- EXPECT_EQ(L"1600 Amphitheatre Parkway Mountain View, CA 94043",
- version_info_win->GetStringValue(L"Custom prop 3"));
-
- // Test an non-existing property.
- EXPECT_FALSE(version_info_win->GetValue(L"Unknown property", &str));
- EXPECT_EQ(L"", version_info_win->GetStringValue(L"Unknown property"));
-}
-#endif
diff --git a/base/file_version_info_win.cc b/base/file_version_info_win.cc
index 02a14db..00261b7 100644
--- a/base/file_version_info_win.cc
+++ b/base/file_version_info_win.cc
@@ -6,48 +6,63 @@
#include <windows.h>
#include <stddef.h>
-#include <stdint.h>
-#include "base/file_version_info.h"
#include "base/files/file_path.h"
#include "base/logging.h"
-#include "base/macros.h"
#include "base/threading/thread_restrictions.h"
+#include "base/win/resource_util.h"
using base::FilePath;
-FileVersionInfoWin::FileVersionInfoWin(void* data,
- WORD language,
- WORD code_page)
- : language_(language), code_page_(code_page) {
- base::ThreadRestrictions::AssertIOAllowed();
- data_.reset((char*) data);
- fixed_file_info_ = NULL;
- UINT size;
- ::VerQueryValue(data_.get(), L"\\", (LPVOID*)&fixed_file_info_, &size);
-}
+namespace {
-FileVersionInfoWin::~FileVersionInfoWin() {
- DCHECK(data_.get());
-}
-
-typedef struct {
+struct LanguageAndCodePage {
WORD language;
WORD code_page;
-} LanguageAndCodePage;
+};
+
+// Returns the \\VarFileInfo\\Translation value extracted from the
+// VS_VERSION_INFO resource in |data|.
+LanguageAndCodePage* GetTranslate(const void* data) {
+ LanguageAndCodePage* translate = nullptr;
+ UINT length;
+ if (::VerQueryValue(data, L"\\VarFileInfo\\Translation",
+ reinterpret_cast<void**>(&translate), &length)) {
+ return translate;
+ }
+ return nullptr;
+}
+
+VS_FIXEDFILEINFO* GetVsFixedFileInfo(const void* data) {
+ VS_FIXEDFILEINFO* fixed_file_info = nullptr;
+ UINT length;
+ if (::VerQueryValue(data, L"\\", reinterpret_cast<void**>(&fixed_file_info),
+ &length)) {
+ return fixed_file_info;
+ }
+ return nullptr;
+}
+
+} // namespace
+
+FileVersionInfoWin::~FileVersionInfoWin() = default;
// static
FileVersionInfo* FileVersionInfo::CreateFileVersionInfoForModule(
HMODULE module) {
- // Note that the use of MAX_PATH is basically in line with what we do for
- // all registered paths (PathProviderWin).
- wchar_t system_buffer[MAX_PATH];
- system_buffer[0] = 0;
- if (!GetModuleFileName(module, system_buffer, MAX_PATH))
- return NULL;
+ void* data;
+ size_t version_info_length;
+ const bool has_version_resource = base::win::GetResourceFromModule(
+ module, VS_VERSION_INFO, RT_VERSION, &data, &version_info_length);
+ if (!has_version_resource)
+ return nullptr;
- FilePath app_path(system_buffer);
- return CreateFileVersionInfo(app_path);
+ const LanguageAndCodePage* translate = GetTranslate(data);
+ if (!translate)
+ return nullptr;
+
+ return new FileVersionInfoWin(data, translate->language,
+ translate->code_page);
}
// static
@@ -57,32 +72,21 @@
DWORD dummy;
const wchar_t* path = file_path.value().c_str();
- DWORD length = ::GetFileVersionInfoSize(path, &dummy);
+ const DWORD length = ::GetFileVersionInfoSize(path, &dummy);
if (length == 0)
- return NULL;
+ return nullptr;
- void* data = calloc(length, 1);
- if (!data)
- return NULL;
+ std::vector<uint8_t> data(length, 0);
- if (!::GetFileVersionInfo(path, dummy, length, data)) {
- free(data);
- return NULL;
- }
+ if (!::GetFileVersionInfo(path, dummy, length, data.data()))
+ return nullptr;
- LanguageAndCodePage* translate = NULL;
- uint32_t page_count;
- BOOL query_result = VerQueryValue(data, L"\\VarFileInfo\\Translation",
- (void**) &translate, &page_count);
+ const LanguageAndCodePage* translate = GetTranslate(data.data());
+ if (!translate)
+ return nullptr;
- if (query_result && translate) {
- return new FileVersionInfoWin(data, translate->language,
- translate->code_page);
-
- } else {
- free(data);
- return NULL;
- }
+ return new FileVersionInfoWin(std::move(data), translate->language,
+ translate->code_page);
}
base::string16 FileVersionInfoWin::company_name() {
@@ -175,7 +179,7 @@
L"\\StringFileInfo\\%04x%04x\\%ls", language, code_page, name);
LPVOID value = NULL;
uint32_t size;
- BOOL r = ::VerQueryValue(data_.get(), sub_block, &value, &size);
+ BOOL r = ::VerQueryValue(data_, sub_block, &value, &size);
if (r && value) {
value_str->assign(static_cast<wchar_t*>(value));
return true;
@@ -191,3 +195,24 @@
else
return L"";
}
+
+FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t>&& data,
+ WORD language,
+ WORD code_page)
+ : owned_data_(std::move(data)),
+ data_(owned_data_.data()),
+ language_(language),
+ code_page_(code_page),
+ fixed_file_info_(GetVsFixedFileInfo(data_)) {
+ DCHECK(!owned_data_.empty());
+}
+
+FileVersionInfoWin::FileVersionInfoWin(void* data,
+ WORD language,
+ WORD code_page)
+ : data_(data),
+ language_(language),
+ code_page_(code_page),
+ fixed_file_info_(GetVsFixedFileInfo(data)) {
+ DCHECK(data_);
+}
diff --git a/base/file_version_info_win.h b/base/file_version_info_win.h
index 1e152a8..d91b67f 100644
--- a/base/file_version_info_win.h
+++ b/base/file_version_info_win.h
@@ -5,20 +5,23 @@
#ifndef BASE_FILE_VERSION_INFO_WIN_H_
#define BASE_FILE_VERSION_INFO_WIN_H_
+#include <windows.h>
+
+#include <stdint.h>
+
#include <memory>
#include <string>
+#include <vector>
#include "base/base_export.h"
#include "base/file_version_info.h"
#include "base/macros.h"
-#include "base/memory/free_deleter.h"
struct tagVS_FIXEDFILEINFO;
typedef tagVS_FIXEDFILEINFO VS_FIXEDFILEINFO;
class BASE_EXPORT FileVersionInfoWin : public FileVersionInfo {
public:
- FileVersionInfoWin(void* data, WORD language, WORD code_page);
~FileVersionInfoWin() override;
// Accessors to the different version properties.
@@ -48,14 +51,25 @@
std::wstring GetStringValue(const wchar_t* name);
// Get the fixed file info if it exists. Otherwise NULL
- VS_FIXEDFILEINFO* fixed_file_info() { return fixed_file_info_; }
+ const VS_FIXEDFILEINFO* fixed_file_info() const { return fixed_file_info_; }
private:
- std::unique_ptr<char, base::FreeDeleter> data_;
- WORD language_;
- WORD code_page_;
- // This is a pointer into the data_ if it exists. Otherwise NULL.
- VS_FIXEDFILEINFO* fixed_file_info_;
+ friend FileVersionInfo;
+
+ // |data| is a VS_VERSION_INFO resource. |language| and |code_page| are
+ // extracted from the \VarFileInfo\Translation value of |data|.
+ FileVersionInfoWin(std::vector<uint8_t>&& data,
+ WORD language,
+ WORD code_page);
+ FileVersionInfoWin(void* data, WORD language, WORD code_page);
+
+ const std::vector<uint8_t> owned_data_;
+ const void* const data_;
+ const WORD language_;
+ const WORD code_page_;
+
+ // This is a pointer into |data_| if it exists. Otherwise nullptr.
+ const VS_FIXEDFILEINFO* const fixed_file_info_;
DISALLOW_COPY_AND_ASSIGN(FileVersionInfoWin);
};
diff --git a/base/file_version_info_win_unittest.cc b/base/file_version_info_win_unittest.cc
new file mode 100644
index 0000000..b5788fe
--- /dev/null
+++ b/base/file_version_info_win_unittest.cc
@@ -0,0 +1,175 @@
+// 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.
+
+#include "base/file_version_info_win.h"
+
+#include <windows.h>
+
+#include <stddef.h>
+
+#include <memory>
+
+#include "base/file_version_info.h"
+#include "base/files/file_path.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/path_service.h"
+#include "base/scoped_native_library.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::FilePath;
+
+namespace {
+
+FilePath GetTestDataPath() {
+ FilePath path;
+ PathService::Get(base::DIR_SOURCE_ROOT, &path);
+ path = path.AppendASCII("base");
+ path = path.AppendASCII("test");
+ path = path.AppendASCII("data");
+ path = path.AppendASCII("file_version_info_unittest");
+ return path;
+}
+
+class FileVersionInfoFactory {
+ public:
+ explicit FileVersionInfoFactory(const FilePath& path) : path_(path) {}
+
+ std::unique_ptr<FileVersionInfo> Create() const {
+ return base::WrapUnique(FileVersionInfo::CreateFileVersionInfo(path_));
+ }
+
+ private:
+ const FilePath path_;
+
+ DISALLOW_COPY_AND_ASSIGN(FileVersionInfoFactory);
+};
+
+class FileVersionInfoForModuleFactory {
+ public:
+ explicit FileVersionInfoForModuleFactory(const FilePath& path)
+ // Load the library with LOAD_LIBRARY_AS_IMAGE_RESOURCE since it shouldn't
+ // be executed.
+ : library_(::LoadLibraryEx(path.value().c_str(),
+ nullptr,
+ LOAD_LIBRARY_AS_IMAGE_RESOURCE)) {
+ EXPECT_TRUE(library_.is_valid());
+ }
+
+ std::unique_ptr<FileVersionInfo> Create() const {
+ return base::WrapUnique(
+ FileVersionInfo::CreateFileVersionInfoForModule(library_.get()));
+ }
+
+ private:
+ const base::ScopedNativeLibrary library_;
+
+ DISALLOW_COPY_AND_ASSIGN(FileVersionInfoForModuleFactory);
+};
+
+template <typename T>
+class FileVersionInfoTest : public testing::Test {};
+
+using FileVersionInfoFactories =
+ ::testing::Types<FileVersionInfoFactory, FileVersionInfoForModuleFactory>;
+
+} // namespace
+
+TYPED_TEST_CASE(FileVersionInfoTest, FileVersionInfoFactories);
+
+TYPED_TEST(FileVersionInfoTest, HardCodedProperties) {
+ const wchar_t kDLLName[] = {L"FileVersionInfoTest1.dll"};
+
+ const wchar_t* const kExpectedValues[15] = {
+ // FileVersionInfoTest.dll
+ L"Goooooogle", // company_name
+ L"Google", // company_short_name
+ L"This is the product name", // product_name
+ L"This is the product short name", // product_short_name
+ L"The Internal Name", // internal_name
+ L"4.3.2.1", // product_version
+ L"Private build property", // private_build
+ L"Special build property", // special_build
+ L"This is a particularly interesting comment", // comments
+ L"This is the original filename", // original_filename
+ L"This is my file description", // file_description
+ L"1.2.3.4", // file_version
+ L"This is the legal copyright", // legal_copyright
+ L"This is the legal trademarks", // legal_trademarks
+ L"This is the last change", // last_change
+ };
+
+ FilePath dll_path = GetTestDataPath();
+ dll_path = dll_path.Append(kDLLName);
+
+ TypeParam factory(dll_path);
+ std::unique_ptr<FileVersionInfo> version_info(factory.Create());
+ ASSERT_TRUE(version_info);
+
+ int j = 0;
+ EXPECT_EQ(kExpectedValues[j++], version_info->company_name());
+ EXPECT_EQ(kExpectedValues[j++], version_info->company_short_name());
+ EXPECT_EQ(kExpectedValues[j++], version_info->product_name());
+ EXPECT_EQ(kExpectedValues[j++], version_info->product_short_name());
+ EXPECT_EQ(kExpectedValues[j++], version_info->internal_name());
+ EXPECT_EQ(kExpectedValues[j++], version_info->product_version());
+ EXPECT_EQ(kExpectedValues[j++], version_info->private_build());
+ EXPECT_EQ(kExpectedValues[j++], version_info->special_build());
+ EXPECT_EQ(kExpectedValues[j++], version_info->comments());
+ EXPECT_EQ(kExpectedValues[j++], version_info->original_filename());
+ EXPECT_EQ(kExpectedValues[j++], version_info->file_description());
+ EXPECT_EQ(kExpectedValues[j++], version_info->file_version());
+ EXPECT_EQ(kExpectedValues[j++], version_info->legal_copyright());
+ EXPECT_EQ(kExpectedValues[j++], version_info->legal_trademarks());
+ EXPECT_EQ(kExpectedValues[j++], version_info->last_change());
+}
+
+TYPED_TEST(FileVersionInfoTest, IsOfficialBuild) {
+ constexpr struct {
+ const wchar_t* const dll_name;
+ const bool is_official_build;
+ } kTestItems[]{
+ {L"FileVersionInfoTest1.dll", true}, {L"FileVersionInfoTest2.dll", false},
+ };
+
+ for (const auto& test_item : kTestItems) {
+ const FilePath dll_path = GetTestDataPath().Append(test_item.dll_name);
+
+ TypeParam factory(dll_path);
+ std::unique_ptr<FileVersionInfo> version_info(factory.Create());
+ ASSERT_TRUE(version_info);
+
+ EXPECT_EQ(test_item.is_official_build, version_info->is_official_build());
+ }
+}
+
+TYPED_TEST(FileVersionInfoTest, CustomProperties) {
+ FilePath dll_path = GetTestDataPath();
+ dll_path = dll_path.AppendASCII("FileVersionInfoTest1.dll");
+
+ TypeParam factory(dll_path);
+ std::unique_ptr<FileVersionInfo> version_info(factory.Create());
+ ASSERT_TRUE(version_info);
+
+ // Test few existing properties.
+ std::wstring str;
+ FileVersionInfoWin* version_info_win =
+ static_cast<FileVersionInfoWin*>(version_info.get());
+ EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 1", &str));
+ EXPECT_EQ(L"Un", str);
+ EXPECT_EQ(L"Un", version_info_win->GetStringValue(L"Custom prop 1"));
+
+ EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 2", &str));
+ EXPECT_EQ(L"Deux", str);
+ EXPECT_EQ(L"Deux", version_info_win->GetStringValue(L"Custom prop 2"));
+
+ EXPECT_TRUE(version_info_win->GetValue(L"Custom prop 3", &str));
+ EXPECT_EQ(L"1600 Amphitheatre Parkway Mountain View, CA 94043", str);
+ EXPECT_EQ(L"1600 Amphitheatre Parkway Mountain View, CA 94043",
+ version_info_win->GetStringValue(L"Custom prop 3"));
+
+ // Test an non-existing property.
+ EXPECT_FALSE(version_info_win->GetValue(L"Unknown property", &str));
+ EXPECT_EQ(L"", version_info_win->GetStringValue(L"Unknown property"));
+}