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.

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.

Move process sandboxing bugs to the new Bugzilla component. (Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

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

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.

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

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

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

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

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

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.

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

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

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.

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

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

(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

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

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?

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

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

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