win: Save contents of TEBs allowing !teb and !gle to work in windbg

win: Save contents of TEBs allowing !teb and !gle to work in windbg

crashy_program's log looks something like this now:

0:000> .ecxr
eax=00000007 ebx=7f24e000 ecx=7f24d000 edx=00000000 esi=00497ec8 edi=00d39ca0
eip=00cf5d12 esp=001ffcd8 ebp=001ffcdc iopl=0         nv up ei ng nz ac po cy
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010293
crashy_program+0x5d12:
00cf5d12 ??              ???
0:000> !teb
TEB at 7f24d000
    ExceptionList:        001ff548
    StackBase:            00200000
    StackLimit:           001fd000
    SubSystemTib:         00000000
    FiberData:            00001e00
    ArbitraryUserPointer: 00000000
    Self:                 7f24d000
    EnvironmentPointer:   00000000
    ClientId:             00003658 . 00004630
    RpcHandle:            00000000
    Tls Storage:          7f24d02c
    PEB Address:          7f24e000
    LastErrorValue:       2
    LastStatusValue:      c000000f
    Count Owned Locks:    0
    HardErrorMode:        0
0:000> !gle
LastErrorValue: (Win32) 0x2 (2) - The system cannot find the file specified.
LastStatusValue: (NTSTATUS) 0xc000000f - {File Not Found}  The file %hs does not exist.

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

[scottmg, 2015-09-29 18:33:52]

Patchset #4 (id:60001) has been deleted

[Mark Mentovai, 2015-09-29 22:14:04]

Busted pointers and unreadable pages in the map are as much of a concern here as
in https://codereview.chromium.org/1360863006/.

https://codereview.chromium.org/1364803004/diff/120001/handler/win/crashy_tes...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1364803004/diff/120001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:25: volatile int* foo =
reinterpret_cast<volatile int*>(7);
Move after CreateFile().

https://codereview.chromium.org/1364803004/diff/120001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:27: CreateFile(L"non-existent file",
Since we don’t control the current directory or its contents…

Maybe don’t crash if this doesn’t return INVALID_HANDLE_VALUE, so that the
inevitable test failure will look like this program’s problem and not the
process/thread reader’s.

SetLastError() may be an easier option, though.

https://codereview.chromium.org/1364803004/diff/120001/minidump/minidump_thre...
File minidump/minidump_thread_writer.cc (right):

https://codereview.chromium.org/1364803004/diff/120001/minidump/minidump_thre...
minidump/minidump_thread_writer.cc:152:
memory_list_writer_->AddFromSnapshot(thread_snapshot->ExtraMemory());
For purposes of locality in the minidump file, I’d like to keep the thread
stacks contiguous in the minidump file. So this ExtraMemory() add should be in a
separate loop that runs after all of the AddThread() calls are done.

I’ve got this idea that a future minidump reader might be able to make some
sense out of incomplete files, so I’d like the more important stuff like thread
stack memory to ride up front, and the less important stuff like miscellaneous
memory regions to follow.

https://codereview.chromium.org/1364803004/diff/120001/snapshot/thread_snapsh...
File snapshot/thread_snapshot.h (right):

https://codereview.chromium.org/1364803004/diff/120001/snapshot/thread_snapsh...
snapshot/thread_snapshot.h:71: //! \return An vector of MemorySnapshot objects
that will be included in the
“an vector?” Don’t tell me that you pronounce this one with a silent V.

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

https://codereview.chromium.org/1364803004/diff/120001/util/win/process_struc...
util/win/process_structs.h:319: typename Traits::Pointer Reserved0[397];
Can you name the reserved fields to match <winternal.h>? Start at Reserved2
(although its base offset is different, so maybe name it something like
SortaReserved2 with a comment), since you’ve got real fields where Reserved1
was.

https://codereview.chromium.org/1364803004/diff/120001/util/win/process_struc...
util/win/process_structs.h:323: typename Traits::Pointer Reserved3[32];
No reason you can’t have this be Reserved5/ForOle/6/TlsExpansionSlots matching
<winternl.h> too.

Even if we’re not looking inside this structure, having things be matchy-matchy
helps reason that it is in fact equivalent to the OS’ idea of the same.

[scottmg, 2015-09-30 18:54:11]

Thanks. I didn't take any action on the "busted pointers" problem here -- a
process with an unreadable TEB has a pretty broken thread. It's certainly not
completely impossible, but it doesn't seem too likely to be an immediate problem
either.

https://codereview.chromium.org/1364803004/diff/120001/handler/win/crashy_tes...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1364803004/diff/120001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:25: volatile int* foo =
reinterpret_cast<volatile int*>(7);
On 2015/09/29 22:14:04, Mark Mentovai - August is over wrote:
> Move after CreateFile().

Done.

https://codereview.chromium.org/1364803004/diff/120001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:27: CreateFile(L"non-existent file",
On 2015/09/29 22:14:04, Mark Mentovai - August is over wrote:
> Since we don’t control the current directory or its contents…
> 
> Maybe don’t crash if this doesn’t return INVALID_HANDLE_VALUE, so that the
> inevitable test failure will look like this program’s problem and not the
> process/thread reader’s.
> 
> SetLastError() may be an easier option, though.

SetLastError() seems much better. Done.

https://codereview.chromium.org/1364803004/diff/120001/minidump/minidump_thre...
File minidump/minidump_thread_writer.cc (right):

https://codereview.chromium.org/1364803004/diff/120001/minidump/minidump_thre...
minidump/minidump_thread_writer.cc:152:
memory_list_writer_->AddFromSnapshot(thread_snapshot->ExtraMemory());
On 2015/09/29 22:14:04, Mark Mentovai - August is over wrote:
> For purposes of locality in the minidump file, I’d like to keep the thread
> stacks contiguous in the minidump file. So this ExtraMemory() add should be in
a
> separate loop that runs after all of the AddThread() calls are done.
> 
> I’ve got this idea that a future minidump reader might be able to make some
> sense out of incomplete files, so I’d like the more important stuff like
thread
> stack memory to ride up front, and the less important stuff like miscellaneous
> memory regions to follow.

Done.

https://codereview.chromium.org/1364803004/diff/120001/snapshot/thread_snapsh...
File snapshot/thread_snapshot.h (right):

https://codereview.chromium.org/1364803004/diff/120001/snapshot/thread_snapsh...
snapshot/thread_snapshot.h:71: //! \return An vector of MemorySnapshot objects
that will be included in the
On 2015/09/29 22:14:04, Mark Mentovai - August is over wrote:
> “an vector?” Don’t tell me that you pronounce this one with a silent V.

:-) Done.

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

https://codereview.chromium.org/1364803004/diff/120001/util/win/process_struc...
util/win/process_structs.h:319: typename Traits::Pointer Reserved0[397];
On 2015/09/29 22:14:04, Mark Mentovai - August is over wrote:
> Can you name the reserved fields to match <winternal.h>? Start at Reserved2
> (although its base offset is different, so maybe name it something like
> SortaReserved2 with a comment), since you’ve got real fields where Reserved1
> was.

Done.

https://codereview.chromium.org/1364803004/diff/120001/util/win/process_struc...
util/win/process_structs.h:323: typename Traits::Pointer Reserved3[32];
On 2015/09/29 22:14:04, Mark Mentovai - August is over wrote:
> No reason you can’t have this be Reserved5/ForOle/6/TlsExpansionSlots matching
> <winternl.h> too.
> 
> Even if we’re not looking inside this structure, having things be
matchy-matchy
> helps reason that it is in fact equivalent to the OS’ idea of the same.

Done.

[Mark Mentovai, 2015-10-01 20:28:06]

LGTM

https://codereview.chromium.org/1364803004/diff/180001/handler/win/crashy_tes...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1364803004/diff/180001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:20: #define STATUS_NO_SUCH_FILE 0xC000000F
<ntstatus.h> uses ((NTSTATUS)0xC000000FL). We should probably match, at least
the NTSTATUS part (but perhaps with implicit_cast or static_cast for Google
style).

https://codereview.chromium.org/1364803004/diff/180001/minidump/minidump_thre...
File minidump/minidump_thread_writer.cc (right):

https://codereview.chromium.org/1364803004/diff/180001/minidump/minidump_thre...
minidump/minidump_thread_writer.cc:154: for (const ThreadSnapshot*
thread_snapshot : thread_snapshots)
This does deserve a comment explaining why we’re doing the same loop twice.

https://codereview.chromium.org/1364803004/diff/180001/snapshot/win/thread_sn...
File snapshot/win/thread_snapshot_win.cc (right):

https://codereview.chromium.org/1364803004/diff/180001/snapshot/win/thread_sn...
snapshot/win/thread_snapshot_win.cc:100: std::vector<const MemorySnapshot*>
extra_memory;
(1, &teb_)

https://codereview.chromium.org/1364803004/diff/180001/snapshot/win/thread_sn...
snapshot/win/thread_snapshot_win.cc:101: extra_memory.push_back(&teb_);
I’ll give you a pass on this for now, as long as you leave behind a link to
crashpad:59.

I guess I’m a little more fearful of this than you. Since a process’ address
space is basically within its own control, it can be busted in pretty much any
way imaginable, intentionally or not. Yeah, it’s true that only a sick thread
would have a screwed-up TEB, but we’re in the crash-reporting business, so we
can’t just throw our hands up in the air because a thread or process is sick.
“Unreadable TEB” may wind up being the reason for a crash some day.

I mention “intentionally” because we should also be aware that garbageware that
injects itself into Chrome might try to break internal structures to prevent it
from being debugged effectively, or to prevent crash dumps from being sent at
all. We’ve got a very strong interest in minimizing that attack area.

Anyway, yeah, we do need to be able to read the TEB ourselves to do our job
here, but we only consult a few of its early fields, and if they’re messed up,
we only bail on reporting the thread altogether. Maybe we should report
something for the thread in that case too, but it’s kind of important that we
don’t toss the whole dump out. With this change, a problem later in the TEB
would be fatal for the dump as a whole, but the thread itself might not appear
to have anything wrong with it until it attempted a TLS operation.

https://codereview.chromium.org/1364803004/diff/180001/util/win/process_struc...
File util/win/process_structs.h (right):

https://codereview.chromium.org/1364803004/diff/180001/util/win/process_struc...
util/win/process_structs.h:319: // Not identical to Reserved2 in winternl's _TEB
because we define ClientId.
Blank before this, and after RemainderOfReserved2.

[scottmg, 2015-10-01 21:01:07]

https://codereview.chromium.org/1364803004/diff/180001/handler/win/crashy_tes...
File handler/win/crashy_test_program.cc (right):

https://codereview.chromium.org/1364803004/diff/180001/handler/win/crashy_tes...
handler/win/crashy_test_program.cc:20: #define STATUS_NO_SUCH_FILE 0xC000000F
On 2015/10/01 20:28:05, Mark Mentovai wrote:
> <ntstatus.h> uses ((NTSTATUS)0xC000000FL). We should probably match, at least
> the NTSTATUS part (but perhaps with implicit_cast or static_cast for Google
> style).

Done.

https://codereview.chromium.org/1364803004/diff/180001/minidump/minidump_thre...
File minidump/minidump_thread_writer.cc (right):

https://codereview.chromium.org/1364803004/diff/180001/minidump/minidump_thre...
minidump/minidump_thread_writer.cc:154: for (const ThreadSnapshot*
thread_snapshot : thread_snapshots)
On 2015/10/01 20:28:05, Mark Mentovai wrote:
> This does deserve a comment explaining why we’re doing the same loop twice.

Done.

https://codereview.chromium.org/1364803004/diff/180001/snapshot/win/thread_sn...
File snapshot/win/thread_snapshot_win.cc (right):

https://codereview.chromium.org/1364803004/diff/180001/snapshot/win/thread_sn...
snapshot/win/thread_snapshot_win.cc:100: std::vector<const MemorySnapshot*>
extra_memory;
On 2015/10/01 20:28:05, Mark Mentovai wrote:
> (1, &teb_)

Done.

https://codereview.chromium.org/1364803004/diff/180001/snapshot/win/thread_sn...
snapshot/win/thread_snapshot_win.cc:101: extra_memory.push_back(&teb_);
On 2015/10/01 20:28:05, Mark Mentovai wrote:
> I’ll give you a pass on this for now, as long as you leave behind a link to
> crashpad:59.
> 
> I guess I’m a little more fearful of this than you. Since a process’ address
> space is basically within its own control, it can be busted in pretty much any
> way imaginable, intentionally or not. Yeah, it’s true that only a sick thread
> would have a screwed-up TEB, but we’re in the crash-reporting business, so we
> can’t just throw our hands up in the air because a thread or process is sick.
> “Unreadable TEB” may wind up being the reason for a crash some day.
> 
> I mention “intentionally”&nbsp;because we should also be aware that
garbageware that
> injects itself into Chrome might try to break internal structures to prevent
it
> from being debugged effectively, or to prevent crash dumps from being sent at
> all. We’ve got a very strong interest in minimizing that attack area.
> 
> Anyway, yeah, we do need to be able to read the TEB ourselves to do our job
> here, but we only consult a few of its early fields, and if they’re messed up,
> we only bail on reporting the thread altogether. Maybe we should report
> something for the thread in that case too, but it’s kind of important that we
> don’t toss the whole dump out. With this change, a problem later in the TEB
> would be fatal for the dump as a whole, but the thread itself might not appear
> to have anything wrong with it until it attempted a TLS operation.

I'm probably not fearful enough. Added TODO links.

https://codereview.chromium.org/1364803004/diff/180001/util/win/process_struc...
File util/win/process_structs.h (right):

https://codereview.chromium.org/1364803004/diff/180001/util/win/process_struc...
util/win/process_structs.h:319: // Not identical to Reserved2 in winternl's _TEB
because we define ClientId.
On 2015/10/01 20:28:05, Mark Mentovai wrote:
> Blank before this, and after RemainderOfReserved2.

Done.

[scottmg, 2015-10-01 21:04:55]

Committed patchset #10 (id:200001) manually as
ecf3b37863367acfca2dcbae14a26e965d713e84 (presubmit successful).