Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1400413002:
win: Add Handles() to ProcessInfo (Closed)
|
Created:
5 years, 2 months ago by scottmg Modified:
5 years, 2 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 Handles() to ProcessInfo
To eventually be used to fill out MINIDUMP_HANDLE_DESCRIPTOR.
R=mark@chromium.org
BUG=crashpad:21, crashpad:46, crashpad:52
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/7de04b02f85ddfeee778b71f804d4d30c971ccd8
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : test #
Total comments: 33
Patch Set 5 : . #
Total comments: 42
Patch Set 6 : fixes #
Total comments: 11
Patch Set 7 : . #
Total comments: 2
Patch Set 8 : . #Patch Set 9 : rebase #
Messages
Total messages: 16 (4 generated)
https://codereview.chromium.org/1400413002/diff/60001/util/win/nt_internals.h File util/win/nt_internals.h (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/nt_internals.h... util/win/nt_internals.h:31: enum { SystemExtendedHandleInformation = 64 }; Tested on Windows XP yet? https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:152: } } else if (!NT_SUCCESS(status)) { NTSTATUS_LOG(ERROR, status) << "NtQueryObject"; https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:355: status = crashpad::NtQuerySystemInformation( Do you want a QuerySystemInformation() wrapper for this to handle the resizing, kind of like QueryObject()? https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:363: buffer_size *= 2; Why don’t you use the ReturnLength argument to tell you what to resize to, as in your QueryObject() wrapper? https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:375: Traits>* system_handle_information_ex = Weird formatting (did clang-format do this?!) https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:388: if (!DuplicateHandle(process, In https://code.google.com/p/crashpad/issues/detail?id=21#c2, you claimed that DuplicateHandle() to dupe handles into our process was scary. It certainly registers on my heebie-jeebie-meter too, but I’m not sure if that’s me being overly paranoid or if there’s an actual risk. But maybe it’s fine to just use with NtQueryObject() as you’re doing here. Can you clarify what you thought was scary? You also said that you thought you’d start with option 1, no type and no name, but here you’re grabbing the type. Did something change in the analysis? https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:396: // EtwRegistration, so we simply drop these. (We also do not strictly know Does MiniDumpWriteDump() lose these too? Is there an error code that we can key this off? https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:428: object_type_information->TypeName.Length / DCHECK that Length % sizeof(result_handle.type_name[0]) is 0. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:431: // The Attributes and GrantedAccess sometimes differ slightly between the Blank before. Same on 439. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:440: DCHECK_GT(object_basic_information->HandleCount, 0u); Should this be > 1? At least one original reference, and one for the duplicate in here? https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:441: result_handle.handle_count = object_basic_information->HandleCount - 1; Happy to see this accounted for! https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:455: handle(), Zeroes in the integer fields. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:473: is_64_bit_(false), handles_() https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h... util/win/process_info.h:61: uint32_t handle; This is at least as good a place for the comment about only 32 bits being significant for handle values as process_info.cc, if not better. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h... util/win/process_info.h:67: //! \brief The ACCESS_MASK for the handle in this process. Ref: `ACCESS_MASK` https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h... util/win/process_info.h:68: //! http://blogs.msdn.com/b/openspecification/archive/2010/04/01/about-the-access... The URL reference shouldn’t be part of the \brief. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info_t... File util/win/process_info_test.cc (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info_t... util/win/process_info_test.cc:565: EXPECT_EQ(1, handle.handle_count); Any way to reason about attributes and access here? If the answer is to make another NtQueryObject() call, it’s not worth it, but if there’s something that we know about the handles, even if it’s just along the lines of testing that (attributes & some_bits) == some_bits, we should do it.
Patchset #5 (id:80001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:140001) has been deleted
Thanks for the quick review! https://codereview.chromium.org/1400413002/diff/60001/util/win/nt_internals.h File util/win/nt_internals.h (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/nt_internals.h... util/win/nt_internals.h:31: enum { SystemExtendedHandleInformation = 64 }; On 2015/10/14 22:49:55, Mark Mentovai wrote: > Tested on Windows XP yet? Yes, as far as I know it's XP+. (And the test works on XP locally.) https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:152: } On 2015/10/14 22:49:55, Mark Mentovai wrote: > } else if (!NT_SUCCESS(status)) { > NTSTATUS_LOG(ERROR, status) << "NtQueryObject"; Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:355: status = crashpad::NtQuerySystemInformation( On 2015/10/14 22:49:55, Mark Mentovai wrote: > Do you want a QuerySystemInformation() wrapper for this to handle the resizing, > kind of like QueryObject()? It's only done once at the moment, so I think it's OK to keep it inline. I could be convinced either way though. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:363: buffer_size *= 2; On 2015/10/14 22:49:55, Mark Mentovai wrote: > Why don’t you use the ReturnLength argument to tell you what to resize to, as in > your QueryObject() wrapper? That's the comment above the loop -- it doesn't properly return the expected length on some OSs unfortunately, see e.g. http://forum.sysinternals.com/topic18892.html at the end of Step 1. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:375: Traits>* system_handle_information_ex = On 2015/10/14 22:49:55, Mark Mentovai wrote: > Weird formatting (did clang-format do this?!) Yup. A little better with an auto*? https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:388: if (!DuplicateHandle(process, On 2015/10/14 22:49:55, Mark Mentovai wrote: > In https://code.google.com/p/crashpad/issues/detail?id=21#c2, you claimed that > DuplicateHandle() to dupe handles into our process was scary. It certainly > registers on my heebie-jeebie-meter too, but I’m not sure if that’s me being > overly paranoid or if there’s an actual risk. But maybe it’s fine to just use > with NtQueryObject() as you’re doing here. Can you clarify what you thought was > scary? > > You also said that you thought you’d start with option 1, no type and no name, > but here you’re grabbing the type. Did something change in the analysis? DuplicateHandle()ing into ourselves scared me at first, but after talking to Justin I was less nervous about that. For "Level #3" I'm definitely a little more nervous about the NtQueryObject calls with obscure/undocumented values as people have clearly noted hangs on some OSs. But I (and more importantly Justin) think it's OK to only do the duplicate and the limited information query. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:396: // EtwRegistration, so we simply drop these. (We also do not strictly know On 2015/10/14 22:49:55, Mark Mentovai wrote: > Does MiniDumpWriteDump() lose these too? > > Is there an error code that we can key this off? Just confirmed with MiniDumpWriteDump() via https://codereview.chromium.org/1405443006 that it doesn't have any information for them (i.e. can't DuplicateHandle() them either, I guess), but it does at least include them in the list of handles with no additional information, which is better. Switched to that on (any) duplicate failure. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:428: object_type_information->TypeName.Length / On 2015/10/14 22:49:55, Mark Mentovai wrote: > DCHECK that Length % sizeof(result_handle.type_name[0]) is 0. Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:431: // The Attributes and GrantedAccess sometimes differ slightly between the On 2015/10/14 22:49:55, Mark Mentovai wrote: > Blank before. Same on 439. Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:440: DCHECK_GT(object_basic_information->HandleCount, 0u); On 2015/10/14 22:49:55, Mark Mentovai wrote: > Should this be > 1? At least one original reference, and one for the duplicate > in here? Yeah, done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:455: handle(), On 2015/10/14 22:49:55, Mark Mentovai wrote: > Zeroes in the integer fields. Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.c... util/win/process_info.cc:473: is_64_bit_(false), On 2015/10/14 22:49:55, Mark Mentovai wrote: > handles_() Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h... util/win/process_info.h:61: uint32_t handle; On 2015/10/14 22:49:55, Mark Mentovai wrote: > This is at least as good a place for the comment about only 32 bits being > significant for handle values as process_info.cc, if not better. Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h... util/win/process_info.h:67: //! \brief The ACCESS_MASK for the handle in this process. Ref: On 2015/10/14 22:49:55, Mark Mentovai wrote: > `ACCESS_MASK` Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info.h... util/win/process_info.h:68: //! http://blogs.msdn.com/b/openspecification/archive/2010/04/01/about-the-access... On 2015/10/14 22:49:55, Mark Mentovai wrote: > The URL reference shouldn’t be part of the \brief. Done. https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info_t... File util/win/process_info_test.cc (right): https://codereview.chromium.org/1400413002/diff/60001/util/win/process_info_t... util/win/process_info_test.cc:565: EXPECT_EQ(1, handle.handle_count); On 2015/10/14 22:49:55, Mark Mentovai wrote: > Any way to reason about attributes and access here? If the answer is to make > another NtQueryObject() call, it’s not worth it, but if there’s something that > we know about the handles, even if it’s just along the lines of testing that > (attributes & some_bits) == some_bits, we should do it. attributes is almost always 0, but added some checks for granted_access.
https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:143: DCHECK_GE(return_length, size); DCHECK_GT, not DCHECK_GE. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:155: return buffer.Pass(); buffer.resize(return_length) to trim any excess. (The hint may have been big.) https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:348: ULONG buffer_size = 2 * 1024 * 1024; Pretty huge buffer. At its minimum size, it’s room for over 52,000 handles when queried from a 64-bit process. What are we expecting to see here? https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:352: // return the correct size in the final argument, but it does not in for this “in for this” → “for” https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:353: // SYSTEM_INFORMATION_CLASS, so we loop and attempt larger sizes. Is the returned size correct when this returns success? If not, there’s no need to take any action. If it is, resize the buffer precisely after a successful call. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:363: } else if (status == STATUS_INFO_LENGTH_MISMATCH) { If status is not this and not success, I don’t think we need to bother retrying with the same size buffer. So: if (status != STATUS_INFO_LENGTH_MISMATCH) break; // resize buffer https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:375: const auto* system_handle_information_ex = Could be & instead of *. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:376: reinterpret_cast<process_types::SYSTEM_HANDLE_INFORMATION_EX<Traits>*>( Since this is system-specific and not process-specific, does SYSTEM_HANDLE_INFORMATION_EX need to be traits-ed? Won’t NtQuerySystemInformation() return something in this process’ bitness? We ought to be able to get away with declaring the pair of structs more naturally. Then this whole function wouldn’t need to be traits-ed either. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:381: for (size_t i = 0; i < system_handle_information_ex->NumberOfHandles; ++i) { Before entering the loop, make sure that offsetof(system_handle_information_ex, Handles) + system_handle_information_ex->NumberOfHandles * sizeof(system_handle_information_ex->Handles[0]) <= buffer.size(). https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:383: if (handle.UniqueProcessId != process_id_) Noticing that handle.UniqueProcessId might be 64 bits while process_id_ is 32. I’m assuming that the high 32 bits of UniqueProcessId will always be clear, and they just designed the struct that way for alignment/packing. (Then again, the could have made UniqueProcessId and HandleValue both 32 bits, and it’d pack just fine that way too.) https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:412: continue; Worthwhile to add the data we could get from SYSTEM_HANDLE_TABLE_ENTRY_INFO_EX here? Maybe this ought to always populate as much as possible, never skipping a handle found in the process: Handle result_handle; result_handle.handle = static_cast<uint32_t>(handle.HandleValue); result_handle.attributes = handle.HandleAttributes; result_handle.granted_access = handle.GrantedAccess; if (DuplicateHandle(…)) { ScopedKernelHandle scoped_dup_handle(dup_handle); scoped_ptr<uint8_t[]> buffer = QueryObject(…); if (buffer) { result_handle.pointer_count = object_basic_information->PointerCount; result_handle.handle_count = object_basic_information->HandleCount - 1; } buffer.reset(); // because you don’t need both at the same time buffer.reset(QueryObject(…)); if (buffer) { result_handle.type_name = std::wstring(…); } } handles.push_back(result_handle); https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:428: Handle result_handle; Didn’t you find that MSVS 2015 didn’t like shadowing variables? (See line 398.) If so, you could just declare this before the DuplicateHandle(). https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:438: // The Attributes and GrantedAccess sometimes differ slightly between the Blank before. And also a blank before pointer_count, since this comment doesn’t apply to that. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:445: result_handle.pointer_count = object_basic_information->PointerCount; Just confirming: making a duplicate doesn’t actually affect this number at all, right? https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:450: handles.push_back(result_handle); Blank before this, too. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:542: handles_ = BuildHandleVector<process_types::internal::Traits64>(process); Since this can’t do anything but get all handles system-wide, and because we may not always want to get handles (like if we ever want to write a handle-stream-less minidump), it makes some sense to do this lazily in Handles(), as opposed to here in Initialize(). https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.h:144: const std::vector<Handle>& Handles() const { return handles_; } All of the other getters aren’t inline so they can also do INITIALIZATION_STATE_DCHECK_VALID. This should be consistent with them. Also see the comment I left in the Initialize() implementation. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... File util/win/process_info_test.cc (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... util/win/process_info_test.cc:568: EXPECT_EQ(0, handle.attributes); Can we reason about pointer_count for any of these? == 1 or at the very least >= 1? https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... util/win/process_info_test.cc:568: EXPECT_EQ(0, handle.attributes); Not critical, but can we at least conjure up one HANDLE that has nonzero attributes? Something inheritable, maybe? I don’t want all of the checks we have for that field to test for what also happens to be its default value.
https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:155: return buffer.Pass(); Mark Mentovai wrote: > buffer.resize(return_length) to trim any excess. (The hint may have been big.) Forget this comment and any other comment where I might have confused scoped_ptr<uint8_t[]> with vector<uint8_t> and suggested resizing a region created with new uint8_t[]. Don’t trim those. However, I still think it’s useful to make return_length available to the caller so that it can protect against accesses outside of the buffer.
https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:155: return buffer.Pass(); I wrote: > Forget this comment and any other comment where I might have confused > scoped_ptr<uint8_t[]> with vector<uint8_t> and suggested resizing a region > created with new uint8_t[]. Don’t trim those. However, I still think it’s useful > to make return_length available to the caller so that it can protect against > accesses outside of the buffer. On the third thought (sorry!): Since this information is coming from the OS which we should be able to trust, these checks can be quick debug-only affairs. And since we’re treating |size| not only as a hint but also a minimum size given the existing DCHECK on line 143, we can simply add another DCHECK right here, and not have to worry about getting the size back to the caller. This function’s comment should be updated to reflect size’s use as a minimum, or maybe the argument should even be renamed. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:381: for (size_t i = 0; i < system_handle_information_ex->NumberOfHandles; ++i) { I wrote: > Before entering the loop, make sure that offsetof(system_handle_information_ex, > Handles) + system_handle_information_ex->NumberOfHandles * > sizeof(system_handle_information_ex->Handles[0]) <= buffer.size(). By the same logic, this can be expressed in a DCHECK too. You’d use the returned size instead of buffer.size() if the returned size is correct when NtQuerySystemInformation() is successful.
Thanks. (I hope I didn't miss anything in the revisions.) https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:143: DCHECK_GE(return_length, size); On 2015/10/15 05:25:17, Mark Mentovai wrote: > DCHECK_GT, not DCHECK_GE. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:155: return buffer.Pass(); On 2015/10/15 15:28:53, Mark Mentovai wrote: > I wrote: > > Forget this comment and any other comment where I might have confused > > scoped_ptr<uint8_t[]> with vector<uint8_t> and suggested resizing a region > > created with new uint8_t[]. Don’t trim those. However, I still think it’s > useful > > to make return_length available to the caller so that it can protect against > > accesses outside of the buffer. > > On the third thought (sorry!): > > Since this information is coming from the OS which we should be able to trust, > these checks can be quick debug-only affairs. And since we’re treating |size| > not only as a hint but also a minimum size given the existing DCHECK on line > 143, we can simply add another DCHECK right here, and not have to worry about > getting the size back to the caller. This function’s comment should be updated > to reflect size’s use as a minimum, or maybe the argument should even be > renamed. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:348: ULONG buffer_size = 2 * 1024 * 1024; On 2015/10/15 05:25:17, Mark Mentovai wrote: > Pretty huge buffer. At its minimum size, it’s room for over 52,000 handles when > queried from a 64-bit process. What are we expecting to see here? Unfortunately yes. I have ~110k handles on my system at the moment. There's no API to retrieve the handles for only the process we care about, so the number retrieved can be quite large. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:352: // return the correct size in the final argument, but it does not in for this On 2015/10/15 05:25:17, Mark Mentovai wrote: > “in for this” → “for” Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:353: // SYSTEM_INFORMATION_CLASS, so we loop and attempt larger sizes. On 2015/10/15 05:25:17, Mark Mentovai wrote: > Is the returned size correct when this returns success? If not, there’s no need > to take any action. If it is, resize the buffer precisely after a successful > call. Not doing anything here per vector/scoped_ptr. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:363: } else if (status == STATUS_INFO_LENGTH_MISMATCH) { On 2015/10/15 05:25:17, Mark Mentovai wrote: > If status is not this and not success, I don’t think we need to bother retrying > with the same size buffer. So: > > if (status != STATUS_INFO_LENGTH_MISMATCH) > break; > // resize buffer Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:375: const auto* system_handle_information_ex = On 2015/10/15 05:25:17, Mark Mentovai wrote: > Could be & instead of *. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:376: reinterpret_cast<process_types::SYSTEM_HANDLE_INFORMATION_EX<Traits>*>( On 2015/10/15 05:25:17, Mark Mentovai wrote: > Since this is system-specific and not process-specific, does > SYSTEM_HANDLE_INFORMATION_EX need to be traits-ed? Won’t > NtQuerySystemInformation() return something in this process’ bitness? We ought > to be able to get away with declaring the pair of structs more naturally. Then > this whole function wouldn’t need to be traits-ed either. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:381: for (size_t i = 0; i < system_handle_information_ex->NumberOfHandles; ++i) { On 2015/10/15 15:28:53, Mark Mentovai wrote: > I wrote: > > Before entering the loop, make sure that > offsetof(system_handle_information_ex, > > Handles) + system_handle_information_ex->NumberOfHandles * > > sizeof(system_handle_information_ex->Handles[0]) <= buffer.size(). > > By the same logic, this can be expressed in a DCHECK too. You’d use the returned > size instead of buffer.size() if the returned size is correct when > NtQuerySystemInformation() is successful. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:383: if (handle.UniqueProcessId != process_id_) On 2015/10/15 05:25:16, Mark Mentovai wrote: > Noticing that handle.UniqueProcessId might be 64 bits while process_id_ is 32. > I’m assuming that the high 32 bits of UniqueProcessId will always be clear, and > they just designed the struct that way for alignment/packing. (Then again, the > could have made UniqueProcessId and HandleValue both 32 bits, and it’d pack just > fine that way too.) Yeah, good point, it's a bit weird. Since they're numbers it seems weird to mask the top of UniqueProcessId, but as far as I know there can't be anything > 0xffffffff in there. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:412: continue; On 2015/10/15 05:25:17, Mark Mentovai wrote: > Worthwhile to add the data we could get from SYSTEM_HANDLE_TABLE_ENTRY_INFO_EX > here? > > Maybe this ought to always populate as much as possible, never skipping a handle > found in the process: > > Handle result_handle; > result_handle.handle = static_cast<uint32_t>(handle.HandleValue); > result_handle.attributes = handle.HandleAttributes; > result_handle.granted_access = handle.GrantedAccess; > if (DuplicateHandle(…)) { > ScopedKernelHandle scoped_dup_handle(dup_handle); > scoped_ptr<uint8_t[]> buffer = QueryObject(…); > if (buffer) { > result_handle.pointer_count = object_basic_information->PointerCount; > result_handle.handle_count = object_basic_information->HandleCount - 1; > } > buffer.reset(); // because you don’t need both at the same time > buffer.reset(QueryObject(…)); > if (buffer) { > result_handle.type_name = std::wstring(…); > } > } > handles.push_back(result_handle); Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:428: Handle result_handle; On 2015/10/15 05:25:17, Mark Mentovai wrote: > Didn’t you find that MSVS 2015 didn’t like shadowing variables? (See line 398.) > If so, you could just declare this before the DuplicateHandle(). With your better version above, this disappears. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:438: // The Attributes and GrantedAccess sometimes differ slightly between the On 2015/10/15 05:25:17, Mark Mentovai wrote: > Blank before. And also a blank before pointer_count, since this comment doesn’t > apply to that. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:445: result_handle.pointer_count = object_basic_information->PointerCount; On 2015/10/15 05:25:17, Mark Mentovai wrote: > Just confirming: making a duplicate doesn’t actually affect this number at all, > right? That one's harder to test, but I believe it's actually two too big. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:450: handles.push_back(result_handle); On 2015/10/15 05:25:16, Mark Mentovai wrote: > Blank before this, too. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.cc:542: handles_ = BuildHandleVector<process_types::internal::Traits64>(process); On 2015/10/15 05:25:17, Mark Mentovai wrote: > Since this can’t do anything but get all handles system-wide, and because we may > not always want to get handles (like if we ever want to write a > handle-stream-less minidump), it makes some sense to do this lazily in > Handles(), as opposed to here in Initialize(). Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info.... util/win/process_info.h:144: const std::vector<Handle>& Handles() const { return handles_; } On 2015/10/15 05:25:17, Mark Mentovai wrote: > All of the other getters aren’t inline so they can also do > INITIALIZATION_STATE_DCHECK_VALID. This should be consistent with them. Also see > the comment I left in the Initialize() implementation. Done. https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... File util/win/process_info_test.cc (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... util/win/process_info_test.cc:568: EXPECT_EQ(0, handle.attributes); On 2015/10/15 05:25:17, Mark Mentovai wrote: > Not critical, but can we at least conjure up one HANDLE that has nonzero > attributes? Something inheritable, maybe? I don’t want all of the checks we have > for that field to test for what also happens to be its default value. So conjured! https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... util/win/process_info_test.cc:568: EXPECT_EQ(0, handle.attributes); On 2015/10/15 05:25:17, Mark Mentovai wrote: > Can we reason about pointer_count for any of these? == 1 or at the very least >= > 1? They seem reasonable-ish on XP (1s and 2s), but on Win10 they're all over the place. It seems like they've masked in some flag bits (?) or maybe use a non-zero base (I recall there was some exploits based on breaking refcounting a while back.) Windbg doesn't know what the flags mean if they're flags, and just reports things like 65537 or 32769, etc. So, I think the best I can do is >= 1 for now.
https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... File util/win/process_info_test.cc (right): https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info_... util/win/process_info_test.cc:568: EXPECT_EQ(0, handle.attributes); scottmg wrote: > On 2015/10/15 05:25:17, Mark Mentovai wrote: > > Can we reason about pointer_count for any of these? == 1 or at the very least > >= > > 1? > > They seem reasonable-ish on XP (1s and 2s), but on Win10 they're all over the > place. It seems like they've masked in some flag bits (?) or maybe use a > non-zero base (I recall there was some exploits based on breaking refcounting a > while back.) Windbg doesn't know what the flags mean if they're flags, and just > reports things like 65537 or 32769, etc. > > So, I think the best I can do is >= 1 for now. Since there may be flag bits in there and we’re not even sure they’re numbers, let’s call it != 0, that probably expresses our intent best. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:157: DCHECK_GE(size, minimum_size); return_length on the left side. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:417: if (object_basic_information_buffer) { This doesn’t totally match what I suggested at https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info..... I thought we’d be able to get ObjectBasicInformation, fill out result_handle appropriately if it succeeded, then get ObjectTypeInformation regardless of whether we could get ObjectBasicInformation, and fill out some more of result_handle if that succeeded. You can leave it as you have it if there’s a good reason, but I don’t see that the two different QueryObject()s depend on each other in any way, and the suggestion would save a level of indentation in a function that’s already got a bunch of indentation. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:446: // NtQueryObject() while the query was being executed. I didn’t realize that NtQueryObject() would have any permanent side effects. Does the PointerCount go down by 1 when scoped_dup_handle is released? I realize that this is a weird thing to test, but I’m trying to tease out whether PointerCount is a reference count or something that only ever increments. If it’s the latter, the subtraction may be pointless. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.h:63: //! 32 bit being the correct size for HANDLEs for proceses, even on Windows 32 bits, plural. Also, `HANDLE`s. Not just handles for processes, handles for anything.
https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:157: DCHECK_GE(size, minimum_size); On 2015/10/16 04:03:04, Mark Mentovai wrote: > return_length on the left side. Done. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:417: if (object_basic_information_buffer) { On 2015/10/16 04:03:04, Mark Mentovai wrote: > This doesn’t totally match what I suggested at > https://codereview.chromium.org/1400413002/diff/160001/util/win/process_info..... > I thought we’d be able to get ObjectBasicInformation, fill out result_handle > appropriately if it succeeded, then get ObjectTypeInformation regardless of > whether we could get ObjectBasicInformation, and fill out some more of > result_handle if that succeeded. > > You can leave it as you have it if there’s a good reason, but I don’t see that > the two different QueryObject()s depend on each other in any way, and the > suggestion would save a level of indentation in a function that’s already got a > bunch of indentation. Oops, of course that's better. Done. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:446: // NtQueryObject() while the query was being executed. On 2015/10/16 04:03:04, Mark Mentovai wrote: > I didn’t realize that NtQueryObject() would have any permanent side effects. > Does the PointerCount go down by 1 when scoped_dup_handle is released? I realize > that this is a weird thing to test, but I’m trying to tease out whether > PointerCount is a reference count or something that only ever increments. If > it’s the latter, the subtraction may be pointless. As far as I know it's a reference count. I'm not very knowledgeable here unfortunately, and I can't find much/any documentation on PointerCount. 2 seems to be the right value by experimentation (at least on some OSs). My hypothesis was that one pointer was added by the DuplicateHandle() which makes sense, probably owned by this process's handle table. My guess for the second was that it was a side-effect of NtQueryObject when querying for ObjectBasicInformation. i.e. in the prolog it +1s to maintain the lifetime, then copies out data, then -1s, but doesn't adjust for its own increment. This is all a little tricky though. If you attach to the process being inspected with windbg the PointerCounts are all out of whack I guess because windbg causes more references. I was verifying using ProcessHacker, but *it* appears to have a bug that causes it to decrement the reference count incorrectly every time you open the dialog that inspects the pointer count. Overall, I'm not sure how useful this value is anyway. We might be better off just removing it from ProcessInfo::Handle structure, and always writing a 0 (or 1) when writing the minidump. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.h:63: //! 32 bit being the correct size for HANDLEs for proceses, even on Windows On 2015/10/16 04:03:04, Mark Mentovai wrote: > 32 bits, plural. Also, `HANDLE`s. Not just handles for processes, handles for > anything. Done. Somehow I always feel illiterate after these reviews! :)
LGTM https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:446: // NtQueryObject() while the query was being executed. On 2015/10/16 20:36:01, scottmg wrote: > On 2015/10/16 04:03:04, Mark Mentovai wrote: > > I didn’t realize that NtQueryObject() would have any permanent side effects. > > Does the PointerCount go down by 1 when scoped_dup_handle is released? I > realize > > that this is a weird thing to test, but I’m trying to tease out whether > > PointerCount is a reference count or something that only ever increments. If > > it’s the latter, the subtraction may be pointless. > > As far as I know it's a reference count. I'm not very knowledgeable here > unfortunately, and I can't find much/any documentation on PointerCount. 2 seems > to be the right value by experimentation (at least on some OSs). > > My hypothesis was that one pointer was added by the DuplicateHandle() which > makes sense, probably owned by this process's handle table. My guess for the > second was that it was a side-effect of NtQueryObject when querying for > ObjectBasicInformation. i.e. in the prolog it +1s to maintain the lifetime, then > copies out data, then -1s, but doesn't adjust for its own increment. > > This is all a little tricky though. If you attach to the process being inspected > with windbg the PointerCounts are all out of whack I guess because windbg causes > more references. I was verifying using ProcessHacker, but *it* appears to have a > bug that causes it to decrement the reference count incorrectly every time you > open the dialog that inspects the pointer count. > > Overall, I'm not sure how useful this value is anyway. We might be better off > just removing it from ProcessInfo::Handle structure, and always writing a 0 (or > 1) when writing the minidump. OK. -2, -0, or 0, at your option. Whatever you think is best. https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.h File util/win/process_info.h (right): https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.h:63: //! 32 bit being the correct size for HANDLEs for proceses, even on Windows On 2015/10/16 20:36:01, scottmg wrote: > On 2015/10/16 04:03:04, Mark Mentovai wrote: > > 32 bits, plural. Also, `HANDLE`s. Not just handles for processes, handles for > > anything. > > Done. Somehow I always feel illiterate after these reviews! :) You can blame it on your text editor like I do. :) https://codereview.chromium.org/1400413002/diff/200001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/200001/util/win/process_info.... util/win/process_info.cc:369: } This is a kind of big allocation, so reset() to empty before reset(new) to avoid having both allocated simultaneously. Optionally you can reset() the first NtQueryObject() buffer below before making the second call, but that's not as critical because I don't expect that those buffers will be very big.
https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/180001/util/win/process_info.... util/win/process_info.cc:446: // NtQueryObject() while the query was being executed. On 2015/10/16 22:04:05, Mark Mentovai wrote: > On 2015/10/16 20:36:01, scottmg wrote: > > On 2015/10/16 04:03:04, Mark Mentovai wrote: > > > I didn’t realize that NtQueryObject() would have any permanent side effects. > > > Does the PointerCount go down by 1 when scoped_dup_handle is released? I > > realize > > > that this is a weird thing to test, but I’m trying to tease out whether > > > PointerCount is a reference count or something that only ever increments. If > > > it’s the latter, the subtraction may be pointless. > > > > As far as I know it's a reference count. I'm not very knowledgeable here > > unfortunately, and I can't find much/any documentation on PointerCount. 2 > seems > > to be the right value by experimentation (at least on some OSs). > > > > My hypothesis was that one pointer was added by the DuplicateHandle() which > > makes sense, probably owned by this process's handle table. My guess for the > > second was that it was a side-effect of NtQueryObject when querying for > > ObjectBasicInformation. i.e. in the prolog it +1s to maintain the lifetime, > then > > copies out data, then -1s, but doesn't adjust for its own increment. > > > > This is all a little tricky though. If you attach to the process being > inspected > > with windbg the PointerCounts are all out of whack I guess because windbg > causes > > more references. I was verifying using ProcessHacker, but *it* appears to have > a > > bug that causes it to decrement the reference count incorrectly every time you > > open the dialog that inspects the pointer count. > > > > Overall, I'm not sure how useful this value is anyway. We might be better off > > just removing it from ProcessInfo::Handle structure, and always writing a 0 > (or > > 1) when writing the minidump. > > OK. -2, -0, or 0, at your option. Whatever you think is best. I'll stick with -2 as it seems as right-est as I can get it for now. https://codereview.chromium.org/1400413002/diff/200001/util/win/process_info.cc File util/win/process_info.cc (right): https://codereview.chromium.org/1400413002/diff/200001/util/win/process_info.... util/win/process_info.cc:369: } On 2015/10/16 22:04:05, Mark Mentovai wrote: > This is a kind of big allocation, so reset() to empty before reset(new) to avoid > having both allocated simultaneously. Done. > Optionally you can reset() the first NtQueryObject() buffer below before making > the second call, but that's not as critical because I don't expect that those > buffers will be very big. Didn't do this one as it makes that allocation seem important when it's < 100 bytes.
Message was sent while issue was closed.
Committed patchset #9 (id:240001) manually as 7de04b02f85ddfeee778b71f804d4d30c971ccd8 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
