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..9613addd77fc6b409c804137e96156b09f6b01e8 100644
--- a/base/file_version_info_win.cc
+++ b/base/file_version_info_win.cc
@@ -6,48 +6,58 @@
 
 #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 {
+
+struct LanguageAndCodePage {
+  WORD language;
+  WORD code_page;
+};
+
+// Returns the \\VarFileInfo\\Translation value extracted from the
+// VS_VERSION_INFO resource in |data|.
+LanguageAndCodePage* GetTranslate(const void* data) {
+  LanguageAndCodePage* translate = nullptr;
+  UINT length;
+  ::VerQueryValue(data, L"\\VarFileInfo\\Translation", (void**)&translate,

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

does msdn state that lplpBuffer won't be modified on error? if not, please stick
with the conservative approach.

[fdoray, 2016/06/21 19:31:11]

Done.

+                  &length);
+  return translate;
 }
 
-FileVersionInfoWin::~FileVersionInfoWin() {
-  DCHECK(data_.get());
+VS_FIXEDFILEINFO* GetVsFixedFileInfo(const void* data) {
+  VS_FIXEDFILEINFO* fixed_file_info = nullptr;
+  UINT length;
+  ::VerQueryValue(data, L"\\", (LPVOID*)&fixed_file_info, &length);
+  return fixed_file_info;
 }
 
-typedef struct {
-  WORD language;
-  WORD code_page;
-} LanguageAndCodePage;
+}  // 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;
+
+  const LanguageAndCodePage* translate = GetTranslate(data);
+  if (!translate)
+    return nullptr;
 
-  FilePath app_path(system_buffer);
-  return CreateFileVersionInfo(app_path);
+  return new FileVersionInfoWin(data, translate->language,
+                                translate->code_page);
 }
 
 // static
@@ -59,30 +69,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;
-  }
-
-  LanguageAndCodePage* translate = NULL;
-  uint32_t page_count;
-  BOOL query_result = VerQueryValue(data, L"\\VarFileInfo\\Translation",
-                                   (void**) &translate, &page_count);
+  if (!::GetFileVersionInfo(path, dummy, data.size(), data.data()))
+    return nullptr;
 
-  if (query_result && translate) {
-    return new FileVersionInfoWin(data, translate->language,
-                                  translate->code_page);
+  const LanguageAndCodePage* translate = GetTranslate(data.data());
+  if (!translate)
+    return nullptr;
 
-  } else {
-    free(data);
-    return NULL;
-  }
+  return new FileVersionInfoWin(std::move(data), translate->language,
+                                translate->code_page);
 }
 
 base::string16 FileVersionInfoWin::company_name() {
@@ -175,7 +174,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;
@@ -191,3 +190,24 @@ std::wstring FileVersionInfoWin::GetStringValue(const wchar_t* name) {
   else
     return L"";
 }
+
+FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t> data,
+                                       WORD language,
+                                       WORD code_page)
+    : data_(data.data()),
+      owned_data_(std::move(data)),

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

while i *think* that this works, i can't convince myself by reading the spec
that the value returned by data.data() is guaranteed to be valid after the move.
please swap these to so that the code doesn't depend on this:
    : owned_data_(std::move(data)),
      data_(owned_data_.data()),
(swap them in the class definition as well)

[fdoray, 2016/06/21 19:31:11]

Done.

+      language_(language),
+      code_page_(code_page),
+      fixed_file_info_(GetVsFixedFileInfo(data_)) {
+  DCHECK(data_);
+}
+
+FileVersionInfoWin::FileVersionInfoWin(void* data,
+                                       WORD language,
+                                       WORD code_page)
+    : data_(data),
+      language_(language),
+      code_page_(code_page),
+      fixed_file_info_(GetVsFixedFileInfo(data)) {
+  DCHECK(data_);
+}