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

(Breaking this out of bug 1114647 comment #111.)

The expected rationale for bug 1101170 wound up not actually happening, and arranging the build of the Linux sandboxing code in that way has become a source of technical debt that makes it harder (sometimes much harder) to make other valuable changes to the build.

So let's undo it and put desktop back to the way it was, with the sandboxing code in a shared library that libxul and the various executables can link against as needed.

Most importantly, this will unblock bug 1114647, whose current goal is to make the "firefox" executable able to run content processes.  This will also remove a bunch of B2G ifdefs (B2G always used libmozsandbox.so, and has run content from fork()ed processes of the "b2g" executable for some time), and should take care of a bunch of the build complexity complaints collected in bug 1229136 and generally reduce the cognitive burden of dealing with this stuff.

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

This has a weird interaction with bug 1176099: moving the symbol interpositions out of the executable into libmozsandbox.so seems as if it should break them… except that if the executable is directly linked against libmozsandbox.so then it will still precede libc.so in the library search path, so the interception still works.  (Unless libc.so itself is LD_PRELOADed, but there probably isn't any reason to do that, and there's a workaround of preloading libmozsandbox.so first.  And an LD_PRELOAD library depending on libc.so is fine.)

So… in practice things should work, because plugin-container and firefox-as-plugin-container (after bug 1114647) will both need to link against libmozsandbox.  Quietly depending on that isn't ideal; it would be good to do a self-test of the interceptions (e.g., try to block SIGSYS and then read back the signal mask) in SandboxEarlyInit, and if not… maybe MOZ_DIAGNOSTIC_ASSERT and disable sandboxing otherwise.  But that could be a followup, I think.

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7302ca39f86

The Valgrind failure looks like a false positive and I'm not seeing how I would have caused it.  The artifact build failure *is* related to adding a new library, but I don't know what (if anything?) I need to do to make sure it doesn't break that way on not-Try.  The rest look like unrelated intermittents.

The dom/ipc/moz.build change isn't strictly necessary, because at least one other place makes libxul depend on libmozsandbox, but there is code in dom/ipc that calls into libmozsandbox and it seems like bad style (at best) to depend on some other directory's dependencies.

This is probably going to break comm-central the way the earlier patches that added/removed libmozsandbox did.

Review/feedback areas: sandboxing, build, media.

	Mike Hommey [:glandium]
	Comment 3 • 8 years ago

Comment on attachment 8748037 [details] [diff] [review]
bug1268733-restore-libmozsandbox-hg2.diff

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

::: dom/ipc/moz.build
@@ +116,5 @@
>  
>  FINAL_LIBRARY = 'xul'
>  
> +if CONFIG['MOZ_SANDBOX'] and \
> +   CONFIG['OS_TARGET'] in ('Darwin', 'Linux', 'Android'):

CONFIG['OS_ARCH'] in ('Darwin', 'Linux') would be shorter and test the same thing.

::: security/sandbox/linux/moz.build
@@ +102,5 @@
>          'rt',
>      ]
>  
>  USE_LIBS += [
>      'mfbt',

you're linking both mozglue and mfbt, mozglue containing mfbt, there's redundancy. That being said, linking mozglue is also wrong on linux, because it's also in the executable, so here you'll end up with duplication of mozglue, including jemalloc. Possibly, you're making the sandbox code use a different allocator than the everything else, with this. This could very well be the cause of the valgrind red, too.

@@ +103,5 @@
>      ]
>  
>  USE_LIBS += [
>      'mfbt',
> +    'nspr',

If the end goal is to link this to the firefox executable, a dependency on nspr is a no go.

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

(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #1)
> This has a weird interaction with bug 1176099: moving the symbol
> interpositions out of the executable into libmozsandbox.so seems as if it
> should break them… except that if the executable is directly linked against
> libmozsandbox.so then it will still precede libc.so in the library search
> path, so the interception still works.

I wouldn't be surprised if that doesn't work with e.g. musl or uclibc (and yes, there are people building firefox against those, don't ask me why). It's also fragile, even with glibc.

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

(In reply to Mike Hommey [:glandium] from comment #3)
> you're linking both mozglue and mfbt, mozglue containing mfbt, there's
> redundancy. That being said, linking mozglue is also wrong on linux, because
> it's also in the executable, so here you'll end up with duplication of
> mozglue, including jemalloc. Possibly, you're making the sandbox code use a
> different allocator than the everything else, with this. This could very
> well be the cause of the valgrind red, too.

Right; that's the one difference between the builds that's not changing (mozglue in executable vs. preloaded).  I'll leave the mozglue dependency under an Android conditional, and I'll see if the mfbt dependency is actually needed.

> @@ +103,5 @@
> >      ]
> >  
> >  USE_LIBS += [
> >      'mfbt',
> > +    'nspr',
> 
> If the end goal is to link this to the firefox executable, a dependency on
> nspr is a no go.

That can go away once bug 1035125 (specifically “Part 7”) lands, and bug 1035125 is a requirement for getting firefox to act as plugin-container on Windows anyway.

	Gian-Carlo Pascutto [:gcp]
	Comment 6 • 8 years ago

Comment on attachment 8748037 [details] [diff] [review]
bug1268733-restore-libmozsandbox-hg2.diff

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

::: security/sandbox/linux/common/SandboxInfo.h
@@ +62,5 @@
>    // single-threaded, to check that the SandboxEarlyInit() call in a
>    // child process is early enough to be single-threaded.  If not,
>    // kUnexpectedThreads is set and affected flags (user namespaces;
>    // possibly others in the future) are cleared.
> +  static MOZ_EXPORT void ThreadingCheck();

I'll defer to glandium for most of this review, but it is not obvious to me how this change relates to the rest of this bug.

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> ::: security/sandbox/linux/common/SandboxInfo.h
> @@ +62,5 @@
> >    // single-threaded, to check that the SandboxEarlyInit() call in a
> >    // child process is early enough to be single-threaded.  If not,
> >    // kUnexpectedThreads is set and affected flags (user namespaces;
> >    // possibly others in the future) are cleared.
> > +  static MOZ_EXPORT void ThreadingCheck();
> 
> I'll defer to glandium for most of this review, but it is not obvious to me
> how this change relates to the rest of this bug.

That function needs to be exported from libmozsandbox so that it can be called from libxul; previously it was linked into libxul.

(Yes, this means that security/sandbox/linux and security/sandbox/linux/common no longer need to be separate directories; I intend to merge them as a followup.)

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 8 • 8 years ago

Comment on attachment 8748037 [details] [diff] [review]
bug1268733-restore-libmozsandbox-hg2.diff

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

Doing this would mean we won't get Adobe Primetime EME on Linux. We're likely to get Widevine EME on Linux before we get Adobe EME on Linux, so we can probably just run with this in the meantime until Adobe start working on Linux EME support, and then figure it out.

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

(In reply to Chris Pearce (:cpearce) from comment #8)
> Doing this would mean we won't get Adobe Primetime EME on Linux. We're
> likely to get Widevine EME on Linux before we get Adobe EME on Linux, so we
> can probably just run with this in the meantime until Adobe start working on
> Linux EME support, and then figure it out.

So that's kind of a problem — if someone is going to have to come back and essentially revert this (and bug 1272062, and anything that happens later that doesn't respect the no-longer-existent divisions I set up) then it seems like this might not be such a good idea after all.

Also, this doesn't strictly need to block bug 1114647 — it's already going to be OS-dependent whether the firefox executable is used for content processes (OS X needs to continue using plugin-container), so that can land for Windows and leave Linux as a followup.  It's also not clear whether using plugin-container for content causes any e10s-release-blocker-worthy problems on Linux, as it does on Windows, so we might just not need to do it at all.

It's also not the only way to deal with making the browser executable able to act as a (sandboxed) child process — bug 1035125 added XREChildData for passing platform-specific values from the main executable into libxul via XRE_InitChildProcess, so the dependencies between libxul and libmozsandbox could all be routed through that instead of using symbol resolution.  That would also help clean up some of the B2G ifdef'ing and misuse of weak symbols and so on.

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

WONTFIXing on the assumption this will be done some other way (which I'm filing a bug for); can be reopened if that changes.

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

Plans have changed, and it looks like this will be doable after all.

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

Once this change is made, sandboxing code won't be able to reference anything defined in libxul, so the telemetry and crash annotation glue needs to be moved to libxul itself.

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

The interposition shims need to stay in the executable after mozsandbox becomes a separate shared library.  (I've checked with nm(1) that the symbol types/visibilities for gSeccompTsyncBroadcastSignum are correct.)

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

And this is the patch that makes the big change.  This should also address glandium's feedback about mfbt/mozglue from the last attempt.  There's no longer an NSPR dependency, thanks to :bobowen adjusting the imported Chromium code, and as for the possible overhead of loading libmozsandbox itself, it should be possible (now that Gonk is gone and bug 1088488 no longer applies) to move the mozsandbox dependencies out of ipc/contentproc and into libxul before attempting bug 1277968.

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33f21219d877fe976f8f95a3124462d2aaee0c33

	Mike Hommey [:glandium]
	Comment 16 • 8 years ago

Comment on attachment 8801363 [details] [diff] [review]
Step 2: Move sandbox interposition shims to their own static library.

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

::: ipc/app/moz.build
@@ +65,5 @@
>  
>  if CONFIG['MOZ_SANDBOX'] and CONFIG['OS_TARGET'] in ('Linux', 'Android'):
>      USE_LIBS += [
>          'mozsandbox',
> +        'mozsandbox_interpose',

Couldn't we also just have mozsandbox as a shared library, including the hooks, and have GeckoChildProcessHost set LD_PRELOAD to point to it?

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

(In reply to Mike Hommey [:glandium] from comment #16)
> Couldn't we also just have mozsandbox as a shared library, including the
> hooks, and have GeckoChildProcessHost set LD_PRELOAD to point to it?

Good point.  But I think I'll make that a followup bug.

	Pulsebot
	Comment 18 • 8 years ago

Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/774759c6861c
Move sandbox telemetry / crash annotation code from mozsandbox to libxul. r=haik
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcfb1c3b654
Move sandbox interposition shims to their own static library. r=gcp r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b07f481251c
Move Linux sandboxing code back out to libmozsandbox.so. r=gcp r=glandium

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/774759c6861c
https://hg.mozilla.org/mozilla-central/rev/7fcfb1c3b654
https://hg.mozilla.org/mozilla-central/rev/8b07f481251c