Move Linux desktop sandboxing code into plugin-container

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

(Blocks 1 bug)

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Following bug 1088488, the Linux sandbox implementation should also move into plugin-container on desktop.  (On B2G we can have content processes running from the "b2g" executable as well as from plugin-container, because of Nuwa, but bug 1088488 doesn't apply to B2G.)

The parent process uses the part of the sandboxing code that determines what system-level features we can use (and what parts are disabled by environment variables and/or build configuration), so that part needs to be separated out.  This can live in libxul, while everything else moves into plugin-container.  Because content process sandboxing doesn't go through GMPLoader, it now needs to call functions in the opposite direction of linking; one solution might be to use the same weak symbol technique as mozglue/MFBT.
r?(kang) for sandboxing, r?(glandium) for build config.

The goal here, as mentioned in the commit message, is for the code that checks for sandbox support status to live in place that's common to parent and child processes, but not requiring an external function call to obtain the information.

No functional change intended.
Attachment #8526407 - Flags: review?(mh+mozilla)
Attachment #8526407 - Flags: review?(gdestuynder)
r?(kang) for sandboxing, r?(glandium) for build config.

This use of weak symbols seems to work on desktop, even with LD_BIND_NOW, and passes Try.
Attachment #8526408 - Flags: review?(mh+mozilla)
Attachment #8526408 - Flags: review?(gdestuynder)
And here's what this leads up to: moving LinuxSandboxStarter and everything it calls into plugin-container, like on Windows.

r?(cpearce) for media, r?(glandium) for yet more build config.  I don't know if I need more review for touching plugin-container like this.
Attachment #8526410 - Flags: review?(mh+mozilla)
Attachment #8526410 - Flags: review?(cpearce)
Attachment #8526407 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8526408 [details] [diff] [review]
Step 2: Move Linux sandbox code into plugin-container on desktop.

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

::: security/sandbox/linux/moz.build
@@ +8,5 @@
>  
> +if CONFIG['OS_TARGET'] == 'Android':
> +    SharedLibrary('mozsandbox')
> +else:
> +    Library('sandbox_static')

No need to call this 'sandbox_static'. 'mozsandbox' works just as well.

@@ +9,5 @@
> +if CONFIG['OS_TARGET'] == 'Android':
> +    SharedLibrary('mozsandbox')
> +else:
> +    Library('sandbox_static')
> +    FORCE_STATIC_LIB = True

I'm surprised this doesn't barf. Please don't put this, it's not required.
Attachment #8526408 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8526410 [details] [diff] [review]
Step 3: Move GMP's LinuxSandboxStarter into plugin-container.

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

::: ipc/app/moz.build
@@ +67,5 @@
>      ]
>  
> +if CONFIG['MOZ_SANDBOX'] and CONFIG['OS_TARGET'] == 'Android':
> +    USE_LIBS += [
> +        'mozsandbox',

If you change the name to 'mozsandbox' in part 2, then both conditions can be merged.
Attachment #8526410 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8526407 [details] [diff] [review]
Step 1: Move sandbox status info into a separate module.

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

Looks good. (also just as note in case I ever re-read this: what was called "sandbox features" is now called "sandbox info")
Attachment #8526407 - Flags: review?(gdestuynder) → review+
Attachment #8526410 - Flags: review?(cpearce) → review+
(In reply to Mike Hommey [:glandium] from comment #4)
> ::: security/sandbox/linux/moz.build
> @@ +8,5 @@
> >  
> > +if CONFIG['OS_TARGET'] == 'Android':
> > +    SharedLibrary('mozsandbox')
> > +else:
> > +    Library('sandbox_static')
> 
> No need to call this 'sandbox_static'. 'mozsandbox' works just as well.

If I do that and don't clobber, the build will dynamically link against the old libmozsandbox.so instead.  Is this expected?  Should I just update CLOBBER, or also file a bug?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #5)
> > +if CONFIG['MOZ_SANDBOX'] and CONFIG['OS_TARGET'] == 'Android':
> > +    USE_LIBS += [
> > +        'mozsandbox',
> 
> If you change the name to 'mozsandbox' in part 2, then both conditions can
> be merged.

This can actually be removed — changing the ifdef to MOZ_GMP_SANDBOX means this dependency doesn't exist on B2G.  When/if it does, this may not be the right approach.
(In reply to Jed Davis [:jld] from comment #8)
> on B2G.

…or Fennec.
(In reply to Jed Davis [:jld] from comment #7)
> If I do that and don't clobber, the build will dynamically link against the
> old libmozsandbox.so instead.  Is this expected?  Should I just update
> CLOBBER, or also file a bug?

Do both :)
Flags: needinfo?(mh+mozilla)
Depends on: 1104835

Updated

4 years ago
Blocks: 1121410
You need to log in before you can comment on or make changes to this bug.