1102195 - Update security/sandbox/chromium/ to Chromium stable channel version 40.0.2214.111

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

In bug 1041775, we updated all base, all Windows, and most of the Linux Chromium sandboxing code to 25/7/14.

In order to update to the latest code we have some other dependencies, which I'll file blocking this bug.

I'll also file a tracker (blocking this bug) related to getting all of the Chromium sandboxing code into the same structure as the Chromium repository.

Comment 1

[Bob Owen (:bobowen)]

Chromium moved over to using nullptr instead of NULL in base/memory/scoped_ptr.h in commit	36a3e81140bab724c8cbae234ac46174935ed38f.

It also uses |decltype(nullptr)|, which our nullptr emulation for gcc-4.4 doesn't seem to handle.

I did get it to compile by force including:

const                        // this is a const object...
class {
public:
  template<class T>          // convertible to any type
    operator T*() const      // of null non-member
    { return 0; }            // pointer...
  template<class C, class T> // or any type of null
    operator T C::*() const  // member pointer...
    { return 0; }
private:
  void operator&() const;    // whose address can't be taken
} nullptr = {};


but I think it's probably best to wait until we move everything to compilers that support nullptr.

Comment 2

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

Move process sandboxing bugs to the new Bugzilla component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

Comment 3

[Bob Owen (:bobowen)]

Try runs narrowing down posix issues.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68c141da3c3f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c748e82c98a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40c581efa7db
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a64619c6b05

Comment 4

[Bob Owen (:bobowen)]

Looking OK with the latest patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bea764e8edc

I think the Mulet failures are unrelated.
I'll get the patches up for review tomorrow.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 1: Update Chromium sandbox code to commit df7cc6c04725630dd4460f29d858a77507343b24.

This will bring us up to the latest stable channel release.
Stable seems like the safest thing to track as this should have had the most testing.

I'll upload the Windows only update and the Posix changes on top of those as separate patches to help with reviews.
This is the two folded together otherwise it would be a changeset with broken builds.

Notes:
* base/hash.* and base/third_party/superfasthash/* required by security/sandbox/chromium/base/win/scoped_handle.cc

* base/numerics/safe_conversions* required by security/sandbox/chromium/sandbox/win/src/policy_engine_opcodes.h

* removed #ifdef around MakeCheckOpString functions in logging.cpp shim.

* On linux/B2G, I had to pull in the new bpf_dsl\* files because they are used by codegen. This then meant pulling in quite a few base files. A lot of these were posix/linux version of windows files that we already have.

* Had to change to use the real version of thread_local_storage.h instead of our shim for B2G.

* Also had to add a few MOZ_IMPLICITs to pass static analysis on Linux.
Do either of you know if there is a way to do this without modifying the code?
I'll have to break these out into a separate patch before checking in.

Comment 6

[Bob Owen (:bobowen)]

Attachment: (Not for Checkin) Part 1 windows: Windows changes.

Comment 7

[Bob Owen (:bobowen)]

Attachment: (Not for Checkin) Part 1 posix: Posix changes on top of Windows changes.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8561474 [details] [diff] [review]
Part 1: Update Chromium sandbox code to commit df7cc6c04725630dd4460f29d858a77507343b24.

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

rs=me for Windows bits.

(In reply to Bob Owen (:bobowen) from comment #5)
> * Also had to add a few MOZ_IMPLICITs to pass static analysis on Linux.
> Do either of you know if there is a way to do this without modifying the
> code?

You mean like disabling that check for code imported from upstream? No idea, sorry.

Comment 9

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

(In reply to Bob Owen (:bobowen) from comment #5)
> * On linux/B2G, I had to pull in the new bpf_dsl\* files because they are
> used by codegen. This then meant pulling in quite a few base files. A lot of
> these were posix/linux version of windows files that we already have.

Thank you!  Once this bug lands I have plans to convert SandboxFilter.cpp to use bpf_dsl, so now I have less work to do for that.

> * Also had to add a few MOZ_IMPLICITs to pass static analysis on Linux.
> Do either of you know if there is a way to do this without modifying the
> code?

I'm not sure; there's nothing immediately obvious in the part of build/clang-plugin/clang-plugin.cpp that checks for it, but we control the plugin so there should be some way to add it.  (Surely this isn't the only third-party import with this problem?)

Comment 10

[Bob Owen (:bobowen)]

(In reply to Jed Davis [:jld] from comment #9)

> > * Also had to add a few MOZ_IMPLICITs to pass static analysis on Linux.
> > Do either of you know if there is a way to do this without modifying the
> > code?
> 
> I'm not sure; there's nothing immediately obvious in the part of
> build/clang-plugin/clang-plugin.cpp that checks for it, but we control the
> plugin so there should be some way to add it.  (Surely this isn't the only
> third-party import with this problem?)

glandium informed me that you can add "ENABLE_CLANG_PLUGIN :=" to Makefile.in.
But that will disable it for all the code including our own at the moment, so I'll stick with the MOZ_IMPLICITs.

I also realised that I can't break those out into a separate patch otherwise we'd have broken builds for that changeset.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 2: Re-apply pre-vista stdout/err process inheritance change to Chromium code after merge. r=tabraldes

Carrying the original review for this patch from Bug 1083701 history item 1 (#h1).

Comment 12

[Bob Owen (:bobowen)]

Attachment: Part 3: Re-apply logging changes to the Chromium interception code. r=tabraldes

Carrying the original review for this patch from Bug 928044 Comment 30.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 4: Re-apply - Change a non-conforming usage of a const value type to a non-const value type, which VS2015 rightly rejects. r=bobowen

Carrying the original review for this patch from Bug 1119072 Comment 50.

A forinfo rather than needinfo:

Brian - just to let you know I'll be re-landing this patch, in case you see your name in the push logs and wonder what's going on.
The other patch, Part 3(b), was no longer needed (for hash_set/map) as we already knew.
I've successfully compiled sandboxbroker.dll and wow_helper.exe with VC2015, so it doesn't look like the updates brought in any new problems.

Tim - again just in case you see your name as reviewer for the previous two parts.

Comment 14

[Bob Owen (:bobowen)]

PGO builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=549433e6180d

And another fairly limited one, to make sure I haven't screwed things up since the last full one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbdbbf702b7b

Comment 15

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/38d12afdc7b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/efa7518e43b0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/421446ab2347
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/beb74056ac2d

Comment 16

[Bob Owen (:bobowen)]

Sorry about the after the fact needinfo here.

I'd forgotten that patch Part 1 has a new licence file in it that I meant to ask you about or at least bring to your attention.
The file (and a read me from Chromium) is (will be) here:

security/sandbox/chromium/base/third_party/superfasthash/

I needed to bring in this code because it is now used by their windows scoped handle implementation.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38d12afdc7b1
https://hg.mozilla.org/mozilla-central/rev/efa7518e43b0
https://hg.mozilla.org/mozilla-central/rev/421446ab2347
https://hg.mozilla.org/mozilla-central/rev/beb74056ac2d

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

> Tim - again just in case you see your name as reviewer for the previous two
> parts.

OK thanks for the heads up

Comment 19

[Gervase Markham [:gerv]]

(In reply to Bob Owen (:bobowen) (PTO 12-19 Feb) from comment #16)
> I'd forgotten that patch Part 1 has a new licence file in it that I meant to
> ask you about or at least bring to your attention.
> The file (and a read me from Chromium) is (will be) here:
> 
> security/sandbox/chromium/base/third_party/superfasthash/

Assuming we are shipping this code with Firefox, this license text needs adding to about:license (toolkit/content/license.html). Please file a bug and do that work, CCing me. Make sure you add it in alphabetical order, as "superfasthash License".

Gerv

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #17)
> https://hg.mozilla.org/mozilla-central/rev/38d12afdc7b1

Part of this changeset is replacing the OVERRIDE macro with the actual C++ keyword "override" (which we're not assuming we have available everywhere yet, AFAIK).

(This is the first bustage that mwargers hits in a local build with gcc 4.6, though he's not sure how far this build actually gets without this error; this is the first time he's built with gcc 4.6 in a while, in an old VM.  According to https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code , gcc 4.6 is the minimum version that we require, so we should not bust it.)

Comment 21

[Daniel Holbert [:dholbert]]

So we probably need to replace all of the "override" annotations that this cset added with "MOZ_OVERRIDE" (#defined in http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h ), so as not to break GCC 4.6.

bobowen, could you take care of that in a followup?

Comment 22

[Daniel Holbert [:dholbert]]

(For the benefit of anyone testing possible-fixes with gcc 4.6 -- mwargers also said "I already had to change security/pkix/warnings.mozbuild to remove '-pedantic-errors'" -- I suspect that might be needed to fix one instance of earlier bustage in a gcc 4.6 build, but I'm not sure)

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> So we probably need to replace all of the "override" annotations that this
> cset added with "MOZ_OVERRIDE" (#defined in
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h ), so as not
> to break GCC 4.6.
> 
> bobowen, could you take care of that in a followup?

In security/pkix, we just "#define override" for GCC 4.6 only. Might be a simpler change.

(In reply to Daniel Holbert [:dholbert] from comment #22)
> (For the benefit of anyone testing possible-fixes with gcc 4.6 -- mwargers
> also said "I already had to change security/pkix/warnings.mozbuild to remove
> '-pedantic-errors'" -- I suspect that might be needed to fix one instance of
> earlier bustage in a gcc 4.6 build, but I'm not sure)

Yes. There is a already a bug filed for disabling -pedantic-errors when building with GCC <4.8.

Comment 24

[Bob Owen (:bobowen)]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> So we probably need to replace all of the "override" annotations that this
> cset added with "MOZ_OVERRIDE" (#defined in
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h ), so as not
> to break GCC 4.6.
> 
> bobowen, could you take care of that in a followup?

Sure bug 1136040 filed.
I only hope that they haven't used lots of other features or I'll likely have to back this out.
It's a bit annoying that none of our automated builds catch this sort of thing.

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #23)

> In security/pkix, we just "#define override" for GCC 4.6 only. Might be a
> simpler change.

Thanks, I much prefer this to having to sprinkle MOZ_OVERRIDE etc. all over the place.