Cleanup sandbox/ in advance of re-enabling some MSVC warnings.

Cleanup sandbox/ in advance of re-enabling some MSVC warnings.

In most files this means removing unused variables/code.

BUG=none
TEST=none

Committed: https://crrev.com/84995b8be698afc758352954fccd0bec5d971b65
Cr-Commit-Position: refs/heads/master@{#297931}

[Peter Kasting, 2014-09-29 23:09:24]

pkasting@chromium.org changed reviewers:
+ cpu@chromium.org

[Peter Kasting, 2014-09-29 23:09:26]

See comments below.  There are two places in this CL where a behavioral question
needs answering.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/policy_opcod...
File sandbox/win/src/policy_opcodes_unittest.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/policy_opcod...
sandbox/win/src/policy_opcodes_unittest.cc:292: // !!! Are we supposed to
Evaluate() this opcode and test the result?
This question needs answering before this CL can be committed.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/process_miti...
File sandbox/win/src/process_mitigations.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/process_miti...
sandbox/win/src/process_mitigations.cc:82: static const int
MEM_EXECUTE_OPTION_ENABLE = 1;
Making these static consts prevents MSVC from warning that some of them are
unused.  (Another option would be to simply remove the unused ones.)

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/process_miti...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/process_miti...
sandbox/win/src/process_mitigations_test.cc:168: static const int
MEM_EXECUTE_OPTION_ENABLE = 1;
See comments in previous file.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
sandbox/win/src/target_process.cc:99: // leaking it OK?
This question must be answered before this change can be committed.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/tests/validation...
File sandbox/win/tests/validation_tests/commands.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/tests/validation...
sandbox/win/tests/validation_tests/commands.cc:18: if (name == L"HKLM")
Chromium style: Do (var == const), not (const == var)

https://codereview.chromium.org/614813002/diff/1/sandbox/win/tests/validation...
sandbox/win/tests/validation_tests/commands.cc:20: if (name == L"HKCR")
Chromium style: No else after return

https://codereview.chromium.org/614813002/diff/1/sandbox/win/tests/validation...
sandbox/win/tests/validation_tests/commands.cc:181: if (current_desk == NULL)
File style: Consistently avoid {} on one-line bodies

[cpu_(ooo_6.6-7.5), 2014-09-30 17:57:27]

still working on it..

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/policy_opcod...
File sandbox/win/src/policy_opcodes_unittest.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/policy_opcod...
sandbox/win/src/policy_opcodes_unittest.cc:292: // !!! Are we supposed to
Evaluate() this opcode and test the result?
On 2014/09/29 23:09:25, Peter Kasting wrote:
> This question needs answering before this CL can be committed.

Yes, looks like somebody forgot to call evaluate() and check the result.

I assume line 295 will move above, to 289

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/process_miti...
File sandbox/win/src/process_mitigations.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/process_miti...
sandbox/win/src/process_mitigations.cc:82: static const int
MEM_EXECUTE_OPTION_ENABLE = 1;
On 2014/09/29 23:09:25, Peter Kasting wrote:
> Making these static consts prevents MSVC from warning that some of them are
> unused.  (Another option would be to simply remove the unused ones.)

Lets keep them. so static ok.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/tests/validation...
File sandbox/win/tests/validation_tests/commands.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/tests/validation...
sandbox/win/tests/validation_tests/commands.cc:18: if (name == L"HKLM")
On 2014/09/29 23:09:26, Peter Kasting wrote:
> Chromium style: Do (var == const), not (const == var)

Acknowledged.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/tests/validation...
sandbox/win/tests/validation_tests/commands.cc:20: if (name == L"HKCR")
On 2014/09/29 23:09:26, Peter Kasting wrote:
> Chromium style: No else after return

Acknowledged.

[cpu_(ooo_6.6-7.5), 2014-09-30 20:23:45]

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
sandbox/win/src/target_process.cc:99: // leaking it OK?
On 2014/09/29 23:09:26, Peter Kasting wrote:
> This question must be answered before this change can be committed.

The theory is that the job object has been closed and that caused the target
process (that sits at the other end of the ipc) to be killed. Now killing
processes is done via a kernel apc (async) so we should arrive at 91 with the
process dead, but GetExitCodeProcess is telling us that is not, thusly we enter
in panic mode and leak the ipc_server_ because we fear that if the target
somehow manages to do another ipc call we would crash the broker (browser)

A know case of this is if the process is killed while a thread is stuck doing IO
in the kernel, win2k dealt with this poorly as a supposedly dead process has a
thread that emerges from kernel to do damage.

So it is doing the right thing but I am not agreeing with my previous self about
the signals; since WaitForSingleObject could have just timed out and 5ms later
it would have been ok to call GetExitCodeProcess.  Maybe it would be better to
wait for 1500ms and just read the result from the wait, if is a timeout then
take this path.

[Peter Kasting, 2014-09-30 20:35:40]

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
sandbox/win/src/target_process.cc:99: // leaking it OK?
On 2014/09/30 20:23:45, cpu wrote:
> So it is doing the right thing but I am not agreeing with my previous self
about
> the signals; since WaitForSingleObject could have just timed out and 5ms later
> it would have been ok to call GetExitCodeProcess.  Maybe it would be better to
> wait for 1500ms and just read the result from the wait, if is a timeout then
> take this path.

It sounds like I should basically leave this code as-is, expand the comment, and
let it be up to you as to whether you want to make a further behavior change.

[cpu_(ooo_6.6-7.5), 2014-09-30 22:44:40]

sure

[Peter Kasting, 2014-10-01 02:32:13]

PTAL.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/policy_opcod...
File sandbox/win/src/policy_opcodes_unittest.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/policy_opcod...
sandbox/win/src/policy_opcodes_unittest.cc:292: // !!! Are we supposed to
Evaluate() this opcode and test the result?
On 2014/09/30 17:57:26, cpu wrote:
> On 2014/09/29 23:09:25, Peter Kasting wrote:
> > This question needs answering before this CL can be committed.
> 
> Yes, looks like somebody forgot to call evaluate() and check the result.

Fixed.

> I assume line 295 will move above, to 289

Sure, assuming we intend to test that if Evaluate() fails, it won't change the
provided value.  I added a few other such checks in here as well.

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
File sandbox/win/src/target_process.cc (right):

https://codereview.chromium.org/614813002/diff/1/sandbox/win/src/target_proce...
sandbox/win/src/target_process.cc:99: // leaking it OK?
On 2014/09/30 20:35:39, Peter Kasting wrote:
> It sounds like I should basically leave this code as-is, expand the comment,
and
> let it be up to you as to whether you want to make a further behavior change.

I attempted to clarify the comments here.

[Peter Kasting, 2014-10-02 18:16:23]

Ping cpu

[cpu_(ooo_6.6-7.5), 2014-10-02 22:25:08]

lgtm

[Peter Kasting, 2014-10-02 22:25:26]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-02 22:27:17]

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

[commit-bot: I haz the power, 2014-10-02 22:38:44]

Committed patchset #2 (id:20001) as ae18df994bc16f6d3f4261eebe51d35ccf3492f2

[commit-bot: I haz the power, 2014-10-02 22:39:25]

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