ScopedProcessInformation protects against process/thread handle leaks from CreateProcess calls.

ScopedProcessInformation protects against process/thread handle leaks from CreateProcess calls.

This CL includes the definition of the class along with changes to clients of CreateProcess.

Please see http://codereview.chromium.org/9959018/ for the most substantial usages I propose introducing.

This was motivated by realizing that my previous fix to LaunchProcess actually still had a leak of the thread handle.

BUG=None
TEST=tests added in base_unittests

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

[alexeypa (please no reviews), 2012-03-15 02:41:31]

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:105: static bool IsSame(HANDLE lhs, HANDLE rhs) {
We will be able to avoid changing all instances of handle traits classes when
making similar changes in the future if we define a base traits class. It should
implement the methods that are usually copy-pasted. Something like this:


template <class T>
class BaseHandleTraits {
 public:
  typedef T Handle;

  // Returns true if the handle value is valid.
  bool IsHandleValid(const T& handle) const {
    return handle != NullHandle();
  }

  bool IsSame(const T& left, const T& right) const {
    return left == right;
  }

  // Returns NULL handle value.
  T NullHandle() const {
    return NULL;
  }
};

class HandleTraits : public BaseHandleTraits<HANDLE> {
 public:

  // Closes the handle.
  bool CloseHandle(HANDLE handle) const {
    return ::CloseHandle(handle) != FALSE;
  }
};

GenericScopedHandle will need to be changed to use non-static methods of the
traits class, which should not be a problem.

[alexeypa (please no reviews), 2012-03-15 02:49:16]

More comments.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:21: static bool
CloseHandle(PROCESS_INFORMATION handle) {
You really want "PROCESS_INFORMATION& handle" here.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:27: handle = NullHandle();
|handle| is a local copy on the stack. This does not change the copy of the
structure contained by GenericScopedHandle.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:31: static bool
IsHandleValid(PROCESS_INFORMATION handle) {
const PROCESS_INFORMATION&

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:36: static bool IsSame(PROCESS_INFORMATION
lhs, PROCESS_INFORMATION rhs) {
const PROCESS_INFORMATION&

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:43: static PROCESS_INFORMATION
NullHandle() {
I don't think returning raw structures like this is the best practice. It is
legal, but not efficient. 

It is better to wrap a pointer to PROCESS_INFORMATION into GenericScopedHandle.

[grt (UTC plus 2), 2012-03-15 02:59:30]

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:51: class ScopedProcessInformation :
public GenericScopedHandle<ProcessInfoTraits> {
did you consider something like this:

class ScopedProcessInformation : public PROCESS_INFORMATION {
  ScopedProcessInformation() : hProcess(), hThread() { }
  ~ScopedProcessInformation() {
    if (hProcess)
      ::CloseHandle(hProcess);
    if (hTread)
      ::CloseHandle(hThread);
  }
  HANDLE TakeProcessHandle() { HANDLE p = hProcess; hProcess = NULL; return p; }
  HANDLE TakeThread() { ... }
  // etc...
};

then consumers don't need calls to Receive or Get, they just treat it like a
normal PROCESS_INFORMATION.  WDYT?

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

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:105: static bool IsSame(HANDLE lhs, HANDLE rhs) {
On 2012/03/15 02:41:31, alexeypa wrote:
> We will be able to avoid changing all instances of handle traits classes when
> making similar changes in the future if we define a base traits class. It
should
> implement the methods that are usually copy-pasted. Something like this:
> 
> 
> template <class T>
> class BaseHandleTraits {
>  public:
>   typedef T Handle;
> 
>   // Returns true if the handle value is valid.
>   bool IsHandleValid(const T& handle) const {
>     return handle != NullHandle();
>   }
> 
>   bool IsSame(const T& left, const T& right) const {
>     return left == right;
>   }
> 
>   // Returns NULL handle value.
>   T NullHandle() const {
>     return NULL;
>   }
> };
> 
> class HandleTraits : public BaseHandleTraits<HANDLE> {
>  public:
> 
>   // Closes the handle.
>   bool CloseHandle(HANDLE handle) const {
>     return ::CloseHandle(handle) != FALSE;
>   }
> };
> 
> GenericScopedHandle will need to be changed to use non-static methods of the
> traits class, which should not be a problem.

I thought about listing this as one of my proposed minor changes. I decided
against it after noting that HandleTraits would only actually use two of these
methods (IsSame and NullHandle).

Another option would be to use a bit of template magic here to use == unless
base::is_class<Handle>
(http://code.google.com/codesearch#OAMlx_jo-ck/src/base/template_util.h&exact_...)
in which case an IsSame method must be provided.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:21: static bool
CloseHandle(PROCESS_INFORMATION handle) {
On 2012/03/15 02:49:16, alexeypa wrote:
> You really want "PROCESS_INFORMATION& handle" here.

Well, the GenericScopedHandle is not expecting this method to modify the passed
Handle (c.f. the existing implementation that passes a HANDLE by value). But I
do agree that here and elsewhere, const references are correct.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:27: handle = NullHandle();
On 2012/03/15 02:49:16, alexeypa wrote:
> |handle| is a local copy on the stack. This does not change the copy of the
> structure contained by GenericScopedHandle.

Done.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:31: static bool
IsHandleValid(PROCESS_INFORMATION handle) {
On 2012/03/15 02:49:16, alexeypa wrote:
> const PROCESS_INFORMATION&

Done.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:36: static bool IsSame(PROCESS_INFORMATION
lhs, PROCESS_INFORMATION rhs) {
On 2012/03/15 02:49:16, alexeypa wrote:
> const PROCESS_INFORMATION&

Done.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:43: static PROCESS_INFORMATION
NullHandle() {
On 2012/03/15 02:49:16, alexeypa wrote:
> I don't think returning raw structures like this is the best practice. It is
> legal, but not efficient. 
> 
> It is better to wrap a pointer to PROCESS_INFORMATION into
GenericScopedHandle.

Hmm. Then the client needs to have both a PROCESS_INFORMATION on the stack and a
ScopedProcessInformation (or a ScopedProcessInformationReleaser, or something).

In general I expect the call to NullHandle to be wiped out during compilation of
the templates, but here I've offered an approach with a const reference and a
static null value. WDYT?

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:51: class ScopedProcessInformation :
public GenericScopedHandle<ProcessInfoTraits> {
On 2012/03/15 02:59:30, grt wrote:
> did you consider something like this:
> 
> class ScopedProcessInformation : public PROCESS_INFORMATION {
>   ScopedProcessInformation() : hProcess(), hThread() { }
>   ~ScopedProcessInformation() {
>     if (hProcess)
>       ::CloseHandle(hProcess);
>     if (hTread)
>       ::CloseHandle(hThread);
>   }
>   HANDLE TakeProcessHandle() { HANDLE p = hProcess; hProcess = NULL; return p;
}
>   HANDLE TakeThread() { ... }
>   // etc...
> };
> 
> then consumers don't need calls to Receive or Get, they just treat it like a
> normal PROCESS_INFORMATION.  WDYT?

WRT Get(), it could also just be process_handle() and thread_handle(), which is
not dramatically more than .hProcess and hThread.

Receive() has an additional protection which is that it requires the object to
currently be NULL. With public derivation of PROCESS_INFORMATION, you could do,
for example:

ScopedProcessInformation pi;
CreateProcess(..., &pi);
CreateProcess(..., &pi);

The first pi is lost. So this would really not be a ScopedX on par with all
other scoped_.<x> and ScopedX classes in Chromium.

[alexeypa (please no reviews), 2012-03-15 16:38:27]

A few more "const reference" comments.

This CL looks good to me. However I'm not a full committer yet so it is not
good. :-)

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:43: static PROCESS_INFORMATION
NullHandle() {
> It is better to wrap a pointer to PROCESS_INFORMATION into
GenericScopedHandle.

After a second thought, wrapping a pointer does not look like a such a good
idea. The code will not be very readable.

> here I've offered an approach with a const reference and a static null value.
WDYT?

I think this will work just fine. This should eliminate unnecessary copying of
the structure in the stack.

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:66: Set(process_info);
I would just modified the structure directly here avoiding the overhead of
Take() and Set().

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:76: process_info.dwThreadId = 0;
Same as above.

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:31: explicit GenericScopedHandle(Handle handle) :
handle_(Traits::NullHandle()) {
const Handle& handle

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:43: void Set(Handle handle) {
const Handle& handle

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:53: Handle Get() const {
Now that Handle is not a simple value this should return "const Handle&".

[sanjeevr, 2012-03-15 17:42:55]

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_process_info...
base/win/scoped_process_information.h:71: process_info.dwProcessId = 0;
Drive-by: Zeroing out the process id here (and the  thread id below) seems like
overkill. There is no reason why a consumer should not be able to access the
process id after taking ownership of the handle.

[grt (UTC plus 2), 2012-03-15 18:01:09]

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:51: class ScopedProcessInformation :
public GenericScopedHandle<ProcessInfoTraits> {
On 2012/03/15 03:29:17, erikwright wrote:
> On 2012/03/15 02:59:30, grt wrote:
> > did you consider something like this:
> > 
> > class ScopedProcessInformation : public PROCESS_INFORMATION {
> >   ScopedProcessInformation() : hProcess(), hThread() { }
> >   ~ScopedProcessInformation() {
> >     if (hProcess)
> >       ::CloseHandle(hProcess);
> >     if (hTread)
> >       ::CloseHandle(hThread);
> >   }
> >   HANDLE TakeProcessHandle() { HANDLE p = hProcess; hProcess = NULL; return
p;
> }
> >   HANDLE TakeThread() { ... }
> >   // etc...
> > };
> > 
> > then consumers don't need calls to Receive or Get, they just treat it like a
> > normal PROCESS_INFORMATION.  WDYT?
> 
> WRT Get(), it could also just be process_handle() and thread_handle(), which
is
> not dramatically more than .hProcess and hThread.
> 
> Receive() has an additional protection which is that it requires the object to
> currently be NULL. With public derivation of PROCESS_INFORMATION, you could
do,
> for example:
> 
> ScopedProcessInformation pi;
> CreateProcess(..., &pi);
> CreateProcess(..., &pi);
> 
> The first pi is lost. So this would really not be a ScopedX on par with all
> other scoped_.<x> and ScopedX classes in Chromium.

It might be a can of worms, but you could:
Handle* operator&() { ... }

I'll bow out now.

[erikwright (departed), 2012-03-15 18:02:43]

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3002/base/win/scoped_process_info...
base/win/scoped_process_information.h:66: Set(process_info);
On 2012/03/15 16:38:27, alexeypa wrote:
> I would just modified the structure directly here avoiding the overhead of
> Take() and Set().

It's a private member. I'd rather not make it protected, so this mildly
round-about approach seems OK.

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:31: explicit GenericScopedHandle(Handle handle) :
handle_(Traits::NullHandle()) {
On 2012/03/15 16:38:27, alexeypa wrote:
> const Handle& handle

Done.

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:43: void Set(Handle handle) {
On 2012/03/15 16:38:27, alexeypa wrote:
> const Handle& handle

Done.

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_handle.h#new...
base/win/scoped_handle.h:53: Handle Get() const {
On 2012/03/15 16:38:27, alexeypa wrote:
> Now that Handle is not a simple value this should return "const Handle&".

Done.

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_process_info...
File base/win/scoped_process_information.h (right):

http://codereview.chromium.org/9700038/diff/3007/base/win/scoped_process_info...
base/win/scoped_process_information.h:71: process_info.dwProcessId = 0;
On 2012/03/15 17:42:55, sanjeevr wrote:
> Drive-by: Zeroing out the process id here (and the  thread id below) seems
like
> overkill. There is no reason why a consumer should not be able to access the
> process id after taking ownership of the handle.

It's primarily to simplify the usage pattern. If someone calls
"TakeProcessHandle" and "TakeThreadHandle", they have taken everything and
therefore it's easier if the object is considered reset at that point.

The alternative introduces some weird states that I would rather not have to
deal with (or document).

[brettw, 2012-03-16 19:35:32]

I feel like deriving from GenericScopedHandle isn't really appropriate here.
It's not a handle, it's a random struct, and I don't thing GenericScopedHandle
really does anything that useful for you anyway. I'd just write a standalone
class that does what you need.

[cpu_(ooo_6.6-7.5), 2012-03-19 00:49:00]

I second brettw@ idea. I don't think this struct "is a" scoped handle. Just
looking at the traits you can see weirdness.

I also second grt@ idea deriving from PI is cleaner, specially combined with
brettw's proposal.

I don't think the
CreateProcess(..., &pi);
CreateProcess(..., &pi);

Is enough of a danger here.

[cpu_(ooo_6.6-7.5), 2012-03-19 01:00:03]

you missed content/common/sandbox_policy.cc

[erikwright (departed), 2012-03-28 19:30:07]

Hi Brett,

I removed the derivation from ScopedHandle.

In this revision, I chose not to derive from PROCESS_INFORMATION. In my opinion
it removes some of the encapsulation and safety that I think is the point of
this class, with the only clear benefit being that you don't need to call
receive and can access members directly (as opposed to via accessors).

brettw@, as the OWNER, if you feel that derivation from PROCESS_INFORMATION is
better I will update the CL accordingly.

PTAL.

Thanks,

Erik

[cpu_(ooo_6.6-7.5), 2012-03-29 02:39:55]

sandbox files lgtm.

[alexeypa (please no reviews), 2012-03-29 04:51:34]

LGTM for remoting.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
File base/win/scoped_process_information.cc (right):

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:27: process_information_.dwProcessId ||
process_information_.dwThreadId;
I think that it should test only handles but not the IDs. However you might find
it useful to DCHECK that both handle and ID are either null or not null.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:32:
::CloseHandle(process_information_.hThread);
It will be useful for debugging if NOTREACHED() will be invoked if CloseHandle
returns an error.

PS. There is such a check in ntdll.dll, but, I believe, it is not always active.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:38: PROCESS_INFORMATION
ScopedProcessInformation::Take() {
Now, since ScopedProcessInformation is not limited by GenericScopedHandle
interface it may be better to use Swap() semantic. I.e. somethink like:

void Swap(ScopedProcessInformation& other) {
  <swap_process_handle>;
  <swap_thred_handle>;
}

It surely depends on how Take() is used in the code and whether Take is more
convenient. Certainly ignore this comment if Take() is more convenient.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
File base/win/scoped_process_information_unittest.cc (right):

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information_unittest.cc:94: *other.Receive() =
process_info.Take();
Swap could be useful here: process_info.Swap(*other);

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

http://codereview.chromium.org/9700038/diff/11020/sandbox/src/target_process....
sandbox/src/target_process.cc:139: PROCESS_INFORMATION* target_info) {
target_info can also be converted to ScopedProcessInformation*.

http://codereview.chromium.org/9700038/diff/11020/sandbox/src/target_process....
sandbox/src/target_process.cc:218: *target_info = process_info.Take();
This is an example where Swap semantic could be clearer express what is
happening (assuming that target_info would also be ScopedProcessInformation).

[brettw, 2012-03-30 17:23:09]

This looks nice, I just have a few questions:

http://codereview.chromium.org/9700038/diff/28004/base/win/scoped_process_inf...
File base/win/scoped_process_information.cc (right):

http://codereview.chromium.org/9700038/diff/28004/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:69: void
ScopedProcessInformation::Swap(ScopedProcessInformation* other) {
Do you need this for anything? Unless you have a need I wouldn't bother writing
it, most of our classes don't have this kind of thing.

http://codereview.chromium.org/9700038/diff/28004/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:76: bool
ScopedProcessInformation::DuplicateFrom(
I'm also wondering about the use case for this function. Won't the normal use
for this class just be to wrap a CreateProcess call, and then you wouldn't
normally need to duplicate the handles around?

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

On 2012/03/30 17:23:09, brettw wrote:
> This looks nice, I just have a few questions:
> 
>
http://codereview.chromium.org/9700038/diff/28004/base/win/scoped_process_inf...
> File base/win/scoped_process_information.cc (right):
> 
>
http://codereview.chromium.org/9700038/diff/28004/base/win/scoped_process_inf...
> base/win/scoped_process_information.cc:69: void
> ScopedProcessInformation::Swap(ScopedProcessInformation* other) {
> Do you need this for anything? Unless you have a need I wouldn't bother
writing
> it, most of our classes don't have this kind of thing.
> 
>
http://codereview.chromium.org/9700038/diff/28004/base/win/scoped_process_inf...
> base/win/scoped_process_information.cc:76: bool
> ScopedProcessInformation::DuplicateFrom(
> I'm also wondering about the use case for this function. Won't the normal use
> for this class just be to wrap a CreateProcess call, and then you wouldn't
> normally need to duplicate the handles around?

Hi Brett,

Great questions, I was just tidying this up and adding a link to the separate CL
that uses these.

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

[erikwright (departed), 2012-03-30 17:30:27]

I separated the changes in sandbox/ and chrome/installer/ into new CLs as I
thought they both could have a lot of other points of discussion.

Please see http://codereview.chromium.org/9959018/ for the sandbox/ changes.
They are the most substantial and I think make the strong case for why this
class is useful.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
File base/win/scoped_process_information.cc (right):

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:27: process_information_.dwProcessId ||
process_information_.dwThreadId;
On 2012/03/29 04:51:35, alexeypa wrote:
> I think that it should test only handles but not the IDs. However you might
find
> it useful to DCHECK that both handle and ID are either null or not null. 

The only way for handle and id to not be in the same state is if someone does
*pi->Receive()->dwProcessId = X without also setting hProcess.

That's sufficiently unexpected that I'd prefer to maintain the otherwise
internal consistency that id and handle are set/checked/cleared/taken together.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:32:
::CloseHandle(process_information_.hThread);
On 2012/03/29 04:51:35, alexeypa wrote:
> It will be useful for debugging if NOTREACHED() will be invoked if CloseHandle
> returns an error.
> 
> PS. There is such a check in ntdll.dll, but, I believe, it is not always
active.

Done.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:38: PROCESS_INFORMATION
ScopedProcessInformation::Take() {
On 2012/03/29 04:51:35, alexeypa wrote:
> Now, since ScopedProcessInformation is not limited by GenericScopedHandle
> interface it may be better to use Swap() semantic. I.e. somethink like:
> 
> void Swap(ScopedProcessInformation& other) {
>   <swap_process_handle>;
>   <swap_thred_handle>;
> }
> 
> It surely depends on how Take() is used in the code and whether Take is more
> convenient. Certainly ignore this comment if Take() is more convenient.

Looking through your suggestions on the use of Swap led me to learn more about
sandbox/ than I had ever wanted, but it was a good suggestion.

For the moment I've left Take() here, even though I don't have an immediate
client for it, because it seems correct from a symmetry point of view. WDYT?

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
File base/win/scoped_process_information_unittest.cc (right):

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information_unittest.cc:94: *other.Receive() =
process_info.Take();
On 2012/03/29 04:51:35, alexeypa wrote:
> Swap could be useful here: process_info.Swap(*other);

Keeping this as an actual test of Take, but there is a separate new test for
Swap.

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

http://codereview.chromium.org/9700038/diff/11020/sandbox/src/target_process....
sandbox/src/target_process.cc:139: PROCESS_INFORMATION* target_info) {
On 2012/03/29 04:51:35, alexeypa wrote:
> target_info can also be converted to ScopedProcessInformation*.

Extracted to a new CL, see
http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...

http://codereview.chromium.org/9700038/diff/11020/sandbox/src/target_process....
sandbox/src/target_process.cc:218: *target_info = process_info.Take();
On 2012/03/29 04:51:35, alexeypa wrote:
> This is an example where Swap semantic could be clearer express what is
> happening (assuming that target_info would also be ScopedProcessInformation).

See
http://codereview.chromium.org/9959018/diff/2001/sandbox/src/target_process.c...

[brettw, 2012-03-30 17:31:33]

Okay, if you have a need for these, they're both fine. LGTM

[erikwright (departed), 2012-03-30 17:40:44]

On 2012/03/30 17:31:33, brettw wrote:
> Okay, if you have a need for these, they're both fine. LGTM

Thanks. I'll hold this until cpu@ reviews the changes in sandbox/, and if these
methods are deemed useful there I'll keep them. Else, I'll get rid of them.

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

Just an FYI.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
File base/win/scoped_process_information.cc (right):

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:27: process_information_.dwProcessId ||
process_information_.dwThreadId;
On 2012/03/30 17:30:27, erikwright wrote:
> The only way for handle and id to not be in the same state is if someone does
> *pi->Receive()->dwProcessId = X without also setting hProcess.

There is one example in the accompanying CL (the sandbox changes) that modified
hProcess without touching dwProcessId. I don't think it is a problem. It just
would be nice it we had some protection against such an inconsistency.

http://codereview.chromium.org/9700038/diff/11020/base/win/scoped_process_inf...
base/win/scoped_process_information.cc:38: PROCESS_INFORMATION
ScopedProcessInformation::Take() {
On 2012/03/30 17:30:27, erikwright wrote:
> For the moment I've left Take() here, even though I don't have an immediate
> client for it, because it seems correct from a symmetry point of view. WDYT?

The main reason to get rid of Take() is to reduce exposure of the naked
PROCESS_INFORMATION structure. Without Take there will be only Receive() and
individual HANDLE accessors (which seem to be safer because having a naked
HANDLE makes people think how to close it, while having a naked
PROCESS_INFORMATION structure - not so much).

[commit-bot: I haz the power, 2012-04-04 15:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/9700038/28004

[commit-bot: I haz the power, 2012-04-04 15:11:55]

Presubmit check for 9700038-28004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    remoting

Presubmit checks took 1.4s to calculate.

[simonmorris, 2012-04-04 15:38:33]

LGTM for remoting.

[commit-bot: I haz the power, 2012-04-04 15:39:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/9700038/28004

[commit-bot: I haz the power, 2012-04-04 16:46:52]

Try job failure for 9700038-28004 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...