Get module versions and types from in-memory images

Get module versions and types from in-memory images

Don't call GetFileVersionInfo(), which calls LoadLibrary() to be able to
access the module's resources. Loading modules from the crashy process
into the handler process can cause trouble. The Crashpad handler
definitely doesn't want to run arbitrary modules' module initializer
code.

Since the VS_FIXEDFILEINFO needed is already in memory in the remote
process' address space, just access it from there.

BUG=crashpad:78
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/5be8ce4ea0fe2345ce6c843c53432e07a22cca41

[Mark Mentovai, 2015-11-25 22:10:45]

mark@chromium.org changed reviewers:
+ scottmg@chromium.org

[Mark Mentovai, 2015-11-25 22:10:46]

If you know any more than this about the language search order, I’d be
interested…

[scottmg, 2015-11-26 21:30:00]

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_reade...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_reade...
snapshot/win/pe_image_reader.cc:242: version_info.wValueLength <
sizeof(version_info.Value) ||
!= for this one rather than <, as if it's not our expected VS_FIXEDFILEINFO
we're confused/out of luck.

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
File snapshot/win/pe_image_resource_reader.cc (right):

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:141: return entry.DataIsDirectory ?
entry.OffsetToDirectory
The != above warns but this one doesn't? Silly.

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:166: // TODO(mark): It seems like the
FindResourceEx() might do something more
Remove "the ".

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:172:
try_languages.push_back(MAKELANGID(LANG_ENGLISH, SUBLANG_DEFAULT));
Presumably this should be the UI language rather than LANG_ENGLISH.

I wondered about adding LANG_INVARIANT here (just based on its name), but I see
little documentation about it, and I don't really know what it is.

I mucked around in windbg for a while, and I think the similar functionality is
happening in ntdll!LdrpSearchResourceSection_U which looks substantially more
complicated than this. I think there's probably a variety of more other things
it tries in the language == 0 case.

But I think the fallback of first entry is realistically fine for our purposes,
so I don't know that it's important that we do anything extra here.

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:240: sizeof((*named_entries)[0]),
Just confirming that this is OK when named_entries is null, right?

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:251: bool
PEImageResourceReader::CheckedReadMemory(WinVMAddress address,
Can we avoid duplicating this here and in PEImageReader?

https://codereview.chromium.org/1475023004/diff/1/util/win/module_version.h
File util/win/module_version.h (right):

https://codereview.chromium.org/1475023004/diff/1/util/win/module_version.h#n...
util/win/module_version.h:27: //! This function calls `GetFileVersionInfo()`,
which can implicitly call
This is just here for system_snapshot_win.cc now? We should switch that to
GetModuleHandle("kernel32.dll") and the new code and delete this entirely.

... Oh, the test too. Maybe it'd be better to move it into the test file or
test/ somewhere.

[Mark Mentovai, 2015-11-27 17:56:28]

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
File snapshot/win/pe_image_resource_reader.cc (right):

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:141: return entry.DataIsDirectory ?
entry.OffsetToDirectory
scottmg wrote:
> The != above warns but this one doesn't? Silly.

What do you mean? It’s allowed to be a directory or a non-directory here.

[scottmg, 2015-11-27 18:06:59]

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
File snapshot/win/pe_image_resource_reader.cc (right):

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:141: return entry.DataIsDirectory ?
entry.OffsetToDirectory
On 2015/11/27 17:56:28, Mark Mentovai wrote:
> scottmg wrote:
> > The != above warns but this one doesn't? Silly.
> 
> What do you mean? It’s allowed to be a directory or a non-directory here.

Sorry, I just meant that there wasn't a need to
static_cast<bool>(entry.DataIsDirectory) here. I was assuming the cast above was
to suppress a warning.

Nothing to do here.

[Mark Mentovai, 2015-11-27 21:40:38]

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
File snapshot/win/pe_image_resource_reader.cc (right):

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:141: return entry.DataIsDirectory ?
entry.OffsetToDirectory
scottmg wrote:
> Sorry, I just meant that there wasn't a need to
> static_cast<bool>(entry.DataIsDirectory) here. I was assuming the cast above
was
> to suppress a warning.
> 
> Nothing to do here.

Oh compiler warning, yeah. That C4800 “performance warning” is pretty bogus and
inconsistent.

[Mark Mentovai, 2015-12-01 18:48:01]

Thanks for the careful review!

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_reade...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_reade...
snapshot/win/pe_image_reader.cc:242: version_info.wValueLength <
sizeof(version_info.Value) ||
scottmg wrote:
> != for this one rather than <, as if it's not our expected VS_FIXEDFILEINFO
> we're confused/out of luck.

Done.

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
File snapshot/win/pe_image_resource_reader.cc (right):

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:166: // TODO(mark): It seems like the
FindResourceEx() might do something more
scottmg wrote:
> Remove "the ".

Done.

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:172:
try_languages.push_back(MAKELANGID(LANG_ENGLISH, SUBLANG_DEFAULT));
scottmg wrote:
> Presumably this should be the UI language rather than LANG_ENGLISH.
> 
> I wondered about adding LANG_INVARIANT here (just based on its name), but I
see
> little documentation about it, and I don't really know what it is.
> 
> I mucked around in windbg for a while, and I think the similar functionality
is
> happening in ntdll!LdrpSearchResourceSection_U which looks substantially more
> complicated than this. I think there's probably a variety of more other things
> it tries in the language == 0 case.
> 
> But I think the fallback of first entry is realistically fine for our
purposes,
> so I don't know that it's important that we do anything extra here.

I changed this up a little bit, but I still don‘t think it totally matches what
Windows does. But it’s closer.

If the language is neutral, it tries the languages from the thread/user/system
locales up front. The hard-coded English fallback down here is intentional. It’s
what the document referenced above says, it’s in ReactOS too, and it matches the
only language found in MS-shipped DLLs like kernel32. (And I doubt that there
are language-specific kernel32s out there, but maybe I’m wrong.)

What the code does now is kind of like ReactOS, but slightly different. There’s
no way to specify that you want to get a resource of
LANG_NEUTRAL/SUBLANG_NEUTRAL as a first choice, but that’s how FindResourceEx()
works too.

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:240: sizeof((*named_entries)[0]),
scottmg wrote:
> Just confirming that this is OK when named_entries is null, right?

I changed it to the type name to avoid problems with overzealous runtime
analysis like ubsan. I’m not saying that ubsan or something else of its ilk
would hate this, just that I can’t say that it’s “definitely OK”.

https://codereview.chromium.org/1475023004/diff/1/snapshot/win/pe_image_resou...
snapshot/win/pe_image_resource_reader.cc:251: bool
PEImageResourceReader::CheckedReadMemory(WinVMAddress address,
scottmg wrote:
> Can we avoid duplicating this here and in PEImageReader?

Done.

https://codereview.chromium.org/1475023004/diff/1/util/win/module_version.h
File util/win/module_version.h (right):

https://codereview.chromium.org/1475023004/diff/1/util/win/module_version.h#n...
util/win/module_version.h:27: //! This function calls `GetFileVersionInfo()`,
which can implicitly call
scottmg wrote:
> This is just here for system_snapshot_win.cc now? We should switch that to
> GetModuleHandle("kernel32.dll") and the new code and delete this entirely.
> 
> ... Oh, the test too. Maybe it'd be better to move it into the test file or
> test/ somewhere.

I started going down this road, but then I stopped when I needed to call
GetModuleInformation() to get the MODULEINFO, which brings in psapi.dll. psapi
seems a little weird, and we only use it in a test now. Is it desirable in
non-test code?

[scottmg, 2015-12-01 19:43:02]

lgtm

https://codereview.chromium.org/1475023004/diff/1/util/win/module_version.h
File util/win/module_version.h (right):

https://codereview.chromium.org/1475023004/diff/1/util/win/module_version.h#n...
util/win/module_version.h:27: //! This function calls `GetFileVersionInfo()`,
which can implicitly call
On 2015/12/01 18:48:01, Mark Mentovai wrote:
> scottmg wrote:
> > This is just here for system_snapshot_win.cc now? We should switch that to
> > GetModuleHandle("kernel32.dll") and the new code and delete this entirely.
> > 
> > ... Oh, the test too. Maybe it'd be better to move it into the test file or
> > test/ somewhere.
> 
> I started going down this road, but then I stopped when I needed to call
> GetModuleInformation() to get the MODULEINFO, which brings in psapi.dll. psapi
> seems a little weird, and we only use it in a test now. Is it desirable in
> non-test code?

Hmm, right, forgot about that. Seems like we should be able to avoid needing to
do that.

But in any case, since we're only using it for kernel32 it's fine to keep using
it, and make sure to heed the warning here in the future.

https://codereview.chromium.org/1475023004/diff/20001/snapshot/win/process_su...
File snapshot/win/process_subrange_reader.h (right):

https://codereview.chromium.org/1475023004/diff/20001/snapshot/win/process_su...
snapshot/win/process_subrange_reader.h:98: // Common helper for Initialize() and
InitializeInternal().
InitializeSubrange() rather than InitializeInternal().

[Mark Mentovai, 2015-12-01 22:06:42]

Description was changed from

==========
Get module versions and types from in-memory images

Don't call GetFileVersionInfo(), which calls LoadLibrary() to be able to
access the module's resources. Loading modules from the crashy process
into the handler process can cause trouble. The Crashpad handler
definitely doesn't want to run arbitrary modules' module initializer
code.

Since the VS_FIXEDFILEINFO needed is already in memory in the remote
process' address space, just access it from there.

BUG=crashpad:78
==========

to

==========
Get module versions and types from in-memory images

Don't call GetFileVersionInfo(), which calls LoadLibrary() to be able to
access the module's resources. Loading modules from the crashy process
into the handler process can cause trouble. The Crashpad handler
definitely doesn't want to run arbitrary modules' module initializer
code.

Since the VS_FIXEDFILEINFO needed is already in memory in the remote
process' address space, just access it from there.

BUG=crashpad:78
R=scottmg@chromium.org

Committed:
https://chromium.googlesource.com/crashpad/crashpad/+/5be8ce4ea0fe2345ce6c843...
==========

[Mark Mentovai, 2015-12-01 22:06:44]

Committed patchset #3 (id:40001) manually as
5be8ce4ea0fe2345ce6c843c53432e07a22cca41 (presubmit successful).