Use ScopedProcessInformation and other RAII types in sandbox.

Use ScopedProcessInformation and other RAII types in sandbox.

See http://codereview.chromium.org/9700038/ for the definition of ScopedProcessInformation.

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130716

[erikwright (departed), 2012-03-30 16:29:31]

Hi Carlos,

The changes to sandbox/ got a bit more significant, so I broke them out and I'd
like you to take another look at them.

Please note that I added a bunch of notes to you in patchset 1. Patchset 2 is
only a single incremental use of ScopedHandle so I recommend that you first
review patchset 1 for the comments, and then patchset 2.

Thanks,

Erik

http://codereview.chromium.org/9959018/diff/1/sandbox/src/broker_services.cc
File sandbox/src/broker_services.cc (left):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/broker_services.cc#...
sandbox/src/broker_services.cc:277: if ((INVALID_HANDLE_VALUE ==
process_info.hProcess) ||
This check was redundant. It only protects you if somehow TargetProcess::Create
returns ERROR_SUCCESS from one of its failure modes. If one wishes to protect
against this type of bug, I suggest instead converting TargetProcess::Create to
return a boolean, and potentially take a DWORD* for returning platform error
codes.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/broker_services.cc#...
sandbox/src/broker_services.cc:301: 0, FALSE, DUPLICATE_SAME_ACCESS))
The responsibility for duplicating is now in TargetProcess::Create

http://codereview.chromium.org/9959018/diff/1/sandbox/src/broker_services.cc
File sandbox/src/broker_services.cc (right):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/broker_services.cc#...
sandbox/src/broker_services.cc:257: if (ERROR_SUCCESS != win_result)
Previously lockdown_token and initial_token could leak here (and in the next
early return).

http://codereview.chromium.org/9959018/diff/1/sandbox/src/broker_services.cc#...
sandbox/src/broker_services.cc:281: if (ERROR_SUCCESS != win_result)
Previously job could be leaked here and in the following early return.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/broker_services.cc#...
sandbox/src/broker_services.cc:294: return SpawnCleanup(target, 0);
tracker was previously leaked here.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/interception_unitte...
File sandbox/src/interception_unittest.cc (right):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/interception_unitte...
sandbox/src/interception_unittest.cc:88: TargetProcess *target =
MakeTestTargetProcess(current_process.Take(),
Previously this would have resulted in a call to CloseHandle() with the special
GetCurrentProcess  handle

http://codereview.chromium.org/9959018/diff/1/sandbox/src/interception_unitte...
sandbox/src/interception_unittest.cc:175: base::win::ScopedHandle
current_process;
ditto

http://codereview.chromium.org/9959018/diff/1/sandbox/src/job_unittest.cc
File sandbox/src/job_unittest.cc (right):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/job_unittest.cc#new...
sandbox/src/job_unittest.cc:186: EXPECT_TRUE(::CloseHandle(job_handle));
Previously process and thread handles were leaked.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/policy_target_test.cc
File sandbox/src/policy_target_test.cc (right):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/policy_target_test....
sandbox/src/policy_target_test.cc:153: base::win::ScopedProcessInformation
process_info;
Previously leaked.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/process_policy_test.cc
File sandbox/src/process_policy_test.cc (right):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/process_policy_test...
sandbox/src/process_policy_test.cc:52: FALSE, 0, NULL, NULL, &si, pi.Receive()))
{
Previously leaked handles.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/process_policy_test...
sandbox/src/process_policy_test.cc:77: NULL, NULL, FALSE, 0, NULL, NULL, &sia,
pi.Receive())) {
ditto.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/sandbox.h
File sandbox/src/sandbox.h (left):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/sandbox.h#oldcode72
sandbox/src/sandbox.h:72: //   handle and PID just as if CreateProcess() had
been called. The caller is
I think the parameter type change makes this self documenting.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/target_process.cc
File sandbox/src/target_process.cc (right):

http://codereview.chromium.org/9959018/diff/1/sandbox/src/target_process.cc#n...
sandbox/src/target_process.cc:202: if
(!target_info->DuplicateFrom(process_info)) {
Previously the caller (broker_services.cc) duplicated the process handle, but
not the thread handle.

content/common/sandbox_policy.cc assumed that it owned the thread handle and
closed it.

So implicitly, this method was returning a PROCESS_INFORMATION, transferring
ownership of hThread but not of hProcess.

Furthermore, in Init, hThread is referenced, so you have to be sure that Init is
called before the Close in sandbox_policy.

While it might not be strictly necessary to duplicate both hProcess and hThread,
it seems to remove some ambiguity.

http://codereview.chromium.org/9959018/diff/1/sandbox/src/target_process.cc#n...
sandbox/src/target_process.cc:321: // After this point we cannot use this handle
anymore.
I assume that this referred to the fact that sandbox_policy.cc might close the
HANDLE. Since it's now duplicated, is it actually meaningful to close it here,
or should we just let that happen in the destructor?

[erikwright (departed), 2012-03-30 16:30:34]

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.cc
File sandbox/src/target_process.cc (right):

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...
sandbox/src/target_process.cc:280: return ::GetLastError();
Theoretical leak of target_shared_section here.

[alexeypa (please no reviews), 2012-03-30 18:18:58]

A drive-by nit.

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.cc
File sandbox/src/target_process.cc (right):

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...
sandbox/src/target_process.cc:337:
target->sandbox_process_info_.Receive()->hProcess = process;
It is not good that the process ID is not set here as well (from a purist's
point of view). Practically though it should not be such a problem...

[erikwright (departed), 2012-03-30 18:36:52]

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.cc
File sandbox/src/target_process.cc (right):

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...
sandbox/src/target_process.cc:337:
target->sandbox_process_info_.Receive()->hProcess = process;
On 2012/03/30 18:18:59, alexeypa wrote:
> It is not good that the process ID is not set here as well (from a purist's
> point of view). Practically though it should not be such a problem... 

This whole function is kind of stinky. It creates a completely invalid
TargetProcess object and just mucks about with its state a bit to make things
hopefully work out.

I prefer not to get in the business of fixing it. At least not in this CL.

[alexeypa (please no reviews), 2012-03-30 18:42:10]

FYI

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.cc
File sandbox/src/target_process.cc (right):

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...
sandbox/src/target_process.cc:337:
target->sandbox_process_info_.Receive()->hProcess = process;
On 2012/03/30 18:36:52, erikwright wrote:
> This whole function is kind of stinky. It creates a completely invalid
> TargetProcess object and just mucks about with its state a bit to make things
> hopefully work out.
> 
> I prefer not to get in the business of fixing it. At least not in this CL.

Yeah, I don't think it should be fixed now.

[erikwright (departed), 2012-04-02 19:47:00]

Ping cpu@?

[cpu_(ooo_6.6-7.5), 2012-04-02 21:11:50]

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/sandbox.h
File sandbox/src/sandbox.h (right):

http://codereview.chromium.org/9959018/diff/2001/sandbox/src/sandbox.h#newcode87
sandbox/src/sandbox.h:87: base::win::ScopedProcessInformation* target) = 0;
I rather keep this interface, in fact this whole file free of base:: objects.
There are several reasons, including 3rd party users of this code.

But it is fine in other headers, I mean in inner classes. The classes here (plus
TargetPolicy) are the only external surface.

[erikwright (departed), 2012-04-03 14:03:27]

Hi Carlos,

I have reverted the changes in sandbox.h . I think this was the only change you
were concerned about, correct?

PTAL :)

[cpu_(ooo_6.6-7.5), 2012-04-04 00:25:23]

looking good, one more question.

http://codereview.chromium.org/9959018/diff/5022/sandbox/src/broker_services.cc
File sandbox/src/broker_services.cc (left):

http://codereview.chromium.org/9959018/diff/5022/sandbox/src/broker_services....
sandbox/src/broker_services.cc:305: ::GetCurrentProcess(), &dup_process_handle,
why remove the duplication of the handle?

[cpu_(ooo_6.6-7.5), 2012-04-04 00:29:39]

also please load the sandbox.sln solution, switch to 64 bit target, build and
run all tests (integration, validation and unit tests)

[erikwright (departed), 2012-04-04 02:30:40]

On 2012/04/04 00:25:23, cpu wrote:
> looking good, one more question.
> 
>
http://codereview.chromium.org/9959018/diff/5022/sandbox/src/broker_services.cc
> File sandbox/src/broker_services.cc (left):
> 
>
http://codereview.chromium.org/9959018/diff/5022/sandbox/src/broker_services....
> sandbox/src/broker_services.cc:305: ::GetCurrentProcess(),
&dup_process_handle,
> why remove the duplication of the handle?

It was pushed down into TargetProcess.

Please see this comment I made in target_process.cc:

http://codereview.chromium.org/9959018/diff/1/sandbox/src/target_process.cc#n...

Previously the caller (broker_services.cc) duplicated the process handle, but
not the thread handle.

content/common/sandbox_policy.cc assumed that it owned the thread handle and
closed it.

So implicitly, this method was returning a PROCESS_INFORMATION, transferring
ownership of hThread but not of hProcess.

Furthermore, in Init, hThread is referenced, so you have to be sure that Init is
called before the Close in sandbox_policy.

While it might not be strictly necessary to duplicate both hProcess and hThread,
it seems to remove some ambiguity.

[erikwright (departed), 2012-04-04 02:31:44]

On 2012/04/04 00:29:39, cpu wrote:
> also please load the sandbox.sln solution, switch to 64 bit target, build and
> run all tests (integration, validation and unit tests)

OK. Will do Wednesday AM.

[erikwright (departed), 2012-04-04 15:10:40]

On 2012/04/04 00:29:39, cpu wrote:
> also please load the sandbox.sln solution, switch to 64 bit target, build and
> run all tests (integration, validation and unit tests)

I tried to do this and received quite a few compilation errors. Are there any
special steps required to get compilation to work on 64 bit?

1>------ Build started: Project: base_static, Configuration: Debug x64 ------
1>Compiling...
1>pe_image.cc
1>.\win\pe_image.cc(18) : fatal error C1189: #error :  This code is not tested
on x64. Please make sure all the base unit tests pass before doing any real
work. The current unit tests don't test the differences between 32- and 64-bits
implementations. Bugs may slip through. You need to improve the coverage before
continuing.
...
2>base64.cc
2>g:\src\cr-git\src\third_party/modp_b64/modp_b64.h(137) : error C2220: warning
treated as error - no 'object' file generated
2>g:\src\cr-git\src\third_party/modp_b64/modp_b64.h(137) : warning C4267:
'argument' : conversion from 'size_t' to 'int', possible loss of data
...
3>.\src\sidestep\mini_disassembler.cpp(8) : fatal error C1189: #error :  The
code in this file should not be used on 64-bit Windows.
3>.\src\sidestep_resolver.cc(147) : error C2485: 'naked' : unrecognized extended
attribute
...
5>g:\src\cr-git\src\base/basictypes.h(319) : error C2118: negative subscript
5>        .\src\service_resolver_unittest.cc(76) : see reference to function
template instantiation 'Dest bit_cast<ULONG,void*>(const Source &)' being
compiled

[cpu_(ooo_6.6-7.5), 2012-04-04 20:01:53]

lgtm

About the compilation failures, I think the dependencies are foobared again. So
ignore that. I haven't done it for a while so now that I try it is pretty
broken.

[rvargas (doing something else), 2012-04-12 20:10:10]

On 2012/04/04 20:01:53, cpu wrote:
> lgtm
> 
> About the compilation failures, I think the dependencies are foobared again.
So
> ignore that. I haven't done it for a while so now that I try it is pretty
> broken.

Not exactly. GYP just doesn't support (yet!) building solutions as 32 or 64
bits. In order to build the 64 bit tests, you have to manually change the
dependencies of each executable from the 32 bit version of each library to the
64 bit version (base.lib to base_nacl_win64.lib) etc, and rebuild.

Note that ipc_unittest.cc is broken (from a long time ago) so it must be
excluded, and service_resolver_unittest.cc should be excluded (be design).

Please make sure there are no more regressions.