1119878 - IPC child process startup leaks parent process handles into the sandbox

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jim Mathies [:jimm]]

See bug 1117140, this bug should block sandbox roll out as it exposes a parent process handle to untrusted content in the sandbox.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We'll have to totally rework the way 'child opens' protocols are handled to avoid needing the handle...

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It looks as if we could avoid needing duplicate permission on the handle if we refcountedly share a single handle instead.  Does removing that permission bit suffice to make the handle not a trivial sandbox escape?

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #1)
> We'll have to totally rework the way 'child opens' protocols are handled to
> avoid needing the handle...

We'd also need to forbid protocols from sending FileDescriptor or Shmem from child to parent, if I understand correctly.

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

During CritSmash, we noticed that whiteboard indicated that this would block shipping the sandbox. It was decided to mark this as also blocking bug 1123755 due to that fact. If incorrect, please feel free to remove.

Comment 4

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: Flash is being sandboxed using our own sandbox on beta, so I assume this affects it.  I'm not nominating this for tracking for beta because it sounds like the plan is to not actually ship it there, but of course it would be good to patch the vulnerability in code we're shipping to our beta users.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking, if this is intended to ship to Beta 37 users we should get an assignee here.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Ni? on module peers to get info on the plan here and who might be able to take this on.

Comment 7

[Benjamin Smedberg]

I think we can something similar for plugin processes that we did for bug 1117140: I don't think there are child shmems or file descriptor issues for plugins.

Comment 8

[Bob Owen (:bobowen)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> I think we can something similar for plugin processes that we did for bug
> 1117140: I don't think there are child shmems or file descriptor issues for
> plugins.

I'm not particularly clear on how the IPC stuff hangs together for NPAPI plugins, but that seems correct to me.

The two places where we seem to open a process handle for NPAPI are at [1] and [2].
[1] is the original one that was changed for bug 1117140, it is a handle to the chrome process.
[2] is, I'm pretty sure, only used for e10s and is a handle to the content process.

Both handles end up at [3], which goes into generated code where it is stored and, from what I can tell, is only used for logging purposes.

I've tried changing [1] and [2] so that the handle is null and flash seems to play fine for both non-e10s and e10s.

So, I'll raise a separate bug for NPAPI processes.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#460
[2] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleChild.cpp#112
[3] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleChild.cpp#214

Comment 9

[Andrew McCreight [:mccr8]]

With the new bug for NPAPI, I think we don't need to track this any more, as we're back to this only affecting e10s, I think.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Unless we can figure out a safe way to dupe handles from child to parent that doesn't allow expanded rights on Windows we're in for a really rough time with Shmem I think... This will need a big chunk of time, and some Windows expertise.

Comment 11

[Jim Mathies [:jimm]]

This doesn't block desktop e10s since security sandboxing of content process is a separate project.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #10)
> Unless we can figure out a safe way to dupe handles from child to parent
> that doesn't allow expanded rights on Windows we're in for a really rough
> time with Shmem I think... This will need a big chunk of time, and some
> Windows expertise.

For what it's worth: We're already going to run into some trouble with Shmems on Linux, since creating the shmem in the child process opens a temporary file, and the graphics code does this (and, incidentally, segfaults on a null pointer if shmem creation fails, possibly because nobody tested those error cases).  This was avoided for GMP by having the child do an intr call to the parent to request creation of the shmem; there was also some work there with caching the shmems (to avoid very frequent creation/deletion) which may be relevant.

It's not an absolute blocker, because for Linux content processes we're already going to have to do system call interception / brokering, which means the approach taken in http://keithp.com/blogs/chromium-dri3/ / https://crbug.com/415681 could be used — basically the same thing as those child→parent intr calls, except without needing to modify any of the graphics code.

Nonetheless, if child-side Shmem creation had to go away, that would not be a bad thing.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Since this is trunk only right now and we're not shipping this soon, I'm marking this tracking minus for 38.

Comment 14

[Bob Owen (:bobowen)]

I think I may have a solution for this.
It involves changing all the IPDL to hold ProcessId instead of ProcessHandle.

Then using a mechanism in the sandbox TargetServices to duplicate the handle to the desired process using the pid of the target process.

I have a PoC of this working where I'm using the pid to duplicate the shared memory from a SharedDIBSurface from a plugin process to the content process.

Unfortunately, this won't solve Jed's problem in Comment 12 as that is to do with the creation of the actual shared memory itself.
From what I can tell in Chromium on Linux they call the broker to created the shared memory, but not on Windows.

I don't think getting rid of the Process Handles in the IPDL code is such a bad thing either as when you get child process crashes (or possibly normal shutdowns), you seem to get a lot of orphaned Process Handles in the chrome process.

It will also fix the IPDL logging for windows regarding the process IDs, at the moment you get garbage because it tries to log the handles.

Comment 15

[Bob Owen (:bobowen)]

Getting there:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82c7f1d21f40

Nobody uses Macs, do they? :-)

Comment 16

[Bob Owen (:bobowen)]

Almost there:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=81488dca6a8c

The B2G failure was a stupid error.

Comment 17

[Bob Owen (:bobowen)]

Linux and B2G:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f64a4daeefa

OSX:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9616679f399a

Comment 18

[Bob Owen (:bobowen)]

Attachment: Part 1: Change SandboxTarget to hold sandbox target services to provide functions.

aklotz - for the sandboxing changes
glandium - for the build file changes
cpearce - for the GMP changes

This is just a little bit of refactoring to hold the Chromium sandbox target services in SandboxTarget, so that we can expose DuplicateHandle from it.

Comment 19

[Bob Owen (:bobowen)]

Attachment: Part 2: Change IPC code to hold ProcessID instead of ProcessHandle.

This is the main fix.
The patch applies cleanly at the moment, but bitrots quite quickly.

It changes the IPDL generated code to hold ProcessId instead of ProcessHandle, with all of the resulting repercussions. Including opening process handles when required.

Also changes the main ipc/glue code that uses DuplicateHandle to take a ProcessId instead, so that code can call a new mozilla::ipc::DuplicateHandle that should work in the sandboxed processes as well.

I decided not to add the checks we discussed on windows for high |ProcessId|s.
As I was writing it I realised that as these were in theory valid Process IDs on Windows, the only value that would cause us a problem is the kSameProcessId (0 - <current pid>), which is the one we would be explicitly allowing anyway.
In reality as these are 32 bit unsigned integers the chance of this ever causing a problem seem very slim anyway.

Some other notes below, most of which I think we discussed on Friday.

* Note about how the old code worked by chance: INVALID_HANDLE_VALUE (kInvalidProcessHandle) and the pseudo handle returned from ::GetCurrentProcess have the same numeric value. While this may inadvertently help, by for example automagically duplicating a handle to the correct process (maybe), it might also cause problems like waiting on or killing the current process by mistake.

* Introduced a new kSameProcessId as well as kInvalidProcessId, so we can be sure we're not just picking up the default value and duplicating, etc. to the wrong process.

* Old logging code used to log the other process handle directly, which was fine on posix but basically gave you a random number on Windows. This now works.

* |mChildProcessId|s in CompositorParent, ImageBridgeParent and LayerTransactionParent are only used as a key to a map for holding |ImageBridgeParent|s as far as I can tell. Changing all that to use OtherPid() should give the same outcome. The only difference is when OtherPid() is now the negative of the current PID, but this shouldn't matter for the map.

* GeckoChildProcessHost::SetAlreadyDead was leaking process handles on Windows.

* PluginModuleChromeParent::TerminateChild was leaking process handles for the flash processes on Windows.

Comment 20

[Bob Owen (:bobowen)]

Attachment: Part 3: Use ProcessID for CloneToplevel.

This could probably be moved to another bug, it's slightly related as it is to do with ProcessHandles / ProcessIds.

ProcessHandles are passed around for all this cloning, but they are actually ProcessIds, it just doesn't make a difference on posix.
On top of that they are nearly always used as ProcessIds anyway, through GetProcId.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Al, this blocks bug 1128559 which we are tracking for 37. 
Are we ok deferring bug 1128559 also, or what are the options here?

Comment 22

[Andrew McCreight [:mccr8]]

Bug 1128559 is a special case of this bug.  We don't really need to fix either for 37, as it shouldn't affect code we're shipping there: we don't sandbox NPAPI or e10s content processes using this.

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8578095 [details] [diff] [review]
Part 1: Change SandboxTarget to hold sandbox target services to provide functions.

Review of attachment 8578095 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/chromium-shim/base/MissingBasicTypes.h
@@ +6,5 @@
> +
> +// These types are still used by the Chromium sandbox code. When referencing
> +// Chromium sandbox code from Gecko we can't use the normal base/basictypes.h as
> +// it clashes with the one from ipc/chromium/src/base/. These types have been
> +// removed from the one in ipc/chromium/src/base/.

Nit: can you move this comment to just above the first typedef?

Comment 24

[Bob Owen (:bobowen)]

Thanks all, for the reviews.

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #23)
> Comment on attachment 8578095 [details] [diff] [review]
> Part 1: Change SandboxTarget to hold sandbox target services to provide

> > +// These types are still used by the Chromium sandbox code. When referencing
> > +// Chromium sandbox code from Gecko we can't use the normal base/basictypes.h as
> > +// it clashes with the one from ipc/chromium/src/base/. These types have been
> > +// removed from the one in ipc/chromium/src/base/.
> 
> Nit: can you move this comment to just above the first typedef?

Moved in local version.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8578106 [details] [diff] [review]
Part 2: Change IPC code to hold ProcessID instead of ProcessHandle.

Review of attachment 8578106 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Sorry it took me so long. I just have a few questions.

You probably explained this already, but why do we need kSameProcessId? Or, why does it need to be a special negative value? Can't we just set it to the actual process ID?

::: dom/ipc/ContentParent.cpp
@@ -1720,4 @@
>      }
> -    else {
> -        // we need to close the existing handle before setting a new one.
> -        base::CloseProcessHandle(OtherProcess());

I'm confused by the existing code. Why would we ever get OnChannelConnected when we already have a process handle?

@@ -2151,5 @@
>          extraArgs.push_back("-nuwa");
>      }
>      mSubprocess->LaunchAndWaitForProcessHandle(extraArgs);
>  
> -    Open(mSubprocess->GetChannel(), mSubprocess->GetOwnedChildProcessHandle());

It looks like we can get rid of GetOwnedChildProcessHandle?

@@ +3288,5 @@
> +    if (!base::OpenProcessHandle(OtherPid(), &otherProcessHandle)) {
> +        NS_ERROR("Failed to open child process when attempting kill.");
> +        return;
> +    }
> +        

Whitespace.

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +1206,5 @@
>  #endif
>  
> +    mozilla::ipc::ScopedProcessHandle geckoChildProcess;
> +    bool childOpened = base::OpenProcessHandle(OtherPid(),
> +                                               &(geckoChildProcess.rwget()));

Don't think you need the parens. Looks like this happens in a bunch of other places.

::: ipc/chromium/src/base/process_util_win.cc
@@ +73,5 @@
>                                           PROCESS_QUERY_INFORMATION |
>                                           SYNCHRONIZE,
>                                       FALSE, pid);
>  
> +  if (result == NULL || result == INVALID_HANDLE_VALUE)

What's this for?

::: ipc/glue/BackgroundImpl.cpp
@@ +1956,5 @@
>      return NS_OK;
>    }
>  
> +  // Make sure the parent knows it is same process.
> +  parentActor->SetOtherProcessId(kSameProcessId);

Doesn't the actor's Open method already set the PID to kSameProcessId? It seems like this line is unnecessary, but I worry I'm missing something?

::: ipc/ipdl/ipdl/lower.py
@@ +2959,5 @@
>  
>          self.cls.addstmts([ dtor, Whitespace.NL ])
>  
>          if ptype.isToplevel():
>              # Open(Transport*, ProcessHandle, MessageLoop*, Side)

Comment needs to be updated.

::: widget/nsBaseWidget.cpp
@@ +1101,5 @@
>    mCompositorChild = new CompositorChild(lm);
>    mCompositorChild->Open(parentChannel, childMessageLoop, ipc::ChildSide);
>  
> +  // Make sure the parent knows it is same process.
> +  mCompositorParent->SetOtherProcessId(kSameProcessId);

Same here about the need for SetOtherProcessId. It seems unnecessary.

Comment 26

[Bob Owen (:bobowen)]

(In reply to Bill McCloskey (:billm) from comment #25)
> Comment on attachment 8578106 [details] [diff] [review]

In addition to some questions below, do you have any advice on reviewers?
dom/ipc/ - 
dom/media/gmp/ - cpearce (as he reviewed first patch)
dom/plugins/ipc/ - 
gfx/ipc/ - 
gfx/layers/apz/ - 
gfx/layers/ipc/ - (maybe the same as gfx/ipc, not sure it there is someone who can cover all gfx)
ipc/ - I think you suggested dvander (possibly also for gfx, but he doesn't seem to be a peer at the moment)
toolkit/xre/ - 
widget/ - 

> You probably explained this already, but why do we need kSameProcessId? Or,
> why does it need to be a special negative value? Can't we just set it to the
> actual process ID?

Just as a safety precaution.
On Windows the old/current code (probably unwittingly) relies on the fact that kInvalidProcessHandle (INVALID_HANDLE_VALUE) happens to be the same value as the pseudo handle returned from GetCurrentProcess(), both are -1.
This meant that when this was passed into DuplicateHandle it happened to do the right thing and duplicate to the current process.
This wouldn't be the case, if it were used for killing or waiting on a process.
There is some checking for that at the moment, e.g. in mozilla::ipc::FatalError.

If we used the actual process ID as the other pid we might get into the same situation.
But maybe these sorts of bugs would be obvious and we'd want to see them instead of it failing (possibly silently) in some way.
What do you think?


> ::: dom/ipc/ContentParent.cpp
> @@ -1720,4 @@
> >      }
> > -    else {
> > -        // we need to close the existing handle before setting a new one.
> > -        base::CloseProcessHandle(OtherProcess());
> 
> I'm confused by the existing code. Why would we ever get OnChannelConnected
> when we already have a process handle?

Yeah, that looked odd to me.
In fact if we ever did have a process handle here, this may have been dangerous, if there were a chance that something else may have already closed the handle.

This is one of the reasons why I think we're better off holding and passing around pids rather than handles anyway.
We can open handles when we need to and that makes it easier to manage their lifetime.

> > -    Open(mSubprocess->GetChannel(), mSubprocess->GetOwnedChildProcessHandle());
> 
> It looks like we can get rid of GetOwnedChildProcessHandle?

Oh yeah, thanks.
Also, given what I said above, I think we might be able to get rid of the GetChildProcessHandle as well.
Most of the consumers, after this change, seem to want the pid anyway and ChildProcessHandle() in PluginModuleParent doesn't appear to be used at all.
But that can wait for a follow up I think.

> @@ +3288,5 @@
> > +    if (!base::OpenProcessHandle(OtherPid(), &otherProcessHandle)) {
> > +        NS_ERROR("Failed to open child process when attempting kill.");
> > +        return;
> > +    }
> > +        
> 
> Whitespace.

Thanks.

> ::: dom/plugins/ipc/PluginModuleParent.cpp
> @@ +1206,5 @@
> >  #endif
> >  
> > +    mozilla::ipc::ScopedProcessHandle geckoChildProcess;
> > +    bool childOpened = base::OpenProcessHandle(OtherPid(),
> > +                                               &(geckoChildProcess.rwget()));
> 
> Don't think you need the parens. Looks like this happens in a bunch of other
> places.

It was to make it totally clear, but maybe the extra ones are actually more visually confusing, I'll remove.

> ::: ipc/chromium/src/base/process_util_win.cc
> @@ +73,5 @@
> >                                           PROCESS_QUERY_INFORMATION |
> >                                           SYNCHRONIZE,
> >                                       FALSE, pid);
> >  
> > +  if (result == NULL || result == INVALID_HANDLE_VALUE)
> 
> What's this for?

This is actually an existing bug, OpenProcess returns NULL when it fails not INVALID_HANDLE_VALUE (I left that in just in case, maybe I should remove it).
So, it always returns true even if it has actually failed.

Interestingly, when Jim tried to fix this as part of bug 1117140, it caused test failures.
I think these might have been caused by us passing kInvalidProcessHandle from OtherProcess() at times.
These were probably caused by the next bits ...

> ::: ipc/glue/BackgroundImpl.cpp
> @@ +1956,5 @@
> >      return NS_OK;
> >    }
> >  
> > +  // Make sure the parent knows it is same process.
> > +  parentActor->SetOtherProcessId(kSameProcessId);
> 
> Doesn't the actor's Open method already set the PID to kSameProcessId? It
> seems like this line is unnecessary, but I worry I'm missing something?

We've only called Open on the child, as far as I could tell it never gets called on the parent in this particular case.
Although maybe this is a existing bug and Open should be called?

> ::: ipc/ipdl/ipdl/lower.py
> @@ +2959,5 @@
> >  
> >          self.cls.addstmts([ dtor, Whitespace.NL ])
> >  
> >          if ptype.isToplevel():
> >              # Open(Transport*, ProcessHandle, MessageLoop*, Side)
> 
> Comment needs to be updated.

Done, thanks.

> ::: widget/nsBaseWidget.cpp
> @@ +1101,5 @@
> >    mCompositorChild = new CompositorChild(lm);
> >    mCompositorChild->Open(parentChannel, childMessageLoop, ipc::ChildSide);
> >  
> > +  // Make sure the parent knows it is same process.
> > +  mCompositorParent->SetOtherProcessId(kSameProcessId);
> 
> Same here about the need for SetOtherProcessId. It seems unnecessary.

Like before, I don't think Open is called on the parent in this same process usage.

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bob Owen (:bobowen) from comment #26)
> (In reply to Bill McCloskey (:billm) from comment #25)
> > Comment on attachment 8578106 [details] [diff] [review]
> 
> In addition to some questions below, do you have any advice on reviewers?
> dom/ipc/ - 
> dom/media/gmp/ - cpearce (as he reviewed first patch)
> dom/plugins/ipc/ - 
> gfx/ipc/ - 
> gfx/layers/apz/ - 
> gfx/layers/ipc/ - (maybe the same as gfx/ipc, not sure it there is someone
> who can cover all gfx)
> ipc/ - I think you suggested dvander (possibly also for gfx, but he doesn't
> seem to be a peer at the moment)
> toolkit/xre/ - 
> widget/ - 

I can review the dom/ipc, ipc/, and toolkit/ changes. dvander can review gfx/ and widget/. He's not a peer, but I don't think anyone will mind. It's really all IPC code.

> If we used the actual process ID as the other pid we might get into the same
> situation.
> But maybe these sorts of bugs would be obvious and we'd want to see them
> instead of it failing (possibly silently) in some way.
> What do you think?

Yeah, I would prefer if kSameProcessId was just the actual process ID.

> > ::: ipc/glue/BackgroundImpl.cpp
> > @@ +1956,5 @@
> > >      return NS_OK;
> > >    }
> > >  
> > > +  // Make sure the parent knows it is same process.
> > > +  parentActor->SetOtherProcessId(kSameProcessId);
> > 
> > Doesn't the actor's Open method already set the PID to kSameProcessId? It
> > seems like this line is unnecessary, but I worry I'm missing something?
> 
> We've only called Open on the child, as far as I could tell it never gets
> called on the parent in this particular case.
> Although maybe this is a existing bug and Open should be called?

Oh, I see, I didn't read carefully enough. Both seem good then.

Comment 28

[Bob Owen (:bobowen)]

Comment on attachment 8578115 [details] [diff] [review]
Part 3: Use ProcessID for CloneToplevel.

I'm going to move this to another bug as it's only marginally related and I don't want it to slow down this bug.

Comment 29

[Bob Owen (:bobowen)]

Attachment: Part 1: Change SandboxTarget to hold sandbox target services to provide functions.

r=aklotz, r=glandium, r=cpearce - from comment 23 and below

Comment addressed and rebased, in case anyone wants to apply both parts.

Comment 30

[Bob Owen (:bobowen)]

Attachment: Part 2: Change IPC code to hold ProcessID instead of ProcessHandle.

dom/ipc/ - billm
dom/media/gmp/ - cpearce
dom/plugins/ipc/ - billm
gfx/ - dvander
ipc/ - billm
security/sandbox/ - aklotz
toolkit/xre/ - billm
widget/ - dvander

Wasn't sure if you meant yourself for dom/plugins/ipc/ as well, if not maybe Jim Mathies would be a good choice.

Aaron, as well as the sandbox policy changes, I wonder if you wouldn't mind looking at the Windows specific bits as well.
These are mainly under ipc/ and some bits under dom/plugins/ipc/ as well.

Thanks all.

Comment 31

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8584656 [details] [diff] [review]
Part 2: Change IPC code to hold ProcessID instead of ProcessHandle.

Review of attachment 8584656 [details] [diff] [review]:
-----------------------------------------------------------------

A few nits, but ok as far as the scope of my review is concerned.

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +508,5 @@
>      if (mAsyncInitRv != NS_ERROR_NOT_INITIALIZED || mShutdown) {
>          return;
>      }
> +
> +    Open(mSubprocess->GetChannel(), 

Nit: trailing space

@@ +1223,5 @@
> +    if (mFlashProcess1 &&
> +        base::OpenProcessHandle(mFlashProcess1, &flashBrokerProcess.rwget())) {
> +        processHandles.AppendElement(flashBrokerProcess);
> +    }
> +    mozilla::ipc::ScopedProcessHandle flashChildProcess;

Can you name this flashSandboxProcess instead? That would be more consistent with Adobe's nomenclature.

::: ipc/chromium/src/base/process_util_win.cc
@@ +73,5 @@
>                                           PROCESS_QUERY_INFORMATION |
>                                           SYNCHRONIZE,
>                                       FALSE, pid);
>  
> +  if (result == NULL) {

Good catch!

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

peterv: Heads up, I imagine this will bitrot your patches in bug 1057908.

Comment 33

[Peter Van der Beken [:peterv]]

(In reply to Chris Pearce (:cpearce) from comment #32)
> peterv: Heads up, I imagine this will bitrot your patches in bug 1057908.

Yup, thanks. Doesn't look like it'll be too hard to deal with.

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8584656 [details] [diff] [review]
Part 2: Change IPC code to hold ProcessID instead of ProcessHandle.

Review of attachment 8584656 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/ProtocolUtils.h
@@ +62,5 @@
>  const base::ProcessHandle kInvalidProcessHandle = INVALID_HANDLE_VALUE;
>  #else
>  const base::ProcessHandle kInvalidProcessHandle = -1;
>  #endif
> +const base::ProcessId kInvalidProcessId = -1;

It looks like ProcessId is an unsigned type on Windows. Maybe stick a cast here and document the assumption. I found a random stack overflow question that said process IDs in Windows are divisible by 4, so -1 should be okay.

::: ipc/glue/Shmem.cpp
@@ +618,5 @@
>  {
>    AssertInvariants();
>  
> +  // kSameProcessId is used to indicate that it's the same process.
> +  if (aTargetPid == kSameProcessId) {

I don't think we need this anymore.

Comment 35

[Bob Owen (:bobowen)]

(In reply to Bill McCloskey (:billm) from comment #34)
> Comment on attachment 8584656 [details] [diff] [review]
> Part 2: Change IPC code to hold ProcessID instead of ProcessHandle.
> 
> Review of attachment 8584656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/ProtocolUtils.h
> @@ +62,5 @@
> >  const base::ProcessHandle kInvalidProcessHandle = INVALID_HANDLE_VALUE;
> >  #else
> >  const base::ProcessHandle kInvalidProcessHandle = -1;
> >  #endif
> > +const base::ProcessId kInvalidProcessId = -1;
> 
> It looks like ProcessId is an unsigned type on Windows. Maybe stick a cast
> here and document the assumption. I found a random stack overflow question
> that said process IDs in Windows are divisible by 4, so -1 should be okay.

Yeah, apparently it's because the same code that allocates kernel handles is used for process and thread IDs.
Kernel handles have to be divisible by four, because the lowest two bits sometimes get used as extra pseudo-parameters. So, they are ignored generally.

It is not absolutely guaranteed to be the case for process IDs in the future, but I think we're reasonably safe to rely on this one particular case.
I'll add a cast and comment to explain this.
 
> ::: ipc/glue/Shmem.cpp
> @@ +618,5 @@
> >  {
> >    AssertInvariants();
> >  
> > +  // kSameProcessId is used to indicate that it's the same process.
> > +  if (aTargetPid == kSameProcessId) {
> 
> I don't think we need this anymore.

That's assuming that this code should know that kSameProcessId is actually the current process ID.

I thought about removing |kSameProcessId| all together and just replacing it with |base::GetCurrentProcId()|, but thought that if we decided that the old value (i.e. 0 - base::GetCurrentProcId()), was in fact a good idea it would be easy to change back.

That option could possibly break down pretty quickly though, so you'd have to check all the places where it should have been used anyway.
Maybe I'm better off just getting rid of kSameProcessId.

What do you think?

Comment 36

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> Maybe I'm better off just getting rid of kSameProcessId.

I think it's fine to keep it and just think of it as a cached copy of the process ID.

Comment 37

[Bob Owen (:bobowen)]

(In reply to Bill McCloskey (:billm) from comment #36)
> > Maybe I'm better off just getting rid of kSameProcessId.
> 
> I think it's fine to keep it and just think of it as a cached copy of the
> process ID.

OK, thanks.
I'd imagine normally that the compiler does that for you, but with the Chromium abstraction I don't think it would.

I'll remove the uses of kSameProcessId, where it becomes irrelevant given that specific meaning.
I think there's at least one other.

Comment 38

[Bob Owen (:bobowen)]

Thanks to all for the reviews.

Here's a full try push and one after fixing the constructor that had become implicit:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d32d187e416a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb7c0c104e56

I've previously done pushes to try with e10s enabled for Windows and the same things pass/fail as on holly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66eed0d97b60

Patches have changed a bit, but not much.

Comment 39

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6790a442f39a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aee0f61516c5

I forgot to mention that I change kSameProcessId to kCurrentProcessId, as I thought it was clearer, given that other areas of code assume that it really is the current pid.

After reading the sec-approval process, I decided this didn't need sec-approval as e10s is only available on Nightly and given the current strength of the content sandbox, this bug makes no difference at the moment.
We've also effectively already disclosed this bug anyway for the GMP process.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6790a442f39a
https://hg.mozilla.org/mozilla-central/rev/aee0f61516c5

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Apparently this broke b2g somehow, I think it's going to have to be backed out. CC'ing gwagner and fabrice.

Comment 42

[Gregor Wagner [:gwagner]]

It caused bug 1150565.

Comment 43

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I also commented on bug 1150565, but for the historical record: on B2G, child processes are forked by Nuwa, so caching the value of getpid() would require refreshing that cache when it changes.  Fortunately, bug 1149971 removed that part of the patch, and seems to have thereby fixed bug 1150565.

Comment 44

[Bob Owen (:bobowen)]

Thanks Jed.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Does this affect older B2G releases?

Comment 46

[Bob Owen (:bobowen)]

This handle problem was Windows only.

I don't know if any of those b2g versions include a Windows desktop version.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Nope, thanks :)

Comment 48

[Matt Wobensmith [:mwobensmith][:matt:]]

Post-CritSmash triage, setting flag for qe-verify-.

https://securitywiki.mozilla.org/PostCritSmash