Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744043003:
Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows (Closed)
DescriptionUse LoadLibraryExW if applicable in LoadNativeLibrary on Windows
In the current LoadNativeLibrary implementation, LoadLibraryW Windows
API is used to load a DLL. To be able to find dependencies in the same
folder, SetCurrentDirectory() is needed to search for the DLL directory,
and sets it back after the DLL is loaded. This is required because on
Windows, it'll search for dependencies in a search list, which includes
the system "current directory", but not the DLL directory.
However, SetCurrentDirectory() can be potentially problematic. It is not
recommended in a multithreaded application, and could pose a security
risk as "If an attacker gains control of one of the directories on the
DLL search path, it can place a malicious copy of the DLL in that
directory. This is sometimes called a DLL preloading attack or a binary
planting attack."
The right thing to do is to use LoadLibraryExW, where we can specify
additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable
searching in the DLL directory. With this, we can eliminate the need of
doing SetCurrentDirectory().
Using these additional flags requires KB2533623 to be installed and the
method is "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."
Therefore, we can dynamically call LoadLibraryExW if the API and the
flags are available. If not or its call fails, we should use the
LoadLibraryW API.
This CL also adds UMA histogram to record the calling status of both
LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the
LoadNativeLibraryDynamically method as it is not used anywhere.
Running Chromium built with this CL locally shows that LoadLibraryExW
call were successful for kernel32.dll and widevinecdm.dll (which caused
crbug.com/700208), but failed when loading MDMRegistration.dll.
LoadLibraryW succeeds in loading MDMRegistration.dll though.
BUG=700503, 700208
Review-Url: https://codereview.chromium.org/2744043003
Cr-Commit-Position: refs/heads/master@{#457359}
Committed: https://chromium.googlesource.com/chromium/src/+/5946c9233dc393f2e3e275323e66659fe5fb35cd
Patch Set 1 #Patch Set 2 : Add UMA_histogram #
Total comments: 34
Patch Set 3 : Address comments. #
Total comments: 6
Patch Set 4 : Remove LoadNativeLibraryDynamically, update LoadNativeLibraryWithOptions, address comments. #Patch Set 5 : Add a wrapper method for UMA. #
Total comments: 22
Patch Set 6 : Refactor code, address comments. #
Total comments: 4
Patch Set 7 : Better variable names, fix nits. #Patch Set 8 : Add TODO for removing fallback. #
Total comments: 2
Patch Set 9 : Better variable name. #
Messages
Total messages: 104 (80 generated)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #2 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use LoadLibraryExW to replace LoadLibraryW BUG=700503 ========== to ========== Use LoadLibraryExW in LoadNativeLibrary on Windows BUG=700503 ==========
Description was changed from ========== Use LoadLibraryExW in LoadNativeLibrary on Windows BUG=700503 ========== to ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). These additional flags KB2533623 to be 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. Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ==========
Description was changed from ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). These additional flags KB2533623 to be 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. Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ========== to ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). These additional flags KB2533623 to be 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. Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ==========
Description was changed from ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). These additional flags KB2533623 to be 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. Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ========== to ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be 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. Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ==========
Description was changed from ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be 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. Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ========== to ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ==========
Description was changed from ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ========== to ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ==========
Description was changed from ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. BUG=700503 ========== to ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. BUG=700503 ==========
Description was changed from ========== Use LoadLibraryExW in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. BUG=700503 ========== to ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. BUG=700503 ==========
chengx@chromium.org changed reviewers: + xhwang@chromium.org
PTAL~
Thank you sooo much for working on this! To test this manually, you can check the example in https://bugs.chromium.org/p/chromium/issues/detail?id=700208 where you can use Process Monitor to check the DLL dependency search order. I have a few comments on the implementation, mostly nits. Thank you again! https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:22: DWORD dwFlags); nit: line 19 has a space after HMODULE. Is the new format recommended by clang-format? If so, fix 19 as well? https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:64: library_path.value().c_str(), NULL, here and everywhere, s/NULL/nullptr? https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:65: LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_DEFAULT_DIRS); nit: Add a comment why we need LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR, similar to the comments on line 32-33. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:81: NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path, 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. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:90: enum LoadLibraryResult { nit: enum class? https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:90: enum LoadLibraryResult { Add a comment that combinations of these are used to report UMA, so any update should involve histogram update. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:94: LoadLibW_FAIL = 1 << 2, 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? https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:108: // https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx The link seems to be broken for me... https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:112: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory")); nit: Can we move this into a helper function that returns a boolean? That'll help keep this function clean. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:117: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryExW")); 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. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:119: LoadNativeLibraryHelperEx(library_path, load_library, NULL); 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. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:129: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW")); nit: ditto about the possibility of moving this into LoadNativeLibraryHelper.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have addressed all the comments except that if we want to remove LoadNativeLibraryDynamically() and pack everything into LoadNativeLibraryWithOptions(). I think this is the correct thing to do but still have some concern. Please see my inline replies to your comments below. PTAL~ > To test this manually, you can check the example in > https://bugs.chromium.org/p/chromium/issues/detail? > id=700208 where you can use > Process Monitor to check the DLL dependency search order. Yep, I will do the check. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:22: DWORD dwFlags); On 2017/03/13 20:47:09, xhwang_slow wrote: > nit: line 19 has a space after HMODULE. Is the new format recommended by > clang-format? If so, fix 19 as well? Sorry, it is typo. Now it is fixed. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:64: library_path.value().c_str(), NULL, On 2017/03/13 20:47:08, xhwang_slow wrote: > here and everywhere, s/NULL/nullptr? Done. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:65: LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_DEFAULT_DIRS); On 2017/03/13 20:47:09, xhwang_slow wrote: > nit: Add a comment why we need LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR, similar to the > comments on line 32-33. > Done. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:81: NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path, On 2017/03/13 20:47:09, xhwang_slow wrote: > 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. 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? https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:90: enum LoadLibraryResult { On 2017/03/13 20:47:09, xhwang_slow wrote: > Add a comment that combinations of these are used to report UMA, so any update > should involve histogram update. The enum is updated, so maybe this comment is no longer needed? https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:90: enum LoadLibraryResult { On 2017/03/13 20:47:09, xhwang_slow wrote: > nit: enum class? 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. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:94: LoadLibW_FAIL = 1 << 2, On 2017/03/13 20:47:09, xhwang_slow wrote: > 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? Agreed and fixed. I was addicted to playing with the bits thing too much. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:108: // https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx On 2017/03/13 20:47:09, xhwang_slow wrote: > The link seems to be broken for me... Did you copy the "aspx" in the following line? https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:112: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory")); On 2017/03/13 20:47:08, xhwang_slow wrote: > nit: Can we move this into a helper function that returns a boolean? That'll > help keep this function clean. 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. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:117: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryExW")); On 2017/03/13 20:47:09, xhwang_slow wrote: > 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. 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. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:119: LoadNativeLibraryHelperEx(library_path, load_library, NULL); On 2017/03/13 20:47:09, xhwang_slow wrote: > 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. Acknowledged. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:129: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW")); On 2017/03/13 20:47:08, xhwang_slow wrote: > nit: ditto about the possibility of moving this into LoadNativeLibraryHelper. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Thanks! I have a few more comments. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:81: NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path, On 2017/03/13 22:20:22, chengx wrote: > On 2017/03/13 20:47:09, xhwang_slow wrote: > > 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. > > 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? 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")). https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:90: enum LoadLibraryResult { On 2017/03/13 22:20:22, chengx wrote: > On 2017/03/13 20:47:09, xhwang_slow wrote: > > nit: enum class? > > 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. We do recommend enum class in new code. How about typed enum class like this? enum class Foo : uint32_t { ... } https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:108: // https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx On 2017/03/13 22:20:22, chengx wrote: > On 2017/03/13 20:47:09, xhwang_slow wrote: > > The link seems to be broken for me... > > Did you copy the "aspx" in the following line? I did and I checked... but now I tried it again and it's working. Sorry about the noise. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:112: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory")); On 2017/03/13 22:20:22, chengx wrote: > On 2017/03/13 20:47:08, xhwang_slow wrote: > > nit: Can we move this into a helper function that returns a boolean? That'll > > help keep this function clean. > > 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. You can always store !!add_dll_dir_func into a boolean? https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... base/native_library_win.cc:108: END nit: We use UPPER_CASE style for enums https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Also, the names seem a bit verbose to me. Assuming you'll explain what these are in histograms.xml, how about something like: SUCCEED, FAIL_AND_SUCCEED, FAIL_AND_FAIL, UNAVAILABLE_AND_SUCCEED, UNAVAILABLE_AND_FAIL, https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... base/native_library_win.cc:130: LoadNativeLibraryHelperEx(library_path, load_library, nullptr); I guess we are mixing DLL load failure with LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR not supported failure. But we really are only interested in LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR failure, not the former... If LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR is not supported, what kind of error code will we get? https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... base/native_library_win.cc:131: } nit: We (media) like to return early... So here you can do something like: if (load_library_module) { ReportResultToUMA(...); return load_library_module; } So the code below doesn't need to be a big if block... Feel free to ignore if base/ or your team has different style.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:140001) has been deleted
PTAL~ https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:81: NativeLibrary LoadNativeLibraryWithOptions(const FilePath& library_path, On 2017/03/14 00:14:31, xhwang_slow wrote: > On 2017/03/13 22:20:22, chengx wrote: > > On 2017/03/13 20:47:09, xhwang_slow wrote: > > > 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. > > > > 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? > > 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")). Agreed. LoadLibrayExW and LoadLibraryW can be called safely. We just need to be care of the flags. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:90: enum LoadLibraryResult { On 2017/03/14 00:14:31, xhwang_slow wrote: > On 2017/03/13 22:20:22, chengx wrote: > > On 2017/03/13 20:47:09, xhwang_slow wrote: > > > nit: enum class? > > > > 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. > > We do recommend enum class in new code. > > How about typed enum class like this? > > enum class Foo : uint32_t { ... } 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". https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:108: // https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx On 2017/03/14 00:14:31, xhwang_slow wrote: > On 2017/03/13 22:20:22, chengx wrote: > > On 2017/03/13 20:47:09, xhwang_slow wrote: > > > The link seems to be broken for me... > > > > Did you copy the "aspx" in the following line? > > I did and I checked... but now I tried it again and it's working. Sorry about > the noise. Cool! https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:112: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "AddDllDirectory")); On 2017/03/14 00:14:31, xhwang_slow wrote: > On 2017/03/13 22:20:22, chengx wrote: > > On 2017/03/13 20:47:08, xhwang_slow wrote: > > > nit: Can we move this into a helper function that returns a boolean? That'll > > > help keep this function clean. > > > > 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. > > You can always store !!add_dll_dir_func into a boolean? accepted. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:119: LoadNativeLibraryHelperEx(library_path, load_library, NULL); On 2017/03/13 22:20:22, chengx wrote: > On 2017/03/13 20:47:09, xhwang_slow wrote: > > 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. > > Acknowledged. Alright, LoadNativeLibraryWithOptions updated. https://codereview.chromium.org/2744043003/diff/100001/base/native_library_wi... base/native_library_win.cc:129: GetProcAddress(GetModuleHandle(L"kernel32.dll"), "LoadLibraryW")); On 2017/03/13 22:20:22, chengx wrote: > On 2017/03/13 20:47:08, xhwang_slow wrote: > > nit: ditto about the possibility of moving this into LoadNativeLibraryHelper. > > Acknowledged. Yep, moved into LoadNativeLibraryHelper in the new patch set. https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... base/native_library_win.cc:108: END On 2017/03/14 00:14:32, xhwang_slow wrote: > nit: We use UPPER_CASE style for enums > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Also, the names seem a bit verbose to me. Assuming you'll explain what these are > in histograms.xml, how about something like: > > SUCCEED, > FAIL_AND_SUCCEED, > FAIL_AND_FAIL, > UNAVAILABLE_AND_SUCCEED, > UNAVAILABLE_AND_FAIL, Done. https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... base/native_library_win.cc:130: LoadNativeLibraryHelperEx(library_path, load_library, nullptr); On 2017/03/14 00:14:32, xhwang_slow wrote: > I guess we are mixing DLL load failure with LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR not > supported failure. But we really are only interested in > LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR failure, not the former... > > If LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR is not supported, what kind of error code > will we get? I think you are right. We have LoadLibraryW and LoadLibraryExW defined in the Winbase.h already, so there is no need to GetProcAddress for them. So there will be no DLL load failures now. https://codereview.chromium.org/2744043003/diff/120001/base/native_library_wi... base/native_library_win.cc:131: } On 2017/03/14 00:14:32, xhwang_slow wrote: > nit: We (media) like to return early... > > So here you can do something like: > > if (load_library_module) { > ReportResultToUMA(...); > return load_library_module; > } > > So the code below doesn't need to be a big if block... > > Feel free to ignore if base/ or your team has different style. Return early SGTM.
Description was changed from ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not, we should get back to the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. BUG=700503 ========== to ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes LoadNativeLibraryDynamically method as it is not used anywhere. BUG=700503 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes LoadNativeLibraryDynamically method as it is not used anywhere. BUG=700503 ========== to ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. BUG=700503 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
chengx@chromium.org changed reviewers: + dcheng@chromium.org, isherman@chromium.org
PTAL~ isherman@ owner for tools/metrics/histograms/histograms.xml dcheng@ owner for //base. Thank you! P.S. Linux build bots which are irrelevant to this CL have some issues as shown in waterfall currently. All build bots should pass once those issues are gone.
Metrics LGTM % nits: https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:21: enum LoadLibraryResult { nit: Please document that this is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24522: + results, which are used in LoadNativeLibraryDynamically method in nit: s/used in/used in https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:98792: + <int value="0" label="LoadLibraryExW Succeed"/> nit: either "Succeeded" or "Success"
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. BUG=700503 ========== to ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. BUG=700503,700208 ==========
looking good! I only have a few nits. Thanks! https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:41: void LogLibrarayLoadResultToUMA(LoadLibraryResult& result) { We pretty much never pass non-const ref in Chromium. Just pass by value? https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:48: bool IsLoadLibSearchFlagsAvailable() { nit: s/Is/Are. Also since this is related to LOAD_LIBRARY_SEARCH flags. So to be exact, probably name it to AreLoadLibrarySearchFlagsAvailable() or simply AreSearchFlagsAvailable() since this whole file is in the context of "LoadLibrary". https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:56: // method above. "above" is not accurate now https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:67: HMODULE load_library_module = nullptr; nit: ISTM the old name |module| is concise and clear, and |load_library| here doesn't add much value. https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:92: // LoadLibraryW API. nit: add an empty line here since these two lines are for all the code left in this function. https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:120: } else if (load_library_module) nit: use curly here and below as well: https://google.github.io/styleguide/cppguide.html#Conditionals // Not allowed - curly on IF but not ELSE if (condition) { foo; } else bar; https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:123: load_library_status = LoadLibraryResult::UNAVAILABLE_AND_FAIL; nit: Probably move these into a helper function as well to keep the main logic clean: LoadLibraryResult GetLoadLibraryResult(bool, bool) https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24522: + results, which are used in LoadNativeLibraryDynamically method in LoadNativeLibraryDynamically is not true now...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi xhwang@, comments all addressed in the new patch set. PTAL, thanks! https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:21: enum LoadLibraryResult { On 2017/03/14 18:42:28, Ilya Sherman wrote: > nit: Please document that this is used to back an UMA histogram, and should > therefore be treated as append-only. Done. https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:41: void LogLibrarayLoadResultToUMA(LoadLibraryResult& result) { On 2017/03/14 21:48:25, xhwang_slow wrote: > We pretty much never pass non-const ref in Chromium. Just pass by value? Yep, should pass by const ref or pass by value (accepted). https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:48: bool IsLoadLibSearchFlagsAvailable() { On 2017/03/14 21:48:25, xhwang_slow wrote: > nit: s/Is/Are. Also since this is related to LOAD_LIBRARY_SEARCH flags. So to be > exact, probably name it to > > AreLoadLibrarySearchFlagsAvailable() > > or simply > > AreSearchFlagsAvailable() since this whole file is in the context of > "LoadLibrary". Renamed to AreSearchFlagsAvailable(). https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:56: // method above. On 2017/03/14 21:48:25, xhwang_slow wrote: > "above" is not accurate now Done. https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:67: HMODULE load_library_module = nullptr; On 2017/03/14 21:48:25, xhwang_slow wrote: > nit: ISTM the old name |module| is concise and clear, and |load_library| here > doesn't add much value. Done. https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:92: // LoadLibraryW API. On 2017/03/14 21:48:25, xhwang_slow wrote: > nit: add an empty line here since these two lines are for all the code left in > this function. Done. https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:120: } else if (load_library_module) On 2017/03/14 21:48:25, xhwang_slow wrote: > nit: use curly here and below as well: > > https://google.github.io/styleguide/cppguide.html#Conditionals > > // Not allowed - curly on IF but not ELSE > if (condition) { > foo; > } else > bar; Done. https://codereview.chromium.org/2744043003/diff/180001/base/native_library_wi... base/native_library_win.cc:123: load_library_status = LoadLibraryResult::UNAVAILABLE_AND_FAIL; On 2017/03/14 21:48:25, xhwang_slow wrote: > nit: Probably move these into a helper function as well to keep the main logic > clean: > > LoadLibraryResult GetLoadLibraryResult(bool, bool) Done. https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24522: + results, which are used in LoadNativeLibraryDynamically method in On 2017/03/14 21:48:25, xhwang_slow wrote: > LoadNativeLibraryDynamically is not true now... Renamed. https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24522: + results, which are used in LoadNativeLibraryDynamically method in On 2017/03/14 18:42:28, Ilya Sherman wrote: > nit: s/used in/used in Done. https://codereview.chromium.org/2744043003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:98792: + <int value="0" label="LoadLibraryExW Succeed"/> On 2017/03/14 18:42:28, Ilya Sherman wrote: > nit: either "Succeeded" or "Success" Done.
Did you manually test this against crbug/700208? Otherwise LGTM % nit Thank you again for working on this! https://codereview.chromium.org/2744043003/diff/200001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/200001/base/native_library_wi... base/native_library_win.cc:67: bool is_LoadLibraryW_succeeded) { nit: we don't use upper cases in variable names...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Do we need to use the fallback path if we can use LoadLibraryExW? It seems ideal to avoid LoadLibraryW completely if at all possible. https://codereview.chromium.org/2744043003/diff/200001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/200001/base/native_library_wi... base/native_library_win.cc:18: typedef HMODULE(WINAPI* AddDllDirectory)(PCWSTR NewDirectory); Nit: since we're changing this anyway, please switch it to using AddDllDirectory = HMODULE(*)(PCWSTR new_directory);
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/15 09:33:49, dcheng wrote: > Do we need to use the fallback path if we can use LoadLibraryExW? It seems ideal > to avoid LoadLibraryW completely if at all possible. 1) If the LOAD_LIBRARY_SEARCH_* flags are unavailable even though LoadLibraryExW is on system, we have to use LoadLibraryW to do the directory trick so that we can search for the DLL directory as well. More details can be found if you search "KB2533623" in https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx 2) If both LoadLibraryExW and the flags are available but its call fails, do we still want to try LoadLibraryW? IMHO, it is worthwhile to have a try using LoadLibraryW. Maybe the directory trick with LoadLibraryW succeeds in some situations while LoadLibraryExW simply fails. Please correct me if I misunderstood your question.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments addressed in the new patch set. https://codereview.chromium.org/2744043003/diff/200001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/200001/base/native_library_wi... base/native_library_win.cc:18: typedef HMODULE(WINAPI* AddDllDirectory)(PCWSTR NewDirectory); On 2017/03/15 09:33:49, dcheng wrote: > Nit: since we're changing this anyway, please switch it to using AddDllDirectory > = HMODULE(*)(PCWSTR new_directory); Done. https://codereview.chromium.org/2744043003/diff/200001/base/native_library_wi... base/native_library_win.cc:67: bool is_LoadLibraryW_succeeded) { On 2017/03/14 23:17:41, xhwang_slow wrote: > nit: we don't use upper cases in variable names... Done.
On 2017/03/15 17:50:31, chengx wrote: > On 2017/03/15 09:33:49, dcheng wrote: > > Do we need to use the fallback path if we can use LoadLibraryExW? It seems > ideal > > to avoid LoadLibraryW completely if at all possible. > > 1) If the LOAD_LIBRARY_SEARCH_* flags are unavailable even though LoadLibraryExW > is on system, we have to use LoadLibraryW to do the directory trick so that we > can search for the DLL directory as well. More details can be found if you > search "KB2533623" in > https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx Right, I understand we need the fallback to handle the case where the KB isn't installed. > > 2) If both LoadLibraryExW and the flags are available but its call fails, do we > still want to try LoadLibraryW? IMHO, it is worthwhile to have a try using > LoadLibraryW. Maybe the directory trick with LoadLibraryW succeeds in some > situations while LoadLibraryExW simply fails. > > Please correct me if I misunderstood your question. But it seems like we should strictly prefer the LoadLibraryExW path over the LoadLibraryW path. I'm OK with it landing like this, but if we find out that FAIL_AND_FAIL is the primary failure mode, can we come back to remove the fallback for the case when the search flags are available?
On 2017/03/15 19:57:07, dcheng wrote: > On 2017/03/15 17:50:31, chengx wrote: > > On 2017/03/15 09:33:49, dcheng wrote: > > > Do we need to use the fallback path if we can use LoadLibraryExW? It seems > > ideal > > > to avoid LoadLibraryW completely if at all possible. > > > > 1) If the LOAD_LIBRARY_SEARCH_* flags are unavailable even though > LoadLibraryExW > > is on system, we have to use LoadLibraryW to do the directory trick so that we > > can search for the DLL directory as well. More details can be found if you > > search "KB2533623" in > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx > > Right, I understand we need the fallback to handle the case where the KB isn't > installed. > > > > > 2) If both LoadLibraryExW and the flags are available but its call fails, do > we > > still want to try LoadLibraryW? IMHO, it is worthwhile to have a try using > > LoadLibraryW. Maybe the directory trick with LoadLibraryW succeeds in some > > situations while LoadLibraryExW simply fails. > > > > Please correct me if I misunderstood your question. > > But it seems like we should strictly prefer the LoadLibraryExW path over the > LoadLibraryW path. I'm OK with it landing like this, but if we find out that > FAIL_AND_FAIL is the primary failure mode, can we come back to remove the > fallback for the case when the search flags are available? I agree. We can have the current impl in one release, then if FAIL_AND_SUCCESS is close to 0, we can safely remove the fallback path.
On 2017/03/15 19:59:06, xhwang_slow wrote: > On 2017/03/15 19:57:07, dcheng wrote: > > On 2017/03/15 17:50:31, chengx wrote: > > > On 2017/03/15 09:33:49, dcheng wrote: > > > > Do we need to use the fallback path if we can use LoadLibraryExW? It seems > > > ideal > > > > to avoid LoadLibraryW completely if at all possible. > > > > > > 1) If the LOAD_LIBRARY_SEARCH_* flags are unavailable even though > > LoadLibraryExW > > > is on system, we have to use LoadLibraryW to do the directory trick so that > we > > > can search for the DLL directory as well. More details can be found if you > > > search "KB2533623" in > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx > > > > Right, I understand we need the fallback to handle the case where the KB isn't > > installed. > > > > > > > > 2) If both LoadLibraryExW and the flags are available but its call fails, do > > we > > > still want to try LoadLibraryW? IMHO, it is worthwhile to have a try using > > > LoadLibraryW. Maybe the directory trick with LoadLibraryW succeeds in some > > > situations while LoadLibraryExW simply fails. > > > > > > Please correct me if I misunderstood your question. > > > > But it seems like we should strictly prefer the LoadLibraryExW path over the > > LoadLibraryW path. I'm OK with it landing like this, but if we find out that > > FAIL_AND_FAIL is the primary failure mode, can we come back to remove the > > fallback for the case when the search flags are available? > > I agree. We can have the current impl in one release, then if FAIL_AND_SUCCESS > is close to 0, we can safely remove the fallback path. BTW, we should add a TODO with tracking bug for this follow up work.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Patchset #8 (id:240001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/15 19:59:38, xhwang_slow wrote: > On 2017/03/15 19:59:06, xhwang_slow wrote: > > On 2017/03/15 19:57:07, dcheng wrote: > > > But it seems like we should strictly prefer the LoadLibraryExW path over the > > > LoadLibraryW path. I'm OK with it landing like this, but if we find out that > > > FAIL_AND_FAIL is the primary failure mode, can we come back to remove the > > > fallback for the case when the search flags are available? > > > > I agree. We can have the current impl in one release, then if FAIL_AND_SUCCESS > > is close to 0, we can safely remove the fallback path. > > BTW, we should add a TODO with tracking bug for this follow up work. I agree if LoadLibraryW shows no extra benefits at all, we should remove the fallback for the case when the search flags are available. I have filed a bug crbug.com/701944 to record this future work and cc'ed you two. A TODO comment is also added in the code. Thanks!
Thanks! Have you verified whether UMA reporting is working as expected? https://codereview.chromium.org/2744043003/diff/260001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/260001/base/native_library_wi... base/native_library_win.cc:67: bool is_load_library_succeeded) { nit: is..succeeded doesn't sound right, how about has...succeeded?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. BUG=700503,700208 ========== to ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. Running Chromium built with this CL locally shows that LoadLibraryExW call were successful for kernel32.dll and widevinecdm.dll (which caused crbug.com/700208), but failed when loading MDMRegistration.dll. LoadLibraryW succeeds in loading MDMRegistration.dll though. BUG=700503,700208 ==========
PTAL~ On 2017/03/15 21:04:06, xhwang_slow wrote: > Thanks! > > Have you verified whether UMA reporting is working as expected? Verified and updated the description for this CL. https://codereview.chromium.org/2744043003/diff/260001/base/native_library_wi... File base/native_library_win.cc (right): https://codereview.chromium.org/2744043003/diff/260001/base/native_library_wi... base/native_library_win.cc:67: bool is_load_library_succeeded) { On 2017/03/15 21:04:06, xhwang_slow wrote: > nit: is..succeeded doesn't sound right, how about has...succeeded? Done.
On 2017/03/15 23:52:30, chengx wrote: > PTAL~ > > On 2017/03/15 21:04:06, xhwang_slow wrote: > > Thanks! > > > > Have you verified whether UMA reporting is working as expected? > > Verified and updated the description for this CL. > > https://codereview.chromium.org/2744043003/diff/260001/base/native_library_wi... > File base/native_library_win.cc (right): > > https://codereview.chromium.org/2744043003/diff/260001/base/native_library_wi... > base/native_library_win.cc:67: bool is_load_library_succeeded) { > On 2017/03/15 21:04:06, xhwang_slow wrote: > > nit: is..succeeded doesn't sound right, how about has...succeeded? > > Done. When LoadLibraryW successfully loads MDMRegistration.dll, do you see anything special in Process Monitor? You can also use Dependency Walker to inspect MDMRegistration.dll's dependencies. It's odd that LoadLibraryExW failed but LoadLibraryW worked...
On 2017/03/16 00:20:00, xhwang_slow wrote: > When LoadLibraryW successfully loads MDMRegistration.dll, do you see anything > special in Process Monitor? You can also use Dependency Walker to inspect > MDMRegistration.dll's dependencies. It's odd that LoadLibraryExW failed but > LoadLibraryW worked... I will reply to you in the email to attach the screenshot.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2744043003/#ps280001 (title: "Better variable name.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1489643109593920,
"parent_rev": "2a7d3504083f59f10b9150d58a00a863056750a3", "commit_rev":
"5946c9233dc393f2e3e275323e66659fe5fb35cd"}
Message was sent while issue was closed.
Description was changed from ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. Running Chromium built with this CL locally shows that LoadLibraryExW call were successful for kernel32.dll and widevinecdm.dll (which caused crbug.com/700208), but failed when loading MDMRegistration.dll. LoadLibraryW succeeds in loading MDMRegistration.dll though. BUG=700503,700208 ========== to ========== Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "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." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. Running Chromium built with this CL locally shows that LoadLibraryExW call were successful for kernel32.dll and widevinecdm.dll (which caused crbug.com/700208), but failed when loading MDMRegistration.dll. LoadLibraryW succeeds in loading MDMRegistration.dll though. BUG=700503,700208 Review-Url: https://codereview.chromium.org/2744043003 Cr-Commit-Position: refs/heads/master@{#457359} Committed: https://chromium.googlesource.com/chromium/src/+/5946c9233dc393f2e3e275323e66... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001) as https://chromium.googlesource.com/chromium/src/+/5946c9233dc393f2e3e275323e66... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
