Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows

base/native_library_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/native_library.h"
 
 #include <windows.h>
 
 #include "base/files/file_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 
 namespace base {
 
+typedef HMODULE(WINAPI* AddDllDirectory)(PCWSTR NewDirectory);
 typedef HMODULE (WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
+typedef HMODULE(WINAPI* LoadLibraryFunctionEx)(const wchar_t* file_name,
+                                               HANDLE hFile,
+                                               DWORD dwFlags);

[xhwang, 2017/03/13 20:47:09]

nit: line 19 has a space after HMODULE. Is the new format recommended by
clang-format? If so, fix 19 as well?

[chengx, 2017/03/13 22:20:22]

Sorry, it is typo. Now it is fixed.

 
 namespace {
-
+// This helper method uses LoadLibrary() WinAPI.
 NativeLibrary LoadNativeLibraryHelper(const FilePath& library_path,
                                       LoadLibraryFunction load_library_api,
                                       NativeLibraryLoadError* error) {
   // LoadLibrary() opens the file off disk.
   ThreadRestrictions::AssertIOAllowed();
 
   // Switch the current directory to the library directory as the library
   // may have dependencies on DLLs in this directory.
   bool restore_directory = false;
   FilePath current_directory;
@@ skipping 10 matching lines @@
     // GetLastError() needs to be called immediately after |load_library_api|.
     error->code = GetLastError();
   }
 
   if (restore_directory)
     SetCurrentDirectory(current_directory);
 
   return module;
 }
 
+// This helper method uses LoadLibraryEx() WinAPI.
+NativeLibrary LoadNativeLibraryHelperEx(const FilePath& library_path,
+                                        LoadLibraryFunctionEx load_library_api,
+                                        NativeLibraryLoadError* error) {
+  // LoadLibraryEx() opens the file off disk.
+  ThreadRestrictions::AssertIOAllowed();
+
+  HMODULE module = (*load_library_api)(
+      library_path.value().c_str(), NULL,

[xhwang, 2017/03/13 20:47:08]

here and everywhere, s/NULL/nullptr?

[chengx, 2017/03/13 22:20:22]

Done.

+      LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_DEFAULT_DIRS);

[xhwang, 2017/03/13 20:47:09]

nit: Add a comment why we need LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR, similar to the
comments on line 32-33.

[chengx, 2017/03/13 22:20:22]

Done.

+  if (!module && error) {
+    // GetLastError() needs to be called immediately after |load_library_api|.
+    error->code = GetLastError();
+  }
+
+  return module;
+}
+
 }  // namespace
 
 std::string NativeLibraryLoadError::ToString() const {
   return StringPrintf("%u", code);
 }
 
 // static
 NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path,

[xhwang, 2017/03/13 20:47:09]

I think base::LoadNativeLIbrary() is calling this one instead of
LoadNativeLibraryDynamically():
https://cs.chromium.org/chromium/src/base/native_library.cc?rcl=cd1a0de817786...

Actually, I have not seen any usage of LoadNativeLibraryDynamically based on
code search. You can look at the history to see whether we can just remove it.

[chengx, 2017/03/13 22:20:22]

I agree with you. I am not seeing LoadNativeLibraryDynamically referenced
anywhere. Here is thing. I am not sure why initially we created these two
methods. Maybe LoadNativeLibraryWithOptions is for static call, while
LoadNativeLibraryDynamically is for dynamic call. We do have LoadLibraryExW
defined in
https://cs.chromium.org/chromium/src/third_party/perl/c/i686-w64-mingw32/incl...

However, the additional flags are not defined there. So we cannot statically
call LoadLibraryExW. 

In short, remove LoadNativeLibraryDynamically and pack everything into
LoadNativeLibraryWithOptions to make it a dynamic call? How do you think plz?

[xhwang, 2017/03/14 00:14:31]

LoadNativeLibraryDynamically() was added long time ago and I don't know why it's
added.

LoadLibraryExW requires XP or higher, so I suppose we can also use it statically
given that <windows.h> is included? That'll save us the extra call to
GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryExW")).

[chengx, 2017/03/14 02:44:40]

Agreed. LoadLibrayExW and LoadLibraryW can be called safely. We just need to be
care of the flags.

                                            const NativeLibraryOptions& options,
                                            NativeLibraryLoadError* error) {
   return LoadNativeLibraryHelper(library_path, LoadLibraryW, error);
 }
 
 NativeLibrary LoadNativeLibraryDynamically(const FilePath& library_path) {
-  typedef HMODULE (WINAPI* LoadLibraryFunction)(const wchar_t* file_name);
+  HMODULE load_library_module = NULL;
 
-  LoadLibraryFunction load_library = reinterpret_cast<LoadLibraryFunction>(
-      GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW"));
+  enum LoadLibraryResult {

[xhwang, 2017/03/13 20:47:09]

nit: enum class?

[xhwang, 2017/03/13 20:47:09]

Add a comment that combinations of these are used to report UMA, so any update
should involve histogram update.

[chengx, 2017/03/13 22:20:22]

Both should work. Using enum directly rather than enum class avoid applying
static_cast load_library_status to int in UMA_HISTOGRAM_ENUMERATION call, IMHO. 

It seems enum and enum class are both heavily used in the code base.

[chengx, 2017/03/13 22:20:22]

The enum is updated, so maybe this comment is no longer needed?

[xhwang, 2017/03/14 00:14:31]

We do recommend enum class in new code.

How about typed enum class like this?

enum class Foo : uint32_t { ... }

[chengx, 2017/03/14 02:44:40]

I understand enum class if preferred over enum. However, if I use "enum class
Foo" or even "enum class Foo : uint32_t", it doesn't allow me to static_cast to
uint32_t, which is needed by UMA_HISTOGRAM_ENUMERATION. It may be a compiler
issue that I have not figured out yet. I do have a search of Chromium code base,
and seems that all (I have not found a counter example yet) the enums that are
used for UMA_HISTOGRAM_ENUMERATION are simply "enum" rather than "enum class".

+    LoadLibExW_EXIST = 0,
+    LoadLibExW_NONE = 1 << 0,
+    LoadLibExW_FAIL = 1 << 1,
+    LoadLibW_FAIL = 1 << 2,

[xhwang, 2017/03/13 20:47:09]

nit: You are using 3 bit positions (4 enum values) to represent 5 possible
states, which seems to be a bit overkill and confusing. Also, it's not obvious
how to map the valid combinations into the entries in LoadLibraryResultCategory
in the histogram.xml.

Can we just have 5 enums here (same as what you have in
LoadLibraryResultCategory), and report them directly in the impl?

[chengx, 2017/03/13 22:20:22]

Agreed and fixed. I was addicted to playing with the bits thing too much.

+    // This value is beyond the sum of all bit fields above and
+    // should remain last (shifted by one more than the last value)
+    END = 1 << 3
+  };
 
-  return LoadNativeLibraryHelper(library_path, load_library, NULL);
+  // This variable records the library loading result.
+  uint32_t load_library_status = LoadLibraryResult::LoadLibExW_EXIST;
+
+  // The LOAD_LIBRARY_SEARCH_* flags are available on systems that have
+  // KB2533623 installed. To determine whether the flags are available, use
+  // GetProcAddress to get the address of the AddDllDirectory,
+  // RemoveDllDirectory, or SetDefaultDllDirectories function. If GetProcAddress
+  // succeeds, the LOAD_LIBRARY_SEARCH_* flags can be used with LoadLibraryEx.
+  // https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx

[xhwang, 2017/03/13 20:47:09]

The link seems to be broken for me...

[chengx, 2017/03/13 22:20:22]

Did you copy the "aspx" in the following line?

[xhwang, 2017/03/14 00:14:31]

I did and I checked... but now I tried it again and it's working. Sorry about
the noise.

[chengx, 2017/03/14 02:44:40]

Cool!

+  // The LOAD_LIBRARY_SEARCH_* flags are used in LoadNativeLibraryHelperEx
+  // method above.
+  auto add_dll_dir_func = reinterpret_cast<AddDllDirectory>(
+      GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory"));

[xhwang, 2017/03/13 20:47:08]

nit: Can we move this into a helper function that returns a boolean? That'll
help keep this function clean.

[chengx, 2017/03/13 22:20:22]

Now since enum is updated, I need this add_dll_dir_func variable to help assign
value to load_library_status. If moving this to a helper function which returns
a boolean, this method needs to be called twice though.

[xhwang, 2017/03/14 00:14:31]

You can always store !!add_dll_dir_func into a boolean?

[chengx, 2017/03/14 02:44:40]

accepted.

+
+  if (add_dll_dir_func) {
+    LoadLibraryFunctionEx load_library =
+        reinterpret_cast<LoadLibraryFunctionEx>(
+            GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryExW"));

[xhwang, 2017/03/13 20:47:09]

nit for discussion: I don't know what's the value of doing this here instead of
doing it in LoadNativeLibraryHelper, which seems more natural to me.

[chengx, 2017/03/13 22:20:22]

If removing LoadNativeLibraryDynamically and then packing everything into
LoadNativeLibraryWithOptions, I think doing this in LoadNativeLibraryHelper is
obviously better. However, it seems that LoadNativeLibraryWithOptions wants to
use LoadLibraryW () via the import table directly, so I was hesitated to modify
LoadNativeLibraryHelper whose 2nd parameter is the function pointer. Again, this
goes back to the question if we want to make LoadNativeLibraryWithOptions call
APIs dynamically.

+    load_library_module =
+        LoadNativeLibraryHelperEx(library_path, load_library, NULL);

[xhwang, 2017/03/13 20:47:09]

See above that we need to update LoadNativeLibraryWithOptions, where the |error|
will be non-null. That's actually how I found that the error code is 126.

[chengx, 2017/03/13 22:20:22]

Acknowledged.

[chengx, 2017/03/14 02:44:40]

Alright, LoadNativeLibraryWithOptions updated.

+    if (!load_library_module)
+      load_library_status |= LoadLibraryResult::LoadLibExW_FAIL;
+  } else {
+    load_library_status |= LoadLibraryResult::LoadLibExW_NONE;
+  }
+
+  // If LoadLibraryExW API call fails, try LoadLibraryW API.
+  if (!load_library_module) {
+    LoadLibraryFunction load_library = reinterpret_cast<LoadLibraryFunction>(
+        GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW"));

[xhwang, 2017/03/13 20:47:08]

nit: ditto about the possibility of moving this into LoadNativeLibraryHelper.

[chengx, 2017/03/13 22:20:22]

Acknowledged.

[chengx, 2017/03/14 02:44:40]

Yep, moved into LoadNativeLibraryHelper in the new patch set.

+    load_library_module =
+        LoadNativeLibraryHelper(library_path, load_library, NULL);
+    if (!load_library_module)
+      load_library_status |= LoadLibraryResult::LoadLibW_FAIL;
+  }
+
+  UMA_HISTOGRAM_ENUMERATION("LibraryLoader.NativeLibraryDynamicallyLoad",
+                            load_library_status, LoadLibraryResult::END);
+  return load_library_module;
 }
 
 // static
 void UnloadNativeLibrary(NativeLibrary library) {
   FreeLibrary(library);
 }
 
 // static
 void* GetFunctionPointerFromNativeLibrary(NativeLibrary library,
                                           StringPiece name) {
   return GetProcAddress(library, name.data());
 }
 
 // static
 std::string GetNativeLibraryName(StringPiece name) {
   DCHECK(IsStringASCII(name));
   return name.as_string() + ".dll";
 }
 
 }  // namespace base