Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
977003003:
win: Add implementation of ProcessInfo (Closed)
|
Created:
5 years, 9 months ago by scottmg Modified:
5 years, 9 months ago Reviewers:
Mark Mentovai CC:
crashpad-dev_chromium.org Base URL:
https://chromium.googlesource.com/crashpad/crashpad@master Target Ref:
refs/heads/master Project:
crashpad Visibility:
Public. |
Descriptionwin: Add implementation of ProcessInfo
This is as a precursor to ProcessReader. Some basic functionality
is included for now, with more to be added later as necessary.
The PEB code is pretty icky -- walking the doubly-linked list
in the target's address space is cumbersome. The alternative
is to use EnumProcessModules. That would work but:
1) needs different APIs for XP and Vista 64+
2) retrieves modules in memory-location order, rather than
initialization order. I felt retrieving them in initialization order
might be useful when detecting third party DLL injections. In the
end, we may want to make both orders available.
R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/bed7a543c058b29bae1f24ab49799ff3d7198c73
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 75
Patch Set 4 : fixes #
Total comments: 25
Patch Set 5 : fixes2 #
Total comments: 6
Patch Set 6 : fixes3 #
Messages
Total messages: 13 (3 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
This is way less icky than your message led me to believe. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:21: std::wstring ReadUnicodeString(HANDLE process, const UNICODE_STRING& us) { In returning, this should have a way to distinguish between an error in ReadProcessMemory() and a successful read of an empty string. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:24: str.resize(us.Length / sizeof(wchar_t)); DCHECK_EQ(us.Length % sizeof(wchar_t), 0) https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:26: if (!ReadProcessMemory( If I’m reading this correctly, for cross-bitted operation, a 32-bit process can’t ReadProcessMemory() from a 64-bit process because us.Buffer is a native (32-bit) pointer. A 64-bit process would be able to ReadProcessMemory() from a 32-bit process, though. Does that sound right? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:29: PLOG(ERROR) << "ReadProcessMemory UNICODE_STRING"; #include "base/logging.h". Also, PLOG is incorrect if this line was reached because bytes_read != us.Length. Same on line 42. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:36: template <class T> bool ReadStruct(HANDLE process, void* at, T* into) { const void* at, but Using pointer types to refer to things in a different process’ address space is hairy, because it’s easy to mistake it for a pointer to the local address space and accidentally dereference it. [u]intptr_t would be one step better, if there’s no existing typedef specifically for this purpose. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:50: struct FULL_PEB_LDR_DATA { Since you don’t need Length or Initialized, you can make this struct extend the SDK’s PEB_LDR_DATA. And if you did need Length or Initialized, you could use a union. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:54: LIST_ENTRY InLoadOrderModuleList; What’s the difference between load order and initialization order? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:74: decltype(NtQueryInformationProcess)* nt_query_information_process = I like to wrap these kinds of things in their own functions, and ideally store the function pointer in a static (because it’s not going to change during the process’ lifetime). Then you could just call it naturally, either relying on namespacing to get your version of the function, or giving it a slightly altered name like CP_NtQueryInformationProcess(). Same for IsWow64Process(). https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:90: LOG(ERROR) << "NtQueryInformationProcess"; If status < 0, the value of status might be a good thing to include in the message, like how we use PLOG for things that set something that GetLastError() would return. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:96: if (!ReadStruct(process, process_basic_information_.PebBaseAddress, &peb)) This looks like the first point where cross-bitted operation might fail. If we’re not ready to do cross-bitted yet (and if we won’t ever be able to do 32-reads-64), we should detect those cases early and fail fast. As things are, the PEB structure is word size-dependent, and 64-reads-32 will begin interpreting things incorrectly from this point on without something like process_types. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:117: for (;;) { You could hoist some stuff into the for construct to make the loop’s behavior a little more obvious up here: for (LIST_ENTRY* cur = …; cur != last; cur = …->Flink) or even use the comma operator to initialize last in there too. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:123: if (!ReadStruct(process, Your CL description had me geared up for a truly heinous hack. This isn’t that bad. The offset is a little weird, but you’ve gotta do what you’ve gotta do, and that’s about all. Whew! https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:125: offsetof(LDR_DATA_TABLE_ENTRY, Reserved2), I see. Presumably, LDR_DATA_TABLE_ENTRY really starts off like this: { LIST_ENTRY InLoadOrderLinks; // PVOID Reserved1[2] LIST_ENTRY InMemoryOrderLinks; LIST_ENTRY InInitializationOrderLinks; // PVOID Reserved2[2] // … } and that’s why you’re using Reserved2. If that’s so, I think this code would be more self-explanatory if you declared your own FULL_LDR_DATA_TABLE_ENTRY structure to give proper access to InInitializationOrderLinks by name, similar to how you use FULL_PEB_LDR_DATA to give access to InInitializationOrderModuleList. This would also make it more natural to access the Flink to advance to the next iteration, no funky cast required. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:127: return false; {} around this, its controlling condition is so long and ragged that the braces help see that this is the controlled branch much more easily. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:132: return false; A bad module should probably invalidate that module and but almost definitely not all process reading altogether. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:139: decltype(IsWow64Process)* is_wow64_process = If you break this one into its own function, you can give it a better signature like “bool replacementfunction(HANDLE)”—the function itself would wrap the PLOG and just return false for any failure or can’t-be-WoW64 case. In this class, is_wow_64_ could become a bool instead of a BOOL. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:184: return reinterpret_cast<ULONG_PTR>(process_basic_information_.Reserved3); Can process_basic_information_ be a union type that makes a ParentProcessId field available (with the correct type) at the same location as Reserved3? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:48: bool IsWow64() const; IsWoW64() (two capital double-yous)? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:63: //! Process Environment Block. Will the main executable always appear at index 0? Or would that be load order? Knowing which module is the main executable might be more useful, but maybe there’s a different way to work that out that doesn’t need to be part of this class. We stick the main executable at module index 0 in Mac minidumps. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:70: bool is64bit_; is_64_bit_, is_wow64_ https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:74: DISALLOW_COPY_AND_ASSIGN(ProcessInfo); #include "base/basictypes.h" https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... File util/win/process_info_test.cc (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:1: // Copyright 2014 The Crashpad Authors. All rights reserved. 2015 (I’ve been goofing this up a lot too, a PRESUBMIT checking new files could help). https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:28: void TestSelfProcess(const ProcessInfo& process_info) { Why don’t you just put this in-line into the TEST() below? It’s not used anywhere else, and the TEST() is really short. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:32: #if defined(ARCH_CPU_64_BITS) #include "build/build_config.h" https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:33: EXPECT_TRUE(process_info.Is64Bit()); You can check that WoW64 is false here too. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:42: EXPECT_EQ(std::wstring(GetCommandLineW()), command_line); Can’t you just use GetCommandLine() and let the SDK macros give you the right variant? Or is UNICODE not defined here? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:47: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); Not kosher unless you’ve checked that the size >= 9. Also, wcslen(kNtdllName) would be better than 9. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:47: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); Will the module name that shows up here always be a pathname? Will it always have \ separators? If so, you can look for L"\\ntdll.dll". https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:76: std::wstring command_line = child_test_executable + L" " + Ideally we’d have a utility to properly escape command lines given arguments, but for now, a TODO will suffice, and this will be fine as long as there are no spaces or other bogons in the path. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:99: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); The same comments about wcslen() and checking the size apply here. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:99: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); Do you want to look for the main executable as well? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:100: EXPECT_EQ(L"lz32.dll", modules.back().substr(modules.back().size() - 8)); This was a surprise until I read the test file, but once I did, I realized what a good idea this is. Can you just add a comment here saying that it’s a weird module that the test executable loads? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:104: CloseHandle(pi.hThread); Scopers? There are ASSERTs in here that translate to early returns and thus leaks. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... File util/win/process_info_test_child.cc (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test_child.cc:18: // A simple binary to be loaded an inspected by ProcessInfo. an→and https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test_child.cc:20: // Get handles to the events we use to communicate with our parent. if (argc != 3) abort();
Patchset #4 (id:100001) has been deleted
Thanks! https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:21: std::wstring ReadUnicodeString(HANDLE process, const UNICODE_STRING& us) { On 2015/03/05 17:32:22, Mark Mentovai wrote: > In returning, this should have a way to distinguish between an error in > ReadProcessMemory() and a successful read of an empty string. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:24: str.resize(us.Length / sizeof(wchar_t)); On 2015/03/05 17:32:22, Mark Mentovai wrote: > DCHECK_EQ(us.Length % sizeof(wchar_t), 0) Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:26: if (!ReadProcessMemory( On 2015/03/05 17:32:22, Mark Mentovai wrote: > If I’m reading this correctly, for cross-bitted operation, a 32-bit process > can’t ReadProcessMemory() from a 64-bit process because us.Buffer is a native > (32-bit) pointer. A 64-bit process would be able to ReadProcessMemory() from a > 32-bit process, though. Does that sound right? I haven't tested it yet, but yes, I believe that's correct. In theory it would probably be possible to escape out of WOW64 and call the x64 ReadProcessMemory from the x86-on-wow64 process. Hopefully for our purposes we can simply provide only an x64 binary that handles reading from both 32 and 64. (Which reminds me I should change the default gyp configuration here.) https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:29: PLOG(ERROR) << "ReadProcessMemory UNICODE_STRING"; On 2015/03/05 17:32:22, Mark Mentovai wrote: > #include "base/logging.h". Also, PLOG is incorrect if this line was reached > because bytes_read != us.Length. Same on line 42. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:36: template <class T> bool ReadStruct(HANDLE process, void* at, T* into) { On 2015/03/05 17:32:22, Mark Mentovai wrote: > const void* at, but > > Using pointer types to refer to things in a different process’ address space is > hairy, because it’s easy to mistake it for a pointer to the local address space > and accidentally dereference it. [u]intptr_t would be one step better, if > there’s no existing typedef specifically for this purpose. Switched to uintptr_t. ReadProcessMemory just uses const void* for remote address spaces (and yes! I did have one of these incorrect until I tested reading from not-self-process). It makes the callsites a bit uglier as they're all void* in PEB structures, but we'll need our own definitions of those later. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:50: struct FULL_PEB_LDR_DATA { On 2015/03/05 17:32:22, Mark Mentovai wrote: > Since you don’t need Length or Initialized, you can make this struct extend the > SDK’s PEB_LDR_DATA. And if you did need Length or Initialized, you could use a > union. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:54: LIST_ENTRY InLoadOrderModuleList; On 2015/03/05 17:32:22, Mark Mentovai wrote: > What’s the difference between load order and initialization order? Load and Memory order seem to always be the same (perhaps they differed on older OS's with truly-shared DLLs?). Initialization order differs in that it's bottom-up, rather than top-down. If we load A, and A imports B, then, the load and memory order would be [A, B], whereas the initialization order would be [B, A] because the initializers of B need to run before A's. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:74: decltype(NtQueryInformationProcess)* nt_query_information_process = On 2015/03/05 17:32:22, Mark Mentovai wrote: > I like to wrap these kinds of things in their own functions, and ideally store > the function pointer in a static (because it’s not going to change during the > process’ lifetime). Then you could just call it naturally, either relying on > namespacing to get your version of the function, or giving it a slightly altered > name like CP_NtQueryInformationProcess(). Same for IsWow64Process(). Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:90: LOG(ERROR) << "NtQueryInformationProcess"; On 2015/03/05 17:32:22, Mark Mentovai wrote: > If status < 0, the value of status might be a good thing to include in the > message, like how we use PLOG for things that set something that GetLastError() > would return. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:96: if (!ReadStruct(process, process_basic_information_.PebBaseAddress, &peb)) On 2015/03/05 17:32:22, Mark Mentovai wrote: > This looks like the first point where cross-bitted operation might fail. If > we’re not ready to do cross-bitted yet (and if we won’t ever be able to do > 32-reads-64), we should detect those cases early and fail fast. As things are, > the PEB structure is word size-dependent, and 64-reads-32 will begin > interpreting things incorrectly from this point on without something like > process_types. Right, good point. For now I've moved the 32/64 check above here to early out when we don't match. As follow up I'll switch our default build to x64, then start testing x64-reading-x86. I think process_types makes sense at the cost of redefining these SDK structures, but we've already had to make our local versions of them for other reasons anyway. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:117: for (;;) { On 2015/03/05 17:32:21, Mark Mentovai wrote: > You could hoist some stuff into the for construct to make the loop’s behavior a > little more obvious up here: > > for (LIST_ENTRY* cur = …; > cur != last; > cur = …->Flink) > > or even use the comma operator to initialize last in there too. Some hoisted. Unfortunately, it's a "back" pointer, not an "end", so the termination needs to be at the end of the loop, not the top. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:123: if (!ReadStruct(process, On 2015/03/05 17:32:22, Mark Mentovai wrote: > Your CL description had me geared up for a truly heinous hack. This isn’t that > bad. The offset is a little weird, but you’ve gotta do what you’ve gotta do, and > that’s about all. Whew! All in the eye of the beholder, I suppose. :) https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:125: offsetof(LDR_DATA_TABLE_ENTRY, Reserved2), On 2015/03/05 17:32:22, Mark Mentovai wrote: > I see. > > Presumably, LDR_DATA_TABLE_ENTRY really starts off like this: > > { > LIST_ENTRY InLoadOrderLinks; // PVOID Reserved1[2] > LIST_ENTRY InMemoryOrderLinks; > LIST_ENTRY InInitializationOrderLinks; // PVOID Reserved2[2] > // … > } > > and that’s why you’re using Reserved2. That's right. > > If that’s so, I think this code would be more self-explanatory if you declared > your own FULL_LDR_DATA_TABLE_ENTRY structure to give proper access to > InInitializationOrderLinks by name, similar to how you use FULL_PEB_LDR_DATA to > give access to InInitializationOrderModuleList. > > This would also make it more natural to access the Flink to advance to the next > iteration, no funky cast required. Switched to FULL_LDR_DATA_TABLE_ENTRY. As you allude to, we'll need our own anyway for cross-bitness in the future. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:127: return false; On 2015/03/05 17:32:22, Mark Mentovai wrote: > {} around this, its controlling condition is so long and ragged that the braces > help see that this is the controlled branch much more easily. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:132: return false; On 2015/03/05 17:32:22, Mark Mentovai wrote: > A bad module should probably invalidate that module and but almost definitely > not all process reading altogether. Done. I was concerned that a bad PEB meant the world was on fire, but returning an incomplete/incorrect modules list seems preferable. I similarly changed the link reading to not be entirely fatal. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:139: decltype(IsWow64Process)* is_wow64_process = On 2015/03/05 17:32:22, Mark Mentovai wrote: > If you break this one into its own function, you can give it a better signature > like “bool replacementfunction(HANDLE)”—the function itself would wrap the PLOG > and just return false for any failure or can’t-be-WoW64 case. In this class, > is_wow_64_ could become a bool instead of a BOOL. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.cc... util/win/process_info.cc:184: return reinterpret_cast<ULONG_PTR>(process_basic_information_.Reserved3); On 2015/03/05 17:32:22, Mark Mentovai wrote: > Can process_basic_information_ be a union type that makes a ParentProcessId > field available (with the correct type) at the same location as Reserved3? Added FULL_... for this type too. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:48: bool IsWow64() const; On 2015/03/05 17:32:23, Mark Mentovai wrote: > IsWoW64() (two capital double-yous)? I prefer not, but am happy to change it if you like that way better. (The Win32 API functions spell it "Wow64".) https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:63: //! Process Environment Block. On 2015/03/05 17:32:23, Mark Mentovai wrote: > Will the main executable always appear at index 0? Or would that be load order? > > Knowing which module is the main executable might be more useful, but maybe > there’s a different way to work that out that doesn’t need to be part of this > class. We stick the main executable at module index 0 in Mac minidumps. Unfortunately, one drawback of InitializationOrder is that the main executable doesn't appear in the list. In MemoryOrder it would always be at index 0. Perhaps we should prepend it to this list to make that true here too? https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:70: bool is64bit_; On 2015/03/05 17:32:23, Mark Mentovai wrote: > is_64_bit_, is_wow64_ Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:74: DISALLOW_COPY_AND_ASSIGN(ProcessInfo); On 2015/03/05 17:32:23, Mark Mentovai wrote: > #include "base/basictypes.h" Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... File util/win/process_info_test.cc (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:1: // Copyright 2014 The Crashpad Authors. All rights reserved. On 2015/03/05 17:32:23, Mark Mentovai wrote: > 2015 (I’ve been goofing this up a lot too, a PRESUBMIT checking new files could > help). Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:28: void TestSelfProcess(const ProcessInfo& process_info) { On 2015/03/05 17:32:23, Mark Mentovai wrote: > Why don’t you just put this in-line into the TEST() below? It’s not used > anywhere else, and the TEST() is really short. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:32: #if defined(ARCH_CPU_64_BITS) On 2015/03/05 17:32:23, Mark Mentovai wrote: > #include "build/build_config.h" Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:33: EXPECT_TRUE(process_info.Is64Bit()); On 2015/03/05 17:32:23, Mark Mentovai wrote: > You can check that WoW64 is false here too. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:42: EXPECT_EQ(std::wstring(GetCommandLineW()), command_line); On 2015/03/05 17:32:23, Mark Mentovai wrote: > Can’t you just use GetCommandLine() and let the SDK macros give you the right > variant? Or is UNICODE not defined here? Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:47: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); On 2015/03/05 17:32:23, Mark Mentovai wrote: > Will the module name that shows up here always be a pathname? Will it always > have \ separators? If so, you can look for L"\\ntdll.dll". Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:47: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); On 2015/03/05 17:32:23, Mark Mentovai wrote: > Not kosher unless you’ve checked that the size >= 9. Also, wcslen(kNtdllName) > would be better than 9. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:76: std::wstring command_line = child_test_executable + L" " + On 2015/03/05 17:32:23, Mark Mentovai wrote: > Ideally we’d have a utility to properly escape command lines given arguments, > but for now, a TODO will suffice, and this will be fine as long as there are no > spaces or other bogons in the path. TODO for now. At the moment it's just the two guids, so it should be fine in this code. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:99: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); On 2015/03/05 17:32:23, Mark Mentovai wrote: > Do you want to look for the main executable as well? The main executable doesn't appear in the list at the moment. I guess inserting it at 0, and checking for both that, and ntdll at 1 might makes sense. Alternatively we could make the main module a separate field, but maybe that will just be an annoying difference when compared with Mac. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:99: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); On 2015/03/05 17:32:23, Mark Mentovai wrote: > The same comments about wcslen() and checking the size apply here. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:100: EXPECT_EQ(L"lz32.dll", modules.back().substr(modules.back().size() - 8)); On 2015/03/05 17:32:23, Mark Mentovai wrote: > This was a surprise until I read the test file, but once I did, I realized what > a good idea this is. Can you just add a comment here saying that it’s a weird > module that the test executable loads? Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:104: CloseHandle(pi.hThread); On 2015/03/05 17:32:23, Mark Mentovai wrote: > Scopers? There are ASSERTs in here that translate to early returns and thus > leaks. Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... File util/win/process_info_test_child.cc (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test_child.cc:18: // A simple binary to be loaded an inspected by ProcessInfo. On 2015/03/05 17:32:23, Mark Mentovai wrote: > an→and Done. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test_child.cc:20: // Get handles to the events we use to communicate with our parent. On 2015/03/05 17:32:23, Mark Mentovai wrote: > if (argc != 3) > abort(); Done.
https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:48: bool IsWow64() const; scottmg wrote: > On 2015/03/05 17:32:23, Mark Mentovai wrote: > > IsWoW64() (two capital double-yous)? > > I prefer not, but am happy to change it if you like that way better. (The Win32 > API functions spell it "Wow64".) OK, if that’s what they consistently call it. I checked Wikipedia, which called it WoW64—wouldn’t be the first time Wikipedia led us astray. ;) https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:63: //! Process Environment Block. scottmg wrote: > On 2015/03/05 17:32:23, Mark Mentovai wrote: > > Will the main executable always appear at index 0? Or would that be load > order? > > > > Knowing which module is the main executable might be more useful, but maybe > > there’s a different way to work that out that doesn’t need to be part of this > > class. We stick the main executable at module index 0 in Mac minidumps. > > Unfortunately, one drawback of InitializationOrder is that the main executable > doesn't appear in the list. In MemoryOrder it would always be at index 0. > > Perhaps we should prepend it to this list to make that true here too? That aspect of the design is up to you. Yes, it’d be nice for it to show up in the minidump at module index 0, but there are other ways to make that happen outside of this class, and you get to choose how you want to achieve it. For example, on Mac, I deal with this in ProcessReader::InitializeModules(), which is in the snapshot layer. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... File util/win/process_info_test.cc (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info_te... util/win/process_info_test.cc:99: EXPECT_EQ(L"ntdll.dll", modules[0].substr(modules[0].size() - 9)); scottmg wrote: > On 2015/03/05 17:32:23, Mark Mentovai wrote: > > Do you want to look for the main executable as well? > > The main executable doesn't appear in the list at the moment. I guess inserting > it at 0, and checking for both that, and ntdll at 1 might makes sense. See previous comments. If you do decide that this interface should expose the main executable at index 0, it’s easy to test. Otherwise, you should still test that it shows up somewhere in the list. You could do this with a loop over the modules, or I’m sure you could cook up a crafty lambda to do it more concisely and slash or obtusely. > Alternatively we could make the main module a separate field, but maybe that > will just be an annoying difference when compared with Mac. Well, I imagine that given the TODOs, the modules that this class returns will be more than just strings in the future. Having Module MainModule() and vector<Module> AllModulesExceptMain() seems like kind of a weird interface. But Modules() and MainModuleIndex() might be OK. Up to you, though. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:57: *result = std::wstring(); result->clear() https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:84: PLOG(ERROR) << "ReadProcessMemory: " << __FUNCSIG__; Tiny nit: colons on these messages but not on the UNICODE_STRING ones above. I like without, with is fine too, consistency trumps. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:136: command_line_(), Let’s be explicit about everything: modules_ and initialized_(). https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:168: LOG(ERROR) << "Reading x64 process from x86 process not supported"; The “escape WoW64 and call the native function” is interesting, I might want to know more about that in the future. :) https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:195: return false; {} on these raggedy ifs. The next one too. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:215: LIST_ENTRY* last = peb_ldr_data.InInitializationOrderModuleList.Blink; Can last and cur can be pointers to const data? If they’ve got to be pointers that can be incorrectly dereferenced to the local address space, they should at least forbid modification if possible. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:216: FULL_LDR_DATA_TABLE_ENTRY ldr_data_table_entry; Pull this into the loop body. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:236: if (cur == last) Oh right, I see, yeah, this has to be in the loop. Not the first time I wished for a for loop option to have do-while semantics for the condition. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.h... util/win/process_info.h:31: struct FULL_PROCESS_BASIC_INFORMATION { Put this into namespace internal. Also, add a class-level comment explaining that it’s PROCESS_BASIC_INFORMATION with the reserved fields dealt with correctly. For this and the other FULL_ structs, if there’s a reference (site, book, whatever) that explains the reserved fields, it’d be good to call it out. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.h... util/win/process_info.h:40: class ProcessInfo { Class-level comment? https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... File util/win/process_info_test.cc (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... util/win/process_info_test.cc:54: ASSERT_GE(modules[0].size(), wcslen(kNtdllName)); #include <wchar.h>. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... File util/win/process_info_test_child.cc (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... util/win/process_info_test_child.cc:19: int wmain(int argc, wchar_t** argv) { Technically IWYU would want <wchar.h> but I couldn’t imagine wchar_t ever not being available from the other two #includes on Windows. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... util/win/process_info_test_child.cc:43: return 0; Use EXIT_SUCCESS instead of 0. (You’ve already #included <stdlib.h>.)
https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:63: //! Process Environment Block. On 2015/03/05 21:53:36, Mark Mentovai wrote: > scottmg wrote: > > On 2015/03/05 17:32:23, Mark Mentovai wrote: > > > Will the main executable always appear at index 0? Or would that be load > > order? > > > > > > Knowing which module is the main executable might be more useful, but maybe > > > there’s a different way to work that out that doesn’t need to be part of > this > > > class. We stick the main executable at module index 0 in Mac minidumps. > > > > Unfortunately, one drawback of InitializationOrder is that the main executable > > doesn't appear in the list. In MemoryOrder it would always be at index 0. > > > > Perhaps we should prepend it to this list to make that true here too? > > That aspect of the design is up to you. Yes, it’d be nice for it to show up in > the minidump at module index 0, but there are other ways to make that happen > outside of this class, and you get to choose how you want to achieve it. > > For example, on Mac, I deal with this in ProcessReader::InitializeModules(), > which is in the snapshot layer. Inserting it at index 0 seems simplest for now. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:57: *result = std::wstring(); On 2015/03/05 21:53:36, Mark Mentovai wrote: > result->clear() Done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:84: PLOG(ERROR) << "ReadProcessMemory: " << __FUNCSIG__; On 2015/03/05 21:53:36, Mark Mentovai wrote: > Tiny nit: colons on these messages but not on the UNICODE_STRING ones above. I > like without, with is fine too, consistency trumps. Done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:136: command_line_(), On 2015/03/05 21:53:36, Mark Mentovai wrote: > Let’s be explicit about everything: modules_ and initialized_(). Done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:168: LOG(ERROR) << "Reading x64 process from x86 process not supported"; On 2015/03/05 21:53:36, Mark Mentovai wrote: > The “escape WoW64 and call the native function” is interesting, I might want to > know more about that in the future. :) In this case, the Wow64 ntdll contains the necessary functions directly, e.g. NtWow64ReadVirtualMemory64 and similar. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:195: return false; On 2015/03/05 21:53:36, Mark Mentovai wrote: > {} on these raggedy ifs. The next one too. Oops, done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:215: LIST_ENTRY* last = peb_ldr_data.InInitializationOrderModuleList.Blink; On 2015/03/05 21:53:36, Mark Mentovai wrote: > Can last and cur can be pointers to const data? If they’ve got to be pointers > that can be incorrectly dereferenced to the local address space, they should at > least forbid modification if possible. Done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.c... util/win/process_info.cc:216: FULL_LDR_DATA_TABLE_ENTRY ldr_data_table_entry; On 2015/03/05 21:53:36, Mark Mentovai wrote: > Pull this into the loop body. Can't, it's used in the loop-expression. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.h... util/win/process_info.h:31: struct FULL_PROCESS_BASIC_INFORMATION { On 2015/03/05 21:53:36, Mark Mentovai wrote: > Put this into namespace internal. Also, add a class-level comment explaining > that it’s PROCESS_BASIC_INFORMATION with the reserved fields dealt with > correctly. Done. > For this and the other FULL_ structs, if there’s a reference (site, book, > whatever) that explains the reserved fields, it’d be good to call it out. This one I got them from chrome's sandbox which doesn't contain any citations. This one is from ntddk.h. There are some books that document them, but realistically the references all probably trace back to inspection in windbg i.e. from the public ntdll.dll pdbs. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info.h... util/win/process_info.h:40: class ProcessInfo { On 2015/03/05 21:53:36, Mark Mentovai wrote: > Class-level comment? Done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... File util/win/process_info_test.cc (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... util/win/process_info_test.cc:54: ASSERT_GE(modules[0].size(), wcslen(kNtdllName)); On 2015/03/05 21:53:36, Mark Mentovai wrote: > #include <wchar.h>. Done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... File util/win/process_info_test_child.cc (right): https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... util/win/process_info_test_child.cc:19: int wmain(int argc, wchar_t** argv) { On 2015/03/05 21:53:37, Mark Mentovai wrote: > Technically IWYU would want <wchar.h> but I couldn’t imagine wchar_t ever not > being available from the other two #includes on Windows. Done. https://codereview.chromium.org/977003003/diff/120001/util/win/process_info_t... util/win/process_info_test_child.cc:43: return 0; On 2015/03/05 21:53:37, Mark Mentovai wrote: > Use EXIT_SUCCESS instead of 0. (You’ve already #included <stdlib.h>.) Done.
Nice, LGTM. https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.c... util/win/process_info.cc:203: // Include the first module in the memory order list to get our own name as Is this true even if you build an executable with a weird (high) preferred load address? https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.h... util/win/process_info.h:46: //! \brief Gathers about a process given its `HANDLE`. This consists primarily Missing a word. https://codereview.chromium.org/977003003/diff/140001/util/win/process_info_t... File util/win/process_info_test.cc (right): https://codereview.chromium.org/977003003/diff/140001/util/win/process_info_t... util/win/process_info_test.cc:88: STARTUPINFO si = {0}; startup_info, process_info. That’ll make the arguments used in the next few calls a little easier to read.
https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.c... util/win/process_info.cc:203: // Include the first module in the memory order list to get our own name as On 2015/03/06 03:40:23, Mark Mentovai wrote: > Is this true even if you build an executable with a weird (high) preferred load > address? Good question! Experimentally, yes, on Win81 at least. It appears to be the order they were put in memory (i.e. load order) which matches what I observed before. I haven't found any documentation that describes the difference between the two other than by basically restating the field names. I modified the test to load the child executable at a high, fixed image base (0x7800'0000), and looking at the order in windbg I get: ... Ldr.InMemoryOrderModuleList: 00223d20 . 00224258 Base TimeStamp Module 78000000 54f9408f Mar 05 21:52:15 2015 D:\src\crashpad\crashpad\out\Debug\util_test_process_info_test_child.exe 77c80000 54b0d74f Jan 09 23:39:59 2015 C:\Windows\SYSTEM32\ntdll.dll 77010000 532a2e6c Mar 19 16:55:24 2014 C:\Windows\SYSTEM32\KERNEL32.DLL 777d0000 53eeb460 Aug 15 18:31:12 2014 C:\Windows\SYSTEM32\KERNELBASE.dll 74090000 53088706 Feb 22 03:16:22 2014 C:\Windows\system32\apphelp.dll ... That is, it's at the specified high address, and other DLLs appear at lower addresses, but it's still first in the list. I've included the linker settings to do that in the patch, so we will at least see a test fail if this assumption turns out to be false. https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/140001/util/win/process_info.h... util/win/process_info.h:46: //! \brief Gathers about a process given its `HANDLE`. This consists primarily On 2015/03/06 03:40:23, Mark Mentovai wrote: > Missing a word. Done. https://codereview.chromium.org/977003003/diff/140001/util/win/process_info_t... File util/win/process_info_test.cc (right): https://codereview.chromium.org/977003003/diff/140001/util/win/process_info_t... util/win/process_info_test.cc:88: STARTUPINFO si = {0}; On 2015/03/06 03:40:24, Mark Mentovai wrote: > startup_info, process_info. That’ll make the arguments used in the next few > calls a little easier to read. si/pi was a hacky way of avoiding conflict with ProcessInfo process_info. I went with startup_info and process_information which is a bit better and matches the struct names, though a bit confusing to have both process_info and process_information.
Message was sent while issue was closed.
Committed patchset #6 (id:160001) manually as bed7a543c058b29bae1f24ab49799ff3d7198c73 (presubmit successful).
Message was sent while issue was closed.
This is a huge step forward! The following is just a small post-facto thought. https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... util/win/process_info.h:63: //! Process Environment Block. scottmg wrote: > Unfortunately, one drawback of InitializationOrder is that the main executable > doesn't appear in the list. In MemoryOrder it would always be at index 0. Since this is kind of quirky, I was thinking that we should test that the main executable really doesn’t show up in the list twice. An easy way to do this is to remember its load address from the first module in the memory-order list and then check that each module in the initialization-order list doesn’t match the main executable’s load address. That should be faster and more robust than filename comparisons or anything else, but since you’re not tracking load addresses yet, maybe it can wait for that.
Message was sent while issue was closed.
On 2015/03/06 14:33:30, Mark Mentovai wrote: > This is a huge step forward! > > The following is just a small post-facto thought. > > https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h > File util/win/process_info.h (right): > > https://codereview.chromium.org/977003003/diff/80001/util/win/process_info.h#... > util/win/process_info.h:63: //! Process Environment Block. > scottmg wrote: > > Unfortunately, one drawback of InitializationOrder is that the main executable > > doesn't appear in the list. In MemoryOrder it would always be at index 0. > > Since this is kind of quirky, I was thinking that we should test that the main > executable really doesn’t show up in the list twice. An easy way to do this is > to remember its load address from the first module in the memory-order list and > then check that each module in the initialization-order list doesn’t match the > main executable’s load address. That should be faster and more robust than > filename comparisons or anything else, but since you’re not tracking load > addresses yet, maybe it can wait for that. Sounds good. I'm going to switch to x64, then get back at the snapshot level. Once ProcessInfo gathers more module information I'll bulk up the tests. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
