win: Make StartHandler() restrict HANDLES inherited by crashpad_handler

win: Make StartHandler() restrict HANDLES inherited by crashpad_handler

This requires Windows NT 6.0 (Vista and Server 2008). On earlier
operating system versions, the existing behavior of inheriting all
inheritable handles is retained.

BUG=crashpad:69
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/d3825afb25a02aaeae466d37debcf2a9935b7266

[Mark Mentovai, 2015-11-06 19:58:06]

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

[scottmg, 2015-11-06 20:15:02]

LGTM

I haven't used these APIs before. This just means it'll get defaults for all the
standard things (WindowStation, etc.), rather than the same ones as the parent,
right? Is there anything we could test to confirm we're restricting as expected?

https://codereview.chromium.org/1427273003/diff/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1427273003/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:224: GET_FUNCTION(L"kernel32.dll",
::InitializeProcThreadAttributeList);
Probably don't need :: on these?

[scottmg, 2015-11-06 20:15:34]

(Also, http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx
seems like a good reference for future me looking here.)

[Mark Mentovai, 2015-11-06 20:21:13]

scottmg wrote:
> I haven't used these APIs before. This just means it'll get defaults for all
the
> standard things (WindowStation, etc.), rather than the same ones as the
parent,
> right?

That’s my understanding. But if there’s anything that we find we need to carry
through to the child, we can add it here easily.

> Is there anything we could test to confirm we're restricting as expected?

Not easily without promoting this to a library function in util, but I didn’t do
that because it’s single-use. Might make sense, though. What do you think?

> (Also, http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx
> seems like a good reference for future me looking here.)

Do you want me to include that link in the code?

[scottmg, 2015-11-06 20:31:50]

On 2015/11/06 20:21:13, Mark Mentovai wrote:
> scottmg wrote:
> > I haven't used these APIs before. This just means it'll get defaults for all
> the
> > standard things (WindowStation, etc.), rather than the same ones as the
> parent,
> > right?
> 
> That’s my understanding. But if there’s anything that we find we need to carry
> through to the child, we can add it here easily.
> 
> > Is there anything we could test to confirm we're restricting as expected?
> 
> Not easily without promoting this to a library function in util, but I didn’t
do
> that because it’s single-use. Might make sense, though. What do you think?

I guess it depends on the goal of restriction. What's the drawback if some sneak
through? I think the failure case of, say, none getting through is handled by
end_to_end_test.py so we don't need a specifically targeted test there.

> 
> > (Also, http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx
> > seems like a good reference for future me looking here.)
> 
> Do you want me to include that link in the code?

Nah, I think the code and simple searches are enough, it was just a useful
sanity-check.

[Mark Mentovai, 2015-11-06 21:49:09]

Oh, this never got sent.

https://codereview.chromium.org/1427273003/diff/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1427273003/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:224: GET_FUNCTION(L"kernel32.dll",
::InitializeProcThreadAttributeList);
scottmg wrote:
> Probably don't need :: on these?

We used :: everywhere else because of type confusion when a wrapper function of
the same name is present. (It messes up the calling convention.)

There aren’t wrappers here, but who knows how these will be copied and pasted
and borrowed in the future, so it’s safest to just leave ’em.

[scottmg, 2015-11-06 21:53:34]

https://codereview.chromium.org/1427273003/diff/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1427273003/diff/1/client/crashpad_client_win....
client/crashpad_client_win.cc:224: GET_FUNCTION(L"kernel32.dll",
::InitializeProcThreadAttributeList);
On 2015/11/06 21:49:09, Mark Mentovai wrote:
> scottmg wrote:
> > Probably don't need :: on these?
> 
> We used :: everywhere else because of type confusion when a wrapper function
of
> the same name is present. (It messes up the calling convention.)
> 
> There aren’t wrappers here, but who knows how these will be copied and pasted
> and borrowed in the future, so it’s safest to just leave ’em.

OK, that's fine then too.

[Mark Mentovai, 2015-11-06 21:55:36]

Committed patchset #1 (id:1) manually as
d3825afb25a02aaeae466d37debcf2a9935b7266 (presubmit successful).