Closed Bug 1035125 Opened 6 years ago Closed 4 years ago

On Windows, plugin-container.exe is linked against the sandbox_s library twice

Categories

(Core :: Security: Process Sandboxing, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

(Whiteboard: sbwc2)

Attachments

(9 files, 1 obsolete file)

58 bytes, text/x-review-board-request
benjamin
: review+
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
58 bytes, text/x-review-board-request
aklotz
: review+
Details
58 bytes, text/x-review-board-request
aklotz
: review+
Details
58 bytes, text/x-review-board-request
aklotz
: review+
glandium
: review+
cpearce
: feedback+
Details
58 bytes, text/x-review-board-request
aklotz
: review+
glandium
: review+
Details
Currently, plugin-container.exe is linked against the sandbox_s library twice. Once directly and also via xul.dll, which links against sandboxbroker.dll, which links in sandbox_s.

This currently causes some issues, that have to be worked around, where different copies of the code are hit when called from different places.

We should look into resolving this so we only have the library linked in once.
Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)
Component: Security → Security: Process Sandboxing
Whiteboard: sbwc2
This has become a bit more urgent as we're thinking of using firefox.exe for the content child process, which as I understand it means we need the chromium sandbox code linked into the exe for the memory patching to work.

After some code changes and moz.build file juggling, I was hitting [1] because the sandbox code uses malloc.

Then ensued a fairly lengthy disagreement with the linker/windows to try and persuade it to run this hooking code before loading msvcr120(d).dll.
Things were looking bad, particularly because dmajor had clearly already had serious words with the linker.

Then fortunately I did an hg pull and someone had broken the build for VS2013, so I discovered that VS2015 no longer needed this hack.

glandium - do you think we can just get rid of these checks (and in fact eventually all of this static linking) and just drop support for WinXP SP2 when built with VS2013?

[1] https://dxr.mozilla.org/mozilla-central/rev/1da1937a9e03154ae7c60089f2dcf5ad9ee20fa3/toolkit/xre/WindowsCrtPatch.h#126
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Flags: needinfo?(mh+mozilla)
Blocks: e10s-rename
That would be a sensible solution if we're sure we'll stick to VS2015... which is, unfortunately, not a given (see bug 1265615).
Flags: needinfo?(mh+mozilla)
George - here's the current version of my patch.
It's not thoroughly tested and it looks like it currently causes a shutdown crash.
Flags: needinfo?(gwright)
Depends on: vs2015
(In reply to Bob Owen (:bobowen) from comment #5)
> George - here's the current version of my patch.
> It's not thoroughly tested and it looks like it currently causes a shutdown
> crash.

This shutdown crash is because for the lazily constructed std::locale::facet the ctype<char> _Ctype._Table is getting allocated with _calloc_crt in _Getctype in _tolower.c.
But it is getting freed by jemalloc.

Not sure how to fix this yet.
Try push for this bug with a couple of patches on top that make the content process use firefox.exe:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3a9b0245df72c6aba979e1306cd3f9c0bebd1f

... and another with the leak logging fixed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e710870b3ff32abbbadd56993684a442979e78a7
And another hopefully fixing the leaks that were uncovered by fixing the leak logging \o/:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=feb55551c567
Before I upload patches, here are the issues I had with mismatches between allocation code.

Firstly I hit a problem on Windows 10 when compiled with VS2015, because of a silently failing version check in the sandbox code causing a std::ostream to be created, which ends up hitting line 2222 of [1]:

		_Ctype = _Lobj._Getctype();

after going through line 117 of [2] this lands at line 134 of [3].

In the _Getctype function _calloc_crt is used to allocate, which uses the CRT's own allocation, but this memory gets freed at line 2215 of [1], which just uses free, so this goes to jemalloc and we crash.

This looks like a bug in VS2015 to me, but fortunately I could take a patch to the chromium sandbox to get rid of this.

[1] c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xlocale
[2] c:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xlocinfo
[3] C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\crt\src\stl\_tolower.c
Flags: needinfo?(gwright)
Secondly I hit a problem when trying to compile with VS2013, where during start-up memory is allocated for environment variables at line 165 of [1].
It seems that _malloc_crt is just defined as malloc, so this goes to jemalloc.
When it comes to allocate more memory it hits line 249 of [1], which uses _recalloc_crt, which gets defined as _recalloc and ends up in [2] line 53 (_recalloc_base also gets defined as _recalloc).
I couldn't find a way to change this with the CRT static linking, so I resorted to removing all the static linking, which I think we wanted to do at some point anyway.

[1] C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\crt\src\setenv.c
[2] C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\crt\src\recalloc.c
The original changeset that is being backed out had comment:
Bug 1023941 - Part 5: Loader hook to redirect the missing import.

Review commit: https://reviewboard.mozilla.org/r/50451/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50451/
Attachment #8748662 - Flags: review?(benjamin)
Attachment #8748663 - Flags: review?(mh+mozilla)
Attachment #8748664 - Flags: review?(mh+mozilla)
Attachment #8748665 - Flags: review?(mh+mozilla)
Attachment #8748666 - Flags: review?(mh+mozilla)
Attachment #8748667 - Flags: review?(aklotz)
Attachment #8748668 - Flags: review?(aklotz)
Attachment #8748669 - Flags: review?(mh+mozilla)
Attachment #8748669 - Flags: review?(aklotz)
Attachment #8748670 - Flags: review?(mh+mozilla)
Attachment #8748670 - Flags: review?(aklotz)
The original changeset that is being backed out had comment:
Bug 1023941 - Part 4: Static-link the CRT into crashreporter.exe.

Review commit: https://reviewboard.mozilla.org/r/50453/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50453/
The original changeset that is being backed out had comment:
Bug 1023941 - Part 3: Static-link the CRT into plugin-hang-ui.exe.

Review commit: https://reviewboard.mozilla.org/r/50455/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50455/
The original changeset that is being backed out had comment:
Bug 1023941 - Part 2: Static-link the CRT into plugin-container.exe.

Review commit: https://reviewboard.mozilla.org/r/50457/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50457/
The original changeset that is being backed out had comment:
Bug 1023941 - Part 1: Static-link the CRT into firefox.exe.

Review commit: https://reviewboard.mozilla.org/r/50459/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50459/
Attachment #8744045 - Attachment is obsolete: true
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/1-2/
Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

https://reviewboard.mozilla.org/r/50451/#review47337

Please make this commit message much clearer: we can do this now because we're building with MSVC2015, and Firefox still runs on XPSP2 even without these changes.
Attachment #8748662 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #21)
> Comment on attachment 8748662 [details]
> MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and
> associated subsequent changes. r?bsmedberg
> 
> https://reviewboard.mozilla.org/r/50451/#review47337
> 
> Please make this commit message much clearer: we can do this now because
> we're building with MSVC2015, and Firefox still runs on XPSP2 even without
> these changes.

cf. comment 3, that may not stay this way.
Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

https://reviewboard.mozilla.org/r/50461/#review48141

rs=me
Attachment #8748667 - Flags: review?(aklotz) → review+
Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

https://reviewboard.mozilla.org/r/50463/#review48143
Attachment #8748668 - Flags: review?(aklotz) → review+
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50465/#review48149
Attachment #8748669 - Flags: review?(aklotz) → review+
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50467/#review48151

Sandbox bits look okay...
Attachment #8748670 - Flags: review?(aklotz) → review+
Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50453/#review48561
Attachment #8748663 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50455/#review48563
Attachment #8748664 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50457/#review48565

::: dom/media/gmp/rlz/moz.build:23
(Diff revision 1)
>      UNIFIED_SOURCES += [
>          'win/lib/machine_id_win.cc',
>      ]
>  
>  if CONFIG['OS_TARGET'] == 'Darwin':
> +    USE_STATIC_LIBS = True

USE_STATIC_LIBS has no effect on platforms other than Windows (and even on Windows, it's only when building with MSVC). No need to keep it here.

::: ipc/app/moz.build:17
(Diff revision 1)
>          'MozillaRuntimeMainAndroid.cpp',
>      ]
>  
>      DIRS += ['pie']
>  else:
> -    kwargs = {
> +    GeckoProgram(CONFIG['MOZ_CHILD_PROCESS_NAME'], linkage=None)

I think you can change linkage to 'dependent' here and remove the USE_LIBS for nspr and xul, while you're here.

::: ipc/app/moz.build:41
(Diff revision 1)
> +if CONFIG['OS_TARGET'] == 'WINNT':
> +    DISABLE_STL_WRAPPING = True

why?
Attachment #8748665 - Flags: review?(mh+mozilla)
Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50459/#review48583
Attachment #8748666 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50465/#review48587

::: ipc/contentproc/plugin-container.cpp:228
(Diff revision 1)
>  #if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK)
>      // On desktop, the GMPLoader lives in plugin-container, so that its
>      // code can be covered by an EME/GMP vendor's voucher.
>      nsAutoPtr<mozilla::gmp::SandboxStarter> starter(MakeSandboxStarter());
>      if (XRE_GetProcessType() == GeckoProcessType_GMPlugin) {
> -        loader = mozilla::gmp::CreateGMPLoader(starter);
> +        childData.gmpLoader.reset(mozilla::gmp::CreateGMPLoader(starter));

You should be able to use a "normal" assignment.

::: security/sandbox/win/SandboxInitialization.h:22
(Diff revision 1)
> +
> +static sandbox::TargetServices*
> +GetInitializedTargetServices()
> +{
> +  static sandbox::TargetServices* sInitializedTargetServices =
> +    []() -> sandbox::TargetServices* {

I'm not convinced using a lambda makes it particularly better than just using a separate function.

But why not make all this a static method of TargetServices ?

::: toolkit/xre/nsEmbedFunctions.cpp:317
(Diff revision 1)
>      MakeUnique<base::StatisticsRecorder>();
>  
>  #if !defined(MOZ_WIDGET_ANDROID) && !defined(MOZ_WIDGET_GONK)
>    // On non-Fennec Gecko, the GMPLoader code resides in plugin-container,
>    // and we must forward it through to the GMP code here.
> -  GMPProcessChild::SetGMPLoader(aGMPLoader);
> +  GMPProcessChild::SetGMPLoader(aChildData->gmpLoader.get());

Here and further below, you should test aChildData is not nullptr before dereferencing it.

::: xpcom/build/nsXREChildData.h:29
(Diff revision 1)
> +}
> +
> +/**
> + * Data needed to start a child process.
> + */
> +struct nsXREChildData

We should probably not add new classes with a "ns" prefix.

::: xpcom/build/nsXREChildData.h:40
(Diff revision 1)
> +
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)
> +  /**
> +   * Chromium sandbox TargetServices.
> +   */
> +  sandbox::TargetServices* sandboxTargetServices = nullptr;

Does this build on MSVC 2013?
Attachment #8748669 - Flags: review?(mh+mozilla)
Attachment #8748670 - Flags: review?(mh+mozilla)
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50467/#review48593

::: browser/app/nsBrowserApp.cpp:187
(Diff revision 2)
>      for (int i = 1; i < argc; i++) {
>        argv[i] = argv[i + 1];
>      }
> -    return XRE_XPCShellMain(--argc, argv, envp);
> +
> +    nsXREShellData shellData;
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)

This seems like a lot of (hard to find) places to change if we ever end up needing that on other platforms (or any other kind of modification)...  how about having a MOZ_SANDBOX_BROKER define? (the same probably applies to plugin-container code)

::: js/xpconnect/src/XPCShellImpl.cpp:1532
(Diff revision 2)
>          // asynchronized.
>          AutoAudioSession audioSession;
>  
>  #if defined(MOZ_SANDBOX)
>          // Required for sandboxed child processes.
> -        if (!SandboxBroker::Initialize()) {
> +        if (aShellData->sandboxBrokerServices) {

null check on aShellData

::: security/sandbox/win/SandboxInitialization.h:43
(Diff revision 2)
>  
> +static sandbox::BrokerServices*
> +GetInitializedBrokerServices()
> +{
> +  static sandbox::BrokerServices* sInitializedBrokerServices =
> +    []() -> sandbox::BrokerServices* {

Same as the TargetServices comment.

::: xpcom/build/nsXREShellData.h:19
(Diff revision 2)
> +#endif
> +
> +/**
> + * Data needed by XRE_XPCShellMain.
> + */
> +struct nsXREShellData

Same comment as with nsXREAppData.
Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50451/diff/1-2/
Attachment #8748663 - Attachment description: MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?ted → MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium
Attachment #8748668 - Attachment description: MozReview Request: Bug 1035125 Part 7: Remove unsed functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz → MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz
Attachment #8748665 - Flags: review?(mh+mozilla)
Attachment #8748669 - Flags: review?(mh+mozilla)
Attachment #8748670 - Flags: review?(mh+mozilla)
Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50453/diff/1-2/
Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50455/diff/1-2/
Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50457/diff/1-2/
Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50459/diff/1-2/
Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50461/diff/1-2/
Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50463/diff/1-2/
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50465/diff/1-2/
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/2-3/
https://reviewboard.mozilla.org/r/50457/#review48565

Thanks for all the reviews.

> USE_STATIC_LIBS has no effect on platforms other than Windows (and even on Windows, it's only when building with MSVC). No need to keep it here.

Removed.

> I think you can change linkage to 'dependent' here and remove the USE_LIBS for nspr and xul, while you're here.

Seems to work, thanks.

> why?

I have no idea.
I can only guess that this was when I was trying to resolve allocation mismatch problems with firefox.exe and I put this in the wrong file by mistake, sorry.
https://reviewboard.mozilla.org/r/50465/#review48587

> You should be able to use a "normal" assignment.

This is now a UniquePtr, so I couldn't just assign it, but I've reworked CreateGMPLoader to return a UniquePtr.

> I'm not convinced using a lambda makes it particularly better than just using a separate function.
> 
> But why not make all this a static method of TargetServices ?

It was just because I was doing this in the header file, so if I added another function someone might call it by mistake.
TargetServices is part of the Chromium sandbox code, so we don't want to change that unless we have to.

I've put the implementation into a cpp file compiled into the sandbox library and used a separate function in there.
This actually makes things a bit better, because it's not affected by the IPC code headers.

> Here and further below, you should test aChildData is not nullptr before dereferencing it.

I addded a MOZ_ASSERT, hopefully this is OK.

> We should probably not add new classes with a "ns" prefix.

Removed.

> Does this build on MSVC 2013?

Yes member initializers were first supported in VS2013.
https://reviewboard.mozilla.org/r/50467/#review48593

> This seems like a lot of (hard to find) places to change if we ever end up needing that on other platforms (or any other kind of modification)...  how about having a MOZ_SANDBOX_BROKER define? (the same probably applies to plugin-container code)

I'm not quite sure what you're propsing here.
Is it to define a MOZ_SANDBOX_BROKER the same as MOZ_SANDBOX and use it in the broker related parts of  firefox.exe code?
What about the broker related code in xul?

Also to have something like MOZ_SANDBOX_TARGET for the target services stuff in plugin-container and presumably firefox once we start using that as the child.

We'd probably have to keep MOZ_SANDBOX for some common bits, so I'm not sure it would make things better overall.

> null check on aShellData

Added MOZ_ASSERT.

> Same as the TargetServices comment.

Fixed in same way.

> Same comment as with nsXREAppData.

ns dropped.
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/3-4/
Blocks: 1250125
Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

https://reviewboard.mozilla.org/r/50457/#review49331
Attachment #8748665 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50465/#review49333

::: security/sandbox/win/SandboxInitialization.cpp:30
(Diff revision 2)
> +  }
> +
> +  return targetServices;
> +}
> +
> +/* static */

/* static */ usually documents static (class) methods, but here we're not talking about a static method, we're talking about a public namespaced method. So please remove this /* static */ and the one for LowerSandbox.
Attachment #8748669 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

https://reviewboard.mozilla.org/r/50467/#review49335
Attachment #8748670 - Flags: review?(mh+mozilla) → review+
(In reply to Bob Owen (:bobowen) from comment #47)
> https://reviewboard.mozilla.org/r/50467/#review48593
> 
> > This seems like a lot of (hard to find) places to change if we ever end up needing that on other platforms (or any other kind of modification)...  how about having a MOZ_SANDBOX_BROKER define? (the same probably applies to plugin-container code)
> 
> I'm not quite sure what you're propsing here.
> Is it to define a MOZ_SANDBOX_BROKER the same as MOZ_SANDBOX and use it in
> the broker related parts of  firefox.exe code?
> What about the broker related code in xul?
> 
> Also to have something like MOZ_SANDBOX_TARGET for the target services stuff
> in plugin-container and presumably firefox once we start using that as the
> child.
> 
> We'd probably have to keep MOZ_SANDBOX for some common bits, so I'm not sure
> it would make things better overall.

The point I was trying to make is that you have a lot of #if defined(XP_WIN) && defined(MOZ_SANDBOX). If you ever need those bits to be enabled on, say, OSX, then you have to go through all of them and add || defined(XP_DARWIN) or something like that. If instead you had a single define that's "automatically" defined for the equivalent conditions, in, say, configure, then you'd only have to change configure to enable the code on another platform.

How you chose to split things between broker and target (or not to) is up to you.
Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50451/diff/2-3/
Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50453/diff/2-3/
Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50455/diff/2-3/
Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50457/diff/2-3/
Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50459/diff/2-3/
Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50461/diff/2-3/
Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50463/diff/2-3/
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50465/diff/2-3/
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/4-5/
https://reviewboard.mozilla.org/r/50465/#review49333

> /* static */ usually documents static (class) methods, but here we're not talking about a static method, we're talking about a public namespaced method. So please remove this /* static */ and the one for LowerSandbox.

Damn, I copied the signature with static and converted it to a comment, before I'd removed it from the header.
Thanks, removed the one from part 9 as well.
https://reviewboard.mozilla.org/r/50467/#review48593

> I'm not quite sure what you're propsing here.
> Is it to define a MOZ_SANDBOX_BROKER the same as MOZ_SANDBOX and use it in the broker related parts of  firefox.exe code?
> What about the broker related code in xul?
> 
> Also to have something like MOZ_SANDBOX_TARGET for the target services stuff in plugin-container and presumably firefox once we start using that as the child.
> 
> We'd probably have to keep MOZ_SANDBOX for some common bits, so I'm not sure it would make things better overall.

Agreed on IRC to pick this up as part of future refactorings.
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Just thought that this patch has ended up with a few more GMP changes after review.

I don't think there's anything controversial, but requesting feedback so that you're aware of it.
Attachment #8748669 - Flags: feedback?(cpearce)
Comment on attachment 8748662 [details]
MozReview Request: Bug 1035125 Part 1: Back out changeset 1910714b56c6 and associated subsequent changes. r?bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50451/diff/3-4/
Attachment #8748669 - Flags: feedback?(cpearce)
Comment on attachment 8748663 [details]
MozReview Request: Bug 1035125 Part 2: Back out changeset 3c59642f6445 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50453/diff/3-4/
Comment on attachment 8748664 [details]
MozReview Request: Bug 1035125 Part 3: Back out changeset fa15c3e929d0 and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50455/diff/3-4/
Comment on attachment 8748665 [details]
MozReview Request: Bug 1035125 Part 4: Back out changeset 8ae39d920f5c and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50457/diff/3-4/
Comment on attachment 8748666 [details]
MozReview Request: Bug 1035125 Part 5: Back out changeset baa3f852133b and associated subsequent changes. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50459/diff/3-4/
Comment on attachment 8748667 [details]
MozReview Request: Bug 1035125 Part 6: Take Chromium commit 3181ba39ee787e1b40f4aea4be23f4f666ad0945 to add Windows 10 version to enumeration. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50461/diff/3-4/
Comment on attachment 8748668 [details]
MozReview Request: Bug 1035125 Part 7: Remove unused functions in security/sandbox/chromium/base/time/time.h to avoid nspr dependency. r?aklotz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50463/diff/3-4/
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50465/diff/3-4/
Comment on attachment 8748670 [details]
MozReview Request: Bug 1035125 Part 9: Link Chromium sandbox into firefox.exe instead of having a separate DLL. r?aklotz, r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50467/diff/5-6/
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Just thought that this patch has ended up with a few more GMP changes after review.

I don't think there's anything controversial, but requesting feedback so that you're aware of it.
Attachment #8748669 - Flags: feedback?(cpearce)
Comment on attachment 8748669 [details]
MozReview Request: Bug 1035125 Part 8: Pass sandboxing pointers through XRE_InitChildProcess instead of linking to more functions in xul. r?aklotz, r?glandium

Looks fine to me.
Attachment #8748669 - Flags: feedback?(cpearce) → feedback+
Depends on: 1273849
Depends on: 1285356
Duplicate of this bug: 1236931
You need to log in before you can comment on or make changes to this bug.