win: Support reading process info cross-bitness

win: Support reading process info cross-bitness

This only works 64->64, 32->32, and 64->32. We shouldn't have a
need for 32->64. It will also currently not work if the crash service
is running on Wow64 itself (that is, 32->32, but on an x64 OS). We
should also be able to avoid needing that.

Primarily, this change templatizes the winternl.h process structure
types on word size, so the PEB can be read in a foreign bitsize process.

This also happens to resolve using void* as pointer values into foreign
processes, as they're now all either DWORD or DWORD64 depending on which
traits class is used.

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/ab6cdb6bc18ebb91b9523ee12d0385743f0cccad

[scottmg, 2015-03-07 02:53:05]

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

[scottmg, 2015-03-07 02:53:05]

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

[scottmg, 2015-03-07 02:59:31]

Patchset #4 (id:60001) has been deleted

[scottmg, 2015-03-07 02:59:37]

Patchset #3 (id:40001) has been deleted

[scottmg, 2015-03-07 02:59:44]

Patchset #2 (id:20001) has been deleted

[scottmg, 2015-03-07 03:03:49]

Patchset #2 (id:80001) has been deleted

[scottmg, 2015-03-07 03:07:21]

Patchset #2 (id:100001) has been deleted

[Mark Mentovai, 2015-03-09 16:11:46]

Cool, Scott!

https://codereview.chromium.org/981393003/diff/120001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/981393003/diff/120001/util/win/process_info.c...
util/win/process_info.cc:105: uintptr_t peb_address,
peb_address is in the remote process’ address space, so wouldn’t it be better to
base this on T rather than the native pointer width?

If you need to accept a single type as the argument here because the callers
haven’t specialized it to a bit-width-specific type yet, you can convert it to T
right in this function by using AssignIfInRange(), and the just use that.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_info.c...
util/win/process_info.cc:213: // We assume this process is not running on Wow64.
The
I don’t think that this assumption can really hold in the long term, so I’d make
this a TODO. This is fine as an intermediate step for now, though.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_structs.h
File util/win/process_structs.h (right):

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:20: namespace crashpad {
Although we rarely have subnamespaces inside “crashpad” (aside from “test” and
“internal”), I think one is called for here, in the same way that the Mac
equivalent uses crashpad::process_types. Can we use crashpad::process_structs
here? That would let us get rid of the _T on each structure, and it makes it
more obvious at use sites that we’re working with our custom type. (The _T tends
to get lots when you’re reading, especially because the Windows structs are all
full of underscores and capital letters to begin with.)

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:22: //! \brief Selected structures from winternl.h,
ntddk.h, and `dt ntdll!xxx`,
You should have a //! \{ before this, and a //! \} at the end of the file, so
that this comment is used as the documentation for everything in the contained
group. Otherwise, it’ll wind up being applied only to the first thing that
follows it.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:30: //! In all cases, the `T` in the templates below
must be either DWORD for
I think I’d like traits a little better, so that the definitions could use
something like T::Pointer, T::UIntPtr, etc. That helps things be a little more
documentary. It’s important in the code that uses this too, where “T peb_address
= …” isn’t as obvious as “Traits::Pointer peb_address = …”.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:33: #pragma pack(push, 1)
Is this true for the native structs too? Or are you accounting for padding
manually in a way that the native structs didn’t have to because they just rely
on native packing? It seems like the latter based on your use of unions and
padding fields—in that case, add a comment (//, not //!) saying that padding is
up to you in here.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:148: RTL_DRIVE_LETTER_CURDIR_T<T>
CurrentDirectores[32];  // sic.
Ha!

[scottmg, 2015-03-09 21:14:42]

Patchset #3 (id:140001) has been deleted

[scottmg, 2015-03-09 21:16:37]

https://codereview.chromium.org/981393003/diff/120001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/981393003/diff/120001/util/win/process_info.c...
util/win/process_info.cc:105: uintptr_t peb_address,
On 2015/03/09 16:11:46, Mark Mentovai wrote:
> peb_address is in the remote process’ address space, so wouldn’t it be better
to
> base this on T rather than the native pointer width?
> 
> If you need to accept a single type as the argument here because the callers
> haven’t specialized it to a bit-width-specific type yet, you can convert it to
T
> right in this function by using AssignIfInRange(), and the just use that.

Right, that's why. Done.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_info.c...
util/win/process_info.cc:213: // We assume this process is not running on Wow64.
The
On 2015/03/09 16:11:46, Mark Mentovai wrote:
> I don’t think that this assumption can really hold in the long term, so I’d
make
> this a TODO. This is fine as an intermediate step for now, though.

OK. I would prefer not to support running as Wow64 if at all possible, but added
todo for now anyway. We'd of course at least need to actually check that
somewhere.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_structs.h
File util/win/process_structs.h (right):

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:20: namespace crashpad {
On 2015/03/09 16:11:46, Mark Mentovai wrote:
> Although we rarely have subnamespaces inside “crashpad” (aside from “test” and
> “internal”), I think one is called for here, in the same way that the Mac
> equivalent uses crashpad::process_types. Can we use crashpad::process_structs
> here? That would let us get rid of the _T on each structure, and it makes it
> more obvious at use sites that we’re working with our custom type. (The _T
tends
> to get lots when you’re reading, especially because the Windows structs are
all
> full of underscores and capital letters to begin with.)

Done.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:22: //! \brief Selected structures from winternl.h,
ntddk.h, and `dt ntdll!xxx`,
On 2015/03/09 16:11:46, Mark Mentovai wrote:
> You should have a //! \{ before this, and a //! \} at the end of the file, so
> that this comment is used as the documentation for everything in the contained
> group. Otherwise, it’ll wind up being applied only to the first thing that
> follows it.

Done.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:30: //! In all cases, the `T` in the templates below
must be either DWORD for
On 2015/03/09 16:11:46, Mark Mentovai wrote:
> I think I’d like traits a little better, so that the definitions could use
> something like T::Pointer, T::UIntPtr, etc. That helps things be a little more
> documentary. It’s important in the code that uses this too, where “T
peb_address
> = …” isn’t as obvious as “Traits::Pointer peb_address = …”.

The user code does get better, but this gets pretty ugly. Better that way I
suppose. Done.

https://codereview.chromium.org/981393003/diff/120001/util/win/process_struct...
util/win/process_structs.h:33: #pragma pack(push, 1)
On 2015/03/09 16:11:46, Mark Mentovai wrote:
> Is this true for the native structs too? Or are you accounting for padding
> manually in a way that the native structs didn’t have to because they just
rely
> on native packing? It seems like the latter based on your use of unions and
> padding fields—in that case, add a comment (//, not //!) saying that padding
is
> up to you in here.

Right, I set it to 1 so that it would explicitly match what the OS calls expect.
It could have been left up to natural packing and padding, but this seemed a bit
safer/clearer when matching the `dt` dumps. Added a comment.

[Mark Mentovai, 2015-03-09 22:44:45]

LGTM

https://codereview.chromium.org/981393003/diff/160001/util/util.gyp
File util/util.gyp (right):

https://codereview.chromium.org/981393003/diff/160001/util/util.gyp#newcode346
util/util.gyp:346: 'target_name': 'util_test_process_info_test_child_x64',
Do you want to add a TargetMachine: 17 definition here so that this _x64
executable is independent of the bitness chosen by common.gypi? If not, the
target name is a misnomer.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.c...
util/win/process_info.cc:134: // Include the first module in the memory order
list to get our own name as
It’s not actually “our” name, it’s the other process’.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.c...
util/win/process_info.cc:136: std::wstring self_module;
Same concern with this variable’s name. executable_module?

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.c...
util/win/process_info.cc:137: process_types::LDR_DATA_TABLE_ENTRY<Traits>
self_ldr_data_table_entry;
And this variable too.

I guess that for both, you can just use module and ldr_data_table_entry, and not
have to redeclare them later when dealing with the initialization-order list.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_structs.h
File util/win/process_structs.h (right):

https://codereview.chromium.org/981393003/diff/160001/util/win/process_struct...
util/win/process_structs.h:27: using Integral = DWORD;
Let’s have both signed and unsigned. Anything that’s supposed to be signed
should be declared with the proper type. Right now, everything’s unsigned. If
there really isn’t anything that’s supposed to be signed, we should at least
name this one Unsigned so that the signedness is clear at use sites.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_struct...
util/win/process_structs.h:109: typename Traits::Pad padding_for_x86;
x86 but not x64? Same question once later in the file.

[scottmg, 2015-03-09 23:22:46]

Patchset #4 (id:180001) has been deleted

[scottmg, 2015-03-09 23:23:57]

Thanks!

https://codereview.chromium.org/981393003/diff/160001/util/util.gyp
File util/util.gyp (right):

https://codereview.chromium.org/981393003/diff/160001/util/util.gyp#newcode346
util/util.gyp:346: 'target_name': 'util_test_process_info_test_child_x64',
On 2015/03/09 22:44:44, Mark Mentovai wrote:
> Do you want to add a TargetMachine: 17 definition here so that this _x64
> executable is independent of the bitness chosen by common.gypi? If not, the
> target name is a misnomer.

Good point, done. Needed an msvs_configuration_platform too.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.c...
util/win/process_info.cc:134: // Include the first module in the memory order
list to get our own name as
On 2015/03/09 22:44:44, Mark Mentovai wrote:
> It’s not actually “our” name, it’s the other process’.

Done.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.c...
util/win/process_info.cc:136: std::wstring self_module;
On 2015/03/09 22:44:44, Mark Mentovai wrote:
> Same concern with this variable’s name. executable_module?

Done.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_info.c...
util/win/process_info.cc:137: process_types::LDR_DATA_TABLE_ENTRY<Traits>
self_ldr_data_table_entry;
On 2015/03/09 22:44:44, Mark Mentovai wrote:
> And this variable too.
> 
> I guess that for both, you can just use module and ldr_data_table_entry, and
not
> have to redeclare them later when dealing with the initialization-order list.

Done.

https://codereview.chromium.org/981393003/diff/160001/util/win/process_structs.h
File util/win/process_structs.h (right):

https://codereview.chromium.org/981393003/diff/160001/util/win/process_struct...
util/win/process_structs.h:27: using Integral = DWORD;
On 2015/03/09 22:44:44, Mark Mentovai wrote:
> Let’s have both signed and unsigned. Anything that’s supposed to be signed
> should be declared with the proper type. Right now, everything’s unsigned. If
> there really isn’t anything that’s supposed to be signed, we should at least
> name this one Unsigned so that the signedness is clear at use sites.

Done. I don't believe there are any that are signed.

[I didn't use Long and ULong similar to the Mac implementation as on Windows
ULONG means 4 bytes (and ULONG64 means 8 bytes) so that name would be confusing
in this file.]

https://codereview.chromium.org/981393003/diff/160001/util/win/process_struct...
util/win/process_structs.h:109: typename Traits::Pad padding_for_x86;
On 2015/03/09 22:44:44, Mark Mentovai wrote:
> x86 but not x64? Same question once later in the file.

Oops, fixed.

[Mark Mentovai, 2015-03-09 23:36:59]

Great! LGTM.

[scottmg, 2015-03-09 23:37:49]

Committed patchset #5 (id:210001) manually as
ab6cdb6bc18ebb91b9523ee12d0385743f0cccad (presubmit successful).