UpdateProcThreadAttribute has a restriction that its lpValue parameter

UpdateProcThreadAttribute has a restriction that its lpValue parameter
live until DeleteProcThreadAttributeList is called.

An optimization in clang exposed this bug
(https://llvm.org/bugs/show_bug.cgi?id=23220).

Covered by existing tests: ProcessMitigationsTest.CheckDep
BUG=476316
Committed: https://crrev.com/a9f5526d0568508ab6e4913e23d3c8f33d040010
Cr-Commit-Position: refs/heads/master@{#326347}

[majnemer, 2015-04-20 18:09:54]

majnemer@chromium.org changed reviewers:
+ cpu@chromium.org, thakis@chromium.org

[Nico, 2015-04-20 18:11:16]

lgtm, nice work tracking this down!

[Nico, 2015-04-20 18:11:40]

(ima check cq so that this gets try runs; cpu please stamp at your convenience)

[Nico, 2015-04-20 18:11:43]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-04-20 18:12:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1077893005/1

[Nico, 2015-04-20 18:25:10]

https://codereview.chromium.org/1077893005/diff/1/sandbox/win/src/broker_serv...
File sandbox/win/src/broker_services.cc (left):

https://codereview.chromium.org/1077893005/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:427: HANDLE inherit_handle_list[2];
Now that I look at this again, this guy has the same problem.

[Nico, 2015-04-20 18:26:44]

Maybe we should change StartupInformation::UpdateProcThreadAttribute() to make a
copy of its argument and fix this once and for all?

[majnemer, 2015-04-20 18:56:36]

On 2015/04/20 18:26:44, Nico wrote:
> Maybe we should change StartupInformation::UpdateProcThreadAttribute() to make
a
> copy of its argument and fix this once and for all?

StartupInfomration would have to hold a variable amount of stuff.  Furthermore,
|AppContainerAttributes::ShareForStartup|
(https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/ap...)
doesn't seem to have a 1-to-1 relationship between the lpValue and the
StartupInformation.

[Nico, 2015-04-20 19:02:10]

lgtm, let's hope this is all of these then. I did an audit of all other calls to
UpdateProcThreadAttribute and didn't see anything else that's obviously wrong,
but I didn't look super closely.

(If you address a comment, it's good to click the "Done" box – else reviewers
won't know that you uploaded a new patch set.)

[majnemer, 2015-04-20 21:08:54]

https://codereview.chromium.org/1077893005/diff/1/sandbox/win/src/broker_serv...
File sandbox/win/src/broker_services.cc (left):

https://codereview.chromium.org/1077893005/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:427: HANDLE inherit_handle_list[2];
On 2015/04/20 18:25:10, Nico wrote:
> Now that I look at this again, this guy has the same problem.

Done.

[Nico, 2015-04-22 16:37:43]

thakis@chromium.org changed reviewers:
+ jln@chromium.org

[Nico, 2015-04-22 16:37:43]

jln, can you stamp this? rvargas and jschuh are OOO this month, and cpu is
apparently still catching up on things after being OOO last week.

The background here:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686880%28v=vs.85%2...
says """A pointer to the attribute value. This value should persist until the
attribute is destroyed using the DeleteProcThreadAttributeList function.""" We
didn't get that right. It used to not be an issue, but a compiler change makes
the compiler reuse the same stack space for different variables as long as the
variables don't have overlapping lifetimes (if they're in different local
blocks, say), and that compiler change exposes these bugs.

[jln (very slow on Chromium), 2015-04-22 17:27:36]

lgtm, thanks for the fix

+wfh FYI

[Nico, 2015-04-22 17:29:05]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2015-04-22 17:30:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1077893005/20001

[Will Harris, 2015-04-22 17:37:03]

I sort of agree with #8 that we shoudl encapsulate this inside
StartupInformation class otherwise this is likely to just happen again.  Perhaps
we can pass a scoped ptr into updateProcThreadAttribute method and keep track of
it that way?

[Will Harris, 2015-04-22 17:38:12]

On 2015/04/22 17:37:03, Will Harris wrote:
> I sort of agree with #8 that we shoudl encapsulate this inside
> StartupInformation class otherwise this is likely to just happen again. 
Perhaps
> we can pass a scoped ptr into updateProcThreadAttribute method and keep track
of
> it that way?

or... at least, add a comment in startup_information.h to explain that the
arguments must remain valid until the call of the destructor.

[jln (very slow on Chromium), 2015-04-22 17:52:20]

A refactor seems like a good idea. But how about letting this be committed and
split a better "lifetime" refactor to its own task? This bug seems like a pain.

[Nico, 2015-04-22 17:59:00]

On 2015/04/22 17:52:20, jln wrote:
> A refactor seems like a good idea. But how about letting this be committed and
> split a better "lifetime" refactor to its own task? This bug seems like a
pain.

This should definitely land quickly as it blocks a clang change (which I had
reverted thinking it caused miscompiles, but actually our chrome code was
buggy).

It's not clear to us how a better refactor would look like since attributes can
have arbitrary types (ScopedVariant maybe)?

[Will Harris, 2015-04-22 18:00:02]

I'm happy with a comment and a TODO being added to startup_information.h to warn
about this, and landing the CL as is.

[commit-bot: I haz the power, 2015-04-22 18:38:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-22 18:39:55]

Patchset 2 (id:??) landed as
https://crrev.com/a9f5526d0568508ab6e4913e23d3c8f33d040010
Cr-Commit-Position: refs/heads/master@{#326347}