Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Description • 10 years ago

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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 1 • 10 years ago

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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 2 • 10 years ago

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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 3 • 10 years ago

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.

	Mike Hommey [:glandium]
	Comment 4 • 10 years ago

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.

	Mike Hommey [:glandium]
	Comment 5 • 10 years ago

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.

	Guillaume Destuynder [:kang] [this account is no longer active]
	Comment 6 • 10 years ago

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")

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 7 • 10 years ago

(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?

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 8 • 10 years ago

(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.

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 9 • 10 years ago

(In reply to Jed Davis [:jld] from comment #8)
> on B2G.

…or Fennec.

	Mike Hommey [:glandium]
	Comment 10 • 10 years ago

(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 :)

	Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Assignee
	Comment 11 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3bfc3c2791
https://hg.mozilla.org/integration/mozilla-inbound/rev/d06d1a469bb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/059b2c8bffa8

And filed bug 1104342.

	Carsten Book [:Tomcat]
	Comment 12 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0b3bfc3c2791
https://hg.mozilla.org/mozilla-central/rev/d06d1a469bb1
https://hg.mozilla.org/mozilla-central/rev/059b2c8bffa8