Don't use ::GetFileVersionInfo() in CreateFileVersionInfoForModule()

base/file_version_info_win.cc

Index: base/file_version_info_win.cc
diff --git a/base/file_version_info_win.cc b/base/file_version_info_win.cc
index 02a14db0032aa24335d52b546880162e63a3cf77..da844390ec8ec032d0407e19a1fdc98424a66ba8 100644
--- a/base/file_version_info_win.cc
+++ b/base/file_version_info_win.cc
@@ -6,48 +6,69 @@
 
 #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,
+namespace {
+
+struct LanguageAndCodePage {
+  WORD language;
+  WORD code_page;
+};
+
+// Returns the \\VarFileInfo\\Translation value extracted from the
+// VS_VERSION_INFO resource in |data|.
+LanguageAndCodePage* GetTranslate(void* data) {
+  LanguageAndCodePage* translate;
+  uint32_t page_count;
+  if (::VerQueryValue(data, L"\\VarFileInfo\\Translation", (void**)&translate,
+                      &page_count)) {
+    return translate;
+  }
+  return nullptr;
+}
+
+}  // namespace
+
+FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t> data,

[grt (UTC plus 2), 2016/06/17 20:34:59]

nit: move ctors down in the file so that the order of definitions here matches
the order of declarations in the .h file.

[fdoray, 2016/06/20 14:06:41]

Done.

                                        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);
+    : FileVersionInfoWin(data.data(), language, code_page) {

[grt (UTC plus 2), 2016/06/17 20:34:59]

, owned_data_(std::move(data)) {

[fdoray, 2016/06/20 14:06:41]

Doesn't work
error C3511: 'FileVersionInfoWin': a call to a delegating constructor shall be
the only member-initializer

Do you prefer the new version without a delegating constructor?

[grt (UTC plus 2), 2016/06/20 20:05:14]

TIL...
I don't have a strong preference.

+  owned_data_ = std::move(data);
 }
 
-FileVersionInfoWin::~FileVersionInfoWin() {
-  DCHECK(data_.get());
+FileVersionInfoWin::FileVersionInfoWin(void* data,
+                                       WORD language,
+                                       WORD code_page)
+    : data_(data), language_(language), code_page_(code_page) {
+  DCHECK(data_);
+  UINT size;
+  ::VerQueryValue(data_, L"\\", (LPVOID*)&fixed_file_info_, &size);
 }
 
-typedef struct {
-  WORD language;
-  WORD code_page;
-} LanguageAndCodePage;
+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
@@ -59,30 +80,19 @@ FileVersionInfo* FileVersionInfo::CreateFileVersionInfo(
   const wchar_t* path = file_path.value().c_str();
   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, data.size(), 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 +185,7 @@ bool FileVersionInfoWin::GetValue(const wchar_t* name,
                  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;