win: Capture some CRITICAL_SECTION debugging data

win: Capture some CRITICAL_SECTION debugging data

Capture the memory for the loader lock (can be inspected by !cs), as
well as all locks that were created with .DebugInfo which can be viewed
with !locks.

e.g.

0:000> !cs ntdll!LdrpLoaderLock
-----------------------------------------
Critical section   = 0x778d6410 (ntdll!LdrpLoaderLock+0x0)
DebugInfo          = 0x778d6b6c
NOT LOCKED
LockSemaphore      = 0x0
SpinCount          = 0x04000000


0:000> !locks -v

CritSec ntdll!RtlpProcessHeapsListLock+0 at 778d7620
LockCount          NOT LOCKED
RecursionCount     0
OwningThread       0
EntryCount         0
ContentionCount    0

CritSec +7a0248 at 007a0248
LockCount          NOT LOCKED
RecursionCount     0
OwningThread       0
EntryCount         0
ContentionCount    0

CritSec crashy_program!g_critical_section_with_debug_info+0 at 01342c48
LockCount          NOT LOCKED
RecursionCount     0
OwningThread       0
EntryCount         0
ContentionCount    0

CritSec crashy_program!crashpad::`anonymous namespace'::g_test_critical_section+0 at 01342be0
WaiterWoken        No
LockCount          0
RecursionCount     1
OwningThread       34b8
EntryCount         0
ContentionCount    0
*** Locked

Scanned 4 critical sections

R=mark@chromium.org
BUG=crashpad:52
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/4893a9b76d68176ba08857003ea4281d918768cf

[scottmg, 2015-10-08 20:31:16]

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

[Mark Mentovai, 2015-10-12 14:08:59]

https://codereview.chromium.org/1392093003/diff/60001/client/crashpad_client_...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1392093003/diff/60001/client/crashpad_client_...
client/crashpad_client_win.cc:155: message.registration.critical_section_address
=
I’m not sure this is necessary.

Sure, I understand how you’re using it here to give the handler a hint about
where to find the memory it needs for this. But the debugger doesn’t have access
to this particular critical_section_address, and !locks seems to work there just
fine. You don’t have to tell it where a CRITICAL_SECTION is in order for that to
work. This hint is never written to the minidump anywhere.

Our ProcessReader must already have all of the information it needs to find
where the the CRITICAL_SECTIONs with debug info reside, and this must also
already be making it into the minidumps we write. Maybe it’s hiding out
somewhere in the PEB?

(To be clear, I know that we need to write the block of CRITICAL_SECTIONs out as
ExtraMemory, I just don’t see how this hint is necessary at registration time.)

https://codereview.chromium.org/1392093003/diff/60001/snapshot/win/process_sn...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1392093003/diff/60001/snapshot/win/process_sn...
snapshot/win/process_snapshot_win.cc:434: // Add both RTL_CRITICAL_SECTION_DEBUG
and RTL_CRITICAL_SECTION_DEBUG to the
One of these shouldn’t be _DEBUG.

[Mark Mentovai, 2015-10-12 15:55:41]

Asanka tells me via his blog that we want ntdll.dll!RtlCriticalSectionList.
http://soulsampler.blogspot.com/2010/01/critical-section-contention.

Unfortunately, I wasn't able to find the referenced MSDN Magazine article, at
least not from my phone.

[Mark Mentovai, 2015-10-12 16:20:41]

The original article was

Break Free of Code Deadlocks in Critical Sections Under Windows
Matt Pietrek and Russ Osterlund
MSDN Magazine, December 2003

which is now only officially available in this stupid chm archive, 

https://download.microsoft.com/download/3/a/7/3a7fa450-1f33-41f7-9e6d-3aa95b5...

but you can also Google for the title and find it in more accessible formats.

[Mark Mentovai, 2015-10-12 19:24:20]

Oh, I guess the problem is that RtlCriticalSectionList isn’t exported from
ntdll.dll and can’t be found without debugging symbols. The debugger’s got those
and can use them, but we don’t have access to them on a run-of-the-mill client
system, and wouldn’t want to rely on them for this anyway.

Even so, the article says that they’re able to get the
RTL_CRITICAL_SECTION_DEBUG for PEB::LoaderLock and use that.

I saw your comment saying that PEB::LoaderLock may or may not have a valid
DebugInfo field, but that it probably doesn’t. So I this may be something that’s
changed since 2003, OK, I’ll buy that.
RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO is new in Windows 8.1 (or 8). But if
we do need to pass the address of a CRITICAL_SECTION over like this, I think we
should write down in a comment somewhere that this isn’t how a debugger does it,
and why we have to do it differently.

Speaking of RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO being new-ish, is it
accepted and ignored on older OS versions?

[scottmg, 2015-10-13 21:07:33]

On 2015/10/12 19:24:20, Mark Mentovai wrote:
> Oh, I guess the problem is that RtlCriticalSectionList isn’t exported from
> ntdll.dll and can’t be found without debugging symbols. The debugger’s got
those
> and can use them, but we don’t have access to them on a run-of-the-mill client
> system, and wouldn’t want to rely on them for this anyway.

Yes, that's exactly right. Sorry, I should have been more explicit in the
comments to avoid you spelunking around.

> 
> Even so, the article says that they’re able to get the
> RTL_CRITICAL_SECTION_DEBUG for PEB::LoaderLock and use that.

At least on Win10, PEB::LoaderLock has a pseudo RTL_CRITICAL_SECTION_DEBUG that
isn't linked up to the doubly linked list. There's also PEB::FastPebLock, but
it's allocated entirely without debug info.

> 
> I saw your comment saying that PEB::LoaderLock may or may not have a valid
> DebugInfo field, but that it probably doesn’t. So I this may be something
that’s
> changed since 2003, OK, I’ll buy that.
> RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO is new in Windows 8.1 (or 8). But
if
> we do need to pass the address of a CRITICAL_SECTION over like this, I think
we
> should write down in a comment somewhere that this isn’t how a debugger does
it,
> and why we have to do it differently.

Added a comment to that effect.
 
> Speaking of RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO being new-ish, is it
> accepted and ignored on older OS versions?

I just tested on XP. I had the import of InitializeCriticalSectionEx() wrong,
but other than that it seems to work OK.

[Mark Mentovai, 2015-10-15 00:14:57]

This is a clever solution!

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:51: // A `CRITICAL_SECTION` initialized with
// (as opposed to //!) comments aren’t Doxygenated, no need for `backticks`.

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:139: DCHECK_EQ(g_non_crash_dump_done,
INVALID_HANDLE_VALUE);
DCHECK that g_critical_section_with_debug_info is unset too.

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:158: if (CrashpadInitializeCriticalSectionEx(
Otherwise, PLOG().

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/end_to_en...
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/end_to_en...
snapshot/win/end_to_end_test.py:180: # Locks.
There isn’t a # comment for any of the other classes. You can lose this (!locks
is obvious) or add comments for the other things you test here.

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:458: if
(critical_section_debug.ProcessLocksList.Flink == 0)
You’ve only followed the Flinks, so you won’t catch anything that shows up in
the list before the starting point. Could you follow the Blinks first to back up
to the head of the list, and then follow the Flinks?

https://codereview.chromium.org/1392093003/diff/120001/util/win/registration_...
File util/win/registration_protocol_win.h (right):

https://codereview.chromium.org/1392093003/diff/120001/util/win/registration_...
util/win/registration_protocol_win.h:62: //! \brief The address, in the client
process address space, of a
Possessive on “process”.

[scottmg, 2015-10-15 18:21:22]

Patchset #8 (id:140001) has been deleted

[scottmg, 2015-10-15 18:22:23]

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:51: // A `CRITICAL_SECTION` initialized with
On 2015/10/15 00:14:56, Mark Mentovai wrote:
> // (as opposed to //!) comments aren’t Doxygenated, no need for `backticks`.

Done.

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:139: DCHECK_EQ(g_non_crash_dump_done,
INVALID_HANDLE_VALUE);
On 2015/10/15 00:14:56, Mark Mentovai wrote:
> DCHECK that g_critical_section_with_debug_info is unset too.

It's not a handle, it's just a POD struct with no real "initialized" value to
check.

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:158: if (CrashpadInitializeCriticalSectionEx(
On 2015/10/15 00:14:56, Mark Mentovai wrote:
> Otherwise, PLOG().

Done.

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/end_to_en...
File snapshot/win/end_to_end_test.py (right):

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/end_to_en...
snapshot/win/end_to_end_test.py:180: # Locks.
On 2015/10/15 00:14:56, Mark Mentovai wrote:
> There isn’t a # comment for any of the other classes. You can lose this
(!locks
> is obvious) or add comments for the other things you test here.

Done.

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1392093003/diff/120001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:458: if
(critical_section_debug.ProcessLocksList.Flink == 0)
On 2015/10/15 00:14:56, Mark Mentovai wrote:
> You’ve only followed the Flinks, so you won’t catch anything that shows up in
> the list before the starting point. Could you follow the Blinks first to back
up
> to the head of the list, and then follow the Flinks?

Done. It seems to be a circular list, but I can't find anything that says for
sure that it is, so seems safest to do an initial Blink walk before Flink, even
if that means we end up walking the list twice.

https://codereview.chromium.org/1392093003/diff/120001/util/win/registration_...
File util/win/registration_protocol_win.h (right):

https://codereview.chromium.org/1392093003/diff/120001/util/win/registration_...
util/win/registration_protocol_win.h:62: //! \brief The address, in the client
process address space, of a
On 2015/10/15 00:14:56, Mark Mentovai wrote:
> Possessive on “process”.

Done.

[Mark Mentovai, 2015-10-15 19:37:59]

It’s too bad that there’s so much plumbing required to make this work, but it’s
a crafty solution. LGTM.

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:139: DCHECK_EQ(g_non_crash_dump_done,
INVALID_HANDLE_VALUE);
scottmg wrote:
> On 2015/10/15 00:14:56, Mark Mentovai wrote:
> > DCHECK that g_critical_section_with_debug_info is unset too.
> 
> It's not a handle, it's just a POD struct with no real "initialized" value to
> check.

Can’t we check the zeroness of the DebugInfo field?

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:311: // not have a .DebugInfo list, but
typically doesn't. If it does, then we may
It doesn’t on Windows 10 at least is where we landed on this, I think. It
probably does on pre-8 or pre-8.1.

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:439: WinVMAddress current =
critical_section.DebugInfo;
  const WinVMAddress start_address = critical_section.DebugInfo;
  WinVMAddress current_address = start_address;

and then reference start on line 463 instead of critical_section.DebugInfo.
critical_section.DebugInfo is confusing because you’re using it as start, but
it’s not clear without reading the entire loop body that you haven’t changed
critical_section, since the whole point of that loop is to iterate through the
closely-related critical_section_debug.

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:466: // Unexpectedly encountered a bad
record, so restart at the original entry
We could restart at the entry we were looking at before we hit the invalid one.

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.h (right):

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.h:64: //!     .DebugInfo. Used as a starting
point to walk the process's locks.
`` on .DebugInfo.

https://codereview.chromium.org/1392093003/diff/160001/util/win/exception_han...
File util/win/exception_handler_server.h (right):

https://codereview.chromium.org/1392093003/diff/160001/util/win/exception_han...
util/win/exception_handler_server.h:54: //!     .DebugInfo field, or `0` if
unavailable.
`.DebugInfo` here too.

[scottmg, 2015-10-15 20:06:41]

Yeah, I was disappointed at how much passing around there was, but I guess it's
at least only internal to crashpad_client_win, not all the way back to the
client.

I did hunt for quite a while to find something that could get us the ntdll list
head by side-effect, but I couldn't find anything.

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1392093003/diff/120001/client/crashpad_client...
client/crashpad_client_win.cc:139: DCHECK_EQ(g_non_crash_dump_done,
INVALID_HANDLE_VALUE);
On 2015/10/15 19:37:58, Mark Mentovai wrote:
> scottmg wrote:
> > On 2015/10/15 00:14:56, Mark Mentovai wrote:
> > > DCHECK that g_critical_section_with_debug_info is unset too.
> > 
> > It's not a handle, it's just a POD struct with no real "initialized" value
to
> > check.
> 
> Can’t we check the zeroness of the DebugInfo field?

Done.

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:311: // not have a .DebugInfo list, but
typically doesn't. If it does, then we may
On 2015/10/15 19:37:59, Mark Mentovai wrote:
> It doesn’t on Windows 10 at least is where we landed on this, I think. It
> probably does on pre-8 or pre-8.1.

Added what I actually know for sure without more installing/testing.

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:439: WinVMAddress current =
critical_section.DebugInfo;
On 2015/10/15 19:37:59, Mark Mentovai wrote:
>   const WinVMAddress start_address = critical_section.DebugInfo;
>   WinVMAddress current_address = start_address;
> 
> and then reference start on line 463 instead of critical_section.DebugInfo.
> critical_section.DebugInfo is confusing because you’re using it as start, but
> it’s not clear without reading the entire loop body that you haven’t changed
> critical_section, since the whole point of that loop is to iterate through the
> closely-related critical_section_debug.

Done.

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:466: // Unexpectedly encountered a bad
record, so restart at the original entry
On 2015/10/15 19:37:58, Mark Mentovai wrote:
> We could restart at the entry we were looking at before we hit the invalid
one.

Done.

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.h (right):

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.h:64: //!     .DebugInfo. Used as a starting
point to walk the process's locks.
On 2015/10/15 19:37:59, Mark Mentovai wrote:
> `` on .DebugInfo.

Done.

https://codereview.chromium.org/1392093003/diff/160001/util/win/exception_han...
File util/win/exception_handler_server.h (right):

https://codereview.chromium.org/1392093003/diff/160001/util/win/exception_han...
util/win/exception_handler_server.h:54: //!     .DebugInfo field, or `0` if
unavailable.
On 2015/10/15 19:37:59, Mark Mentovai wrote:
> `.DebugInfo` here too.

Done.

[Mark Mentovai, 2015-10-15 20:14:41]

LGTM

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1392093003/diff/160001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:311: // not have a .DebugInfo list, but
typically doesn't. If it does, then we may
scottmg wrote:
> On 2015/10/15 19:37:59, Mark Mentovai wrote:
> > It doesn’t on Windows 10 at least is where we landed on this, I think. It
> > probably does on pre-8 or pre-8.1.
> 
> Added what I actually know for sure without more installing/testing.

Perfect. (Surprised you don’t have VMs for arbitrary weird versions, but it’s
not necessary for this.)

[scottmg, 2015-10-15 20:18:14]

Committed patchset #9 (id:180001) manually as
4893a9b76d68176ba08857003ea4281d918768cf (presubmit successful).