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

base/file_version_info_win.cc

 // 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 <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;
 
-  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
 FileVersionInfo* FileVersionInfo::CreateFileVersionInfo(
     const FilePath& file_path) {
   base::ThreadRestrictions::AssertIOAllowed();
 
   DWORD dummy;
   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() {
   return GetStringValue(L"CompanyName");
 }
 
 base::string16 FileVersionInfoWin::company_short_name() {
   return GetStringValue(L"CompanyShortName");
 }
 
@@ skipping 72 matching lines @@
 
   i = 0;
   while (i < arraysize(lang_codepage)) {
     wchar_t sub_block[MAX_PATH];
     WORD language = lang_codepage[i++];
     WORD code_page = lang_codepage[i++];
     _snwprintf_s(sub_block, MAX_PATH, MAX_PATH,
                  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;
     }
   }
   return false;
 }
 
 std::wstring FileVersionInfoWin::GetStringValue(const wchar_t* name) {
   std::wstring str;
   if (GetValue(name, &str))
     return str;
   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_);
+}