magreenblatt
commented
7 years ago Original comment by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
Closed magreenblatt closed 7 years ago
magreenblatt
commented
7 years ago Original comment by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
magreenblatt
commented
7 years ago Original comment by Peiyuan Song (Bitbucket: SquallATF, GitHub: SquallATF).
Issue #2123 was marked as a duplicate of this issue.
Because chrome_elf.dll set hook for all pe module, but .NET Any Cpu bin is pe32 not pe64, chrome_elf will crash when DisableSetUnhandledExceptionFilter.
The easiest way to fix this issue is create a patch for base/win/pe_image.cc, in PEImage::VerifyMagic() function, check pe image type ,if not pe64 return false to skip hook.
#ifdef _WIN64
if (nt_headers->FileHeader.Machine != IMAGE_FILE_MACHINE_AMD64)
return false;
#endif // _WIN64Original comment by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
@SquallATF it will be a bit dirty, because in that case CEF debug builds will constantly do crash because of assert in DisableSetUnhandledExceptionFilter. This acceptable, but annoying in long-term.
magreenblatt
commented
7 years ago magreenblatt
commented
7 years ago Original comment by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
@SquallATF actually this requires additional testing to understand how it is really affects on exception handling:
throwing .NET exceptions from handler methods (i.e. when CEF on stack)
system exceptions (like AV) from CEF
And this handling in cases:
Exceptions on UI thread (which runs CefRunMessageLoop)
Exceptions on UI thread (with wrap CefRunMessageLoop with try/catch, and mark wrapper method with HandleProcessCorruptedStateExceptions attribute) (previously only one way to reliable die in case of AVs)
Exceptions on other CEF threads
Previously all .NET exceptions had been handled by AppDomain.OnUnhandledException regardless to where they occurs, but this partially breaks crash dump generation. But this should be carefully tested to make productive decisions.
I'm no have do plans do this just now (not earlier than CefGlue vNext will be released, and this testings effectively in scope of .NET bindings rather than CEF).
Personally i'm switched to use of separate executables for x64 / x86, so not critical.
magreenblatt
commented
7 years ago Original comment by Peiyuan Song (Bitbucket: SquallATF, GitHub: SquallATF).
@dmitry-azaraev may be I was wrong:
Any CPU Assembly at runtime will fix PE32 Header to PE64(not change FileHeader.Machine), but I did not known why IMAGE_THUNK_DATA did not fix.
DisableSetUnhandledExceptionFilter only set hook on SetUnhandledExceptionFilter from kernel32.dll, but .NET Assembly only import _CorExeMain from mscoree.dll, skip main module hook may not affect chrome_elf work.
void DisableSetUnhandledExceptionFilter() {
if (g_set_unhandled_exception_filter == nullptr) {
g_set_unhandled_exception_filter = new elf_hook::IATHook();
}
if (g_set_unhandled_exception_filter->Hook(
::GetModuleHandle(nullptr), "kernel32.dll",
"SetUnhandledExceptionFilter",
SetUnhandledExceptionFilterPatch) != NO_ERROR) {
#ifdef _DEBUG
assert(false);
#endif //_DEBUG
}
}chrome_elf search imports and delay imports and set hook, did not hook child module, so can safely ignore hook set for the .NET assembly:
// Applies an import-address-table hook. Returns a system winerror.h code.
// Call RemoveIATHook() with |new_function|, |old_function| and |iat_thunk|
// to remove the hook.
DWORD ApplyIATHook(HMODULE module_handle,
const char* imported_from_module,
const char* function_name,
void* new_function,
void** old_function,
IMAGE_THUNK_DATA** iat_thunk) {
base::win::PEImage target_image(module_handle);
if (!target_image.VerifyMagic())
return ERROR_INVALID_PARAMETER;
IATHookFunctionInfo hook_info = { false,
imported_from_module,
function_name,
new_function,
old_function,
iat_thunk,
ERROR_PROC_NOT_FOUND };
// First go through the IAT. If we don't find the import we are looking
// for in IAT, search delay import table.
target_image.EnumAllImports(IATFindHookFuncCallback, &hook_info);
if (!hook_info.finished_operation) {
target_image.EnumAllDelayImports(IATFindHookFuncCallback, &hook_info);
}
return hook_info.return_code;
}Original comment by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
@SquallATF i'm don't think that you are wrong. The only thing about .NET modules is that it is undocumented very well: when they introduce .NET modules as native executables, they implement them via shims (mscoree import which loads CLR). But starting from Windows 7? they integrate .NET module recognition into OS loader: it effectively can ignore all of this shims, as well as transform PE to corresponding arch. But... as you show - it is doesn't work in this way (surprisingly for me).
From "correctness" perspective I think that it is better to recognize if we running in .NET AnyCPU module in DisableSetUnhandledExceptionFilter before calling Hook, and suppress this call in that case.
I.e. in pseudo-code:
#!c++
void DisableSetUnhandledExceptionFilter() {
if (g_set_unhandled_exception_filter == nullptr) {
g_set_unhandled_exception_filter = new elf_hook::IATHook();
}
base::win::PEImage target_image(::GetModuleHandle(nullptr));
if (target_image.VerifyMagicNetAnyCpu()) return; // assume that we patch PEImage and add this method
// but should be doable ad-hoc in chrome_elf if we don't want patch chrome_elf
if (g_set_unhandled_exception_filter->Hook(
::GetModuleHandle(nullptr), "kernel32.dll",
"SetUnhandledExceptionFilter",
SetUnhandledExceptionFilterPatch) != NO_ERROR) {
#ifdef _DEBUG
assert(false);
#endif //_DEBUG
}
}But, i'm also can be wrong.
PS: Also it is interesting how it works before in CEF 56.
magreenblatt
commented
7 years ago Original comment by Peiyuan Song (Bitbucket: SquallATF, GitHub: SquallATF).
@dmitry-azaraev may be this is why CEF 56 can work chrome_elf_main.cc， ADDRESS_SANITIZER is defined then DisableSetUnhandledExceptionFilter will not executed.
#if !defined(ADDRESS_SANITIZER)
elf_crash::DisableSetUnhandledExceptionFilter();
#endif // !defined (ADDRESS_SANITIZER)Original comment by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
@SquallATF FYI: after 7 hours bitbucket still not publish your last comment into this issue, but notify me on email. I'm repost your last comment again.
May be this is why CEF 56 can work chrome_elf_main.cc, ADDRESS_SANITIZER is defined then DisableSetUnhandledExceptionFilter may not executed.
#!c++
#if !defined(ADDRESS_SANITIZER)
elf_crash::DisableSetUnhandledExceptionFilter();
#endif // !defined (ADDRESS_SANITIZER)Looks like the early chrome_elf initialization has been reverted in https://bugs.chromium.org/p/chromium/issues/detail?id=700371#c21 and the revert will be merged back to 2987 branch shortly. That may be sufficient to resolve this issue.
magreenblatt
commented
7 years ago Original comment by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
Thanks. Looks like this kind of revert has no chance be included in standard CEF distribution. If we had right - testing that image is PE32+AnyCPU should be actually a tiny patch, which should be much easier to maintain.
magreenblatt
commented
7 years ago Original comment by Peiyuan Song (Bitbucket: SquallATF, GitHub: SquallATF).
I try to write a method to verify dot net AnyCPU.
bool PEImage::IsDotNetAnyCPU() const {
PVOID directory = GetImageDirectoryEntryAddr(IMAGE_DIRECTORY_ENTRY_COM_DESCRIPTOR);
DWORD size = GetImageDirectoryEntrySize(IMAGE_DIRECTORY_ENTRY_COM_DESCRIPTOR);
if (NULL == directory || 0 == size)
return false;
PIMAGE_COR20_HEADER cor20 = reinterpret_cast<PIMAGE_COR20_HEADER>(
directory);
if (cor20->Flags != COMIMAGE_FLAGS_ILONLY)
return false;
return true;
}The early chrome_elf initialization has been reverted in 57.0.2987.110 (https://bitbucket.org/chromiumembedded/cef/commits/ffc57735)
magreenblatt
commented
7 years ago I believe this issue is resolved with the revert in Chromium. Please re-open if you disagree.
magreenblatt
commented
7 years ago magreenblatt
commented
7 years ago magreenblatt
commented
7 years ago Original changes by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
magreenblatt
commented
7 years ago Original changes by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
magreenblatt
commented
7 years ago Original changes by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
magreenblatt
commented
7 years ago
Original report by Dmitry Azaraev (Bitbucket: dmitry-azaraev, GitHub: dmitry-azaraev).
I'm encounter in issue that x64 CEF 57 (2987 stable release branch at this moment) build just crashes. Issue are tied to crashpad integration, but there is no matter has config file or it absent, because it is no matter actually, due to nature of chrome_elf integration. I'm tried spotify's builds 3.2987.1591 as well as 1590. My private local 1591 also fails (both release & debug configurations).
I'm get reproduction of this issue by using very simple .NET program (be sure that you are remove prefer 32-bit and use AnyCPU (x64) build). Assemblies which is marked with pure x64 architecture is actually works. I.e. issue happens only on arch-hybrid assemblies.
This program just prints some CEF version info.
It works on x86, and should produce next output:
Instead on x64 it is just crashed and what of course handled by WerFault silently (no any dialogs, no ask for debugging). But configuring WER is easy for application, so out of scope.
I'm debugged error case with WinDbg a bit and got crash site:
Stacktrace (only interested thread is shown):
Exception Info:
Loaded modules:
Of course when EnumOneImportChunk fails it is process "mscoree.dll" (you can see it from callstack), so it is surely tied to .NET CLR.
This code path triggered by chrome_elf's DllMain:
PS: I'm tried to blacklist "mscoree.dll" in chrome_elf's blacklist, but then realize that it is not used in this case.
PPS: CEF 56 (which is also built with chrome_elf) has no this issue there. I.e. it is work at same host with same .NET CLR version.