The chromium sandbox's use of __try/__except breaks mingw

RESOLVED FIXED in Firefox 34

Status

()

Core
Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Away for a while, Assigned: Jacek Caban)

Tracking

unspecified
mozilla34
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34 fixed, firefox35 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
I haven't tested this on minggw myself, but in clang-cl, I've seen build failures that make us fall back to the MSVC compiler since the sandbox code has been turned on by default.

Not sure how we're planning to address this, but I'm filing this so that we're aware of the issue.
Is the eventual plan to have clang as the main compiler on Windows?  Otherwise can we just default sandbox to off for some compilers?  CC'ed TimAbraldes who's the sandbox owner on Windows.
(Assignee)

Comment 2

3 years ago
mingw has even more problems with sandbox code. I have fixes for most of them in my tree, but a strange problem with STL code remains. I have disabled __try/__except blocks in my tree. I haven't looked at them closely yet. Are they critical to get sandbox running (as in Firefox will always crash without them) or are they additional security feature? If they are not critical, then I think fixing the compilation is preferable. Otherwise, I'm affraid that this will have to be disabled for mingw.

I'm happy to help working on this bug, but I will be mostly away until middle August.
I'm on board with disabling sandbox code on compilers that fail to build it.

The __try/__except is in Chromium code which a) we don't want to change because we want to be able to pull upstream changes easily and b) the upstream authors probably already would have fixed if this was an issue they cared about.

I am a bit surprised that __try/__except is causing an issue since there's also a usage of it in "nsprpub/pr/src/md/windows/ntthread.c" but maybe that doesn't build during mingw builds.
(Reporter)

Comment 4

3 years ago
(In reply to comment #3)
> I'm on board with disabling sandbox code on compilers that fail to build it.
> 
> The __try/__except is in Chromium code which a) we don't want to change because
> we want to be able to pull upstream changes easily and b) the upstream authors
> probably already would have fixed if this was an issue they cared about.
> 
> I am a bit surprised that __try/__except is causing an issue since there's also
> a usage of it in "nsprpub/pr/src/md/windows/ntthread.c" but maybe that doesn't
> build during mingw builds.

Those are hidden behind #ifdef _MSC_VER.
(Reporter)

Comment 5

3 years ago
(And we use MOZ_SEH_TRY/MOZ_SEH_CATCH in a couple of other places)
(Assignee)

Comment 6

3 years ago
Yeah, that's not a problem. We could introduce similar abstraction in Chromium and I think that they don't have it now because they don't support clang/mingw at all.

My question was how critical working __try/__except are in runtime. For example, if they are meant to handle a critical errors in a more secure way, we can probably live with it for non-tier1 platforms (disabling is obviously also an option in this case). If they are part of the design and properly working code expects those exceptions to happen, then we need to disable the whole sandbox.
(Reporter)

Comment 7

3 years ago
From a quick look, it seems like the __try/__except blocks are used for setting the thread's name and also protecting against accessing invalid memory (wow!!!), so just removing them doesn't seem like an option.
(Assignee)

Comment 8

3 years ago
Created attachment 8478426 [details] [diff] [review]
WIP

Here is a WIP patch that works for me with mingw build. I added a new configure that disables the whole sandbox support instead. It also adds new MOZ_SANDBOX define that mostly replaces adding defined(MOZ_CONTENT_SANDBOX) || defined(MOZ_GMP_SANDBOX) (and potentially more in the future) all over the place.

Here is try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88211e9a2f6d
Attachment #8478426 - Flags: feedback?(ehsan)
(Assignee)

Comment 9

3 years ago
The previous version had a typo:
https://tbpl.mozilla.org/?tree=Try&rev=78e86ebc6b45
(Reporter)

Comment 10

3 years ago
Comment on attachment 8478426 [details] [diff] [review]
WIP

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

bbondy can give you feedback on the idea, and glandium on the build system changes...
Attachment #8478426 - Flags: feedback?(netzen)
Attachment #8478426 - Flags: feedback?(mh+mozilla)
Attachment #8478426 - Flags: feedback?(ehsan)
Comment on attachment 8478426 [details] [diff] [review]
WIP

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

Thanks for checking.  Yep, I agree with the idea and the implementation.
Attachment #8478426 - Flags: feedback?(netzen) → feedback+
Comment on attachment 8478426 [details] [diff] [review]
WIP

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

::: configure.in
@@ +6359,4 @@
>  dnl = Content process sandboxing
>  dnl ========================================================
>  if test -n "$gonkdir"; then
> +    MOZ_CONTENT_SANDBOX=

You're effectively changing the value of MOZ_CONTENT_SANDBOX on B2G.

@@ +6399,5 @@
>  fi
>  
>  AC_SUBST(MOZ_GMP_SANDBOX)
>  
> +if test -z "$MOZ_CONTENT_SANDBOX" -a -z "$MOZ_CMP_SANDBOX"; then

typo
Attachment #8478426 - Flags: feedback?(mh+mozilla) → feedback-
(Assignee)

Comment 13

3 years ago
Created attachment 8479015 [details] [diff] [review]
patch

Thanks for feedbacks. Try with the typo fixed is green:
https://tbpl.mozilla.org/?tree=Try&rev=78e86ebc6b45
The attached version also fixes the case where $gonkdir is specified.
Assignee: nobody → jacek
Attachment #8478426 - Attachment is obsolete: true
Attachment #8479015 - Flags: review?(mh+mozilla)
Comment on attachment 8479015 [details] [diff] [review]
patch

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

r+ for the build system changes.
Attachment #8479015 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 15

3 years ago
(In reply to Mike Hommey [:glandium] (out from Sep 6 to Sep 22) from comment #14)
> r+ for the build system changes.

Thanks. I assume that's enough, given earlier feedback+:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a12ddb46882
https://hg.mozilla.org/mozilla-central/rev/9a12ddb46882
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Summary: The chromium sanbox's use of __try/__except breaks mingw → The chromium sandbox's use of __try/__except breaks mingw

Comment 17

3 years ago
Does this new option work with the installer?

I've got a feeling that the installer expects sandboxbroker.dll to exist and I'm not sure how it handles things if it doesn't.
Flags: needinfo?(jacek)
(In reply to Bob Owen (:bobowen) from comment #17)
> Does this new option work with the installer?
> 
> I've got a feeling that the installer expects sandboxbroker.dll to exist and
> I'm not sure how it handles things if it doesn't.

If I change the XP_LINUX case to unconditional like the Windows one, and then --disable-sandbox:

Error: /home/jld/obj/gecko-dev/obj-x86_64-unknown-linux-gnu/browser/installer/package-manifest:549: Missing file(s): bin/libmozsandbox.so

So I'm guessing that's how it would handle things.
Created attachment 8480852 [details] [diff] [review]
bug1042426-installer-stuff-hg0.diff

Here's a patch for the manifests.  I can refile it as a separate bug if that would make more sense.
Attachment #8480852 - Flags: review?(mh+mozilla)

Updated

3 years ago
Flags: needinfo?(jacek)
(Assignee)

Comment 20

3 years ago
I missed that because I used a build with installer disabled. Thank you for taking care of that.
Attachment #8480852 - Flags: review?(mh+mozilla) → review?(mshal)

Updated

3 years ago
Attachment #8480852 - Flags: review?(mshal) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1284474ca9ac

And I'll need to go through the procedures for 34 branch uplift.
Flags: needinfo?(jld)
https://hg.mozilla.org/mozilla-central/rev/1284474ca9ac
Comment on attachment 8480852 [details] [diff] [review]
bug1042426-installer-stuff-hg0.diff

Approval Request Comment
[Feature/regressing bug #]: Bug 1042426 — it's cleanup for the main part of this bug, which didn't miss the branch point like this patch did.
[User impact if declined]: --disable-sandbox builds on win32, including mingw, break the installer (and break `make install`, if it's anything like Unix).
[Describe test coverage new/current, TBPL]: Has gone through a few TBPL runs on mozilla-central by now.
[Risks and why]: Minimal — shouldn't change anything for other types of builds.
[String/UUID change made/needed]: None.
Attachment #8480852 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jld)
Comment on attachment 8480852 [details] [diff] [review]
bug1042426-installer-stuff-hg0.diff

This bug is difficult to follow as part of the fix landed on 34 and part landed on 35. To your question about filing another bug, in the future, I would prefer that a follow-up bug be filed for work that has to land after a merge has already been completed. I'm marking Aurora+ on this bug as the second patch has already landed on m-c 35.
Attachment #8480852 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/91d7c9c5b349
status-firefox34: affected → fixed
You need to log in before you can comment on or make changes to this bug.