1431803 - Sandbox MinGW Compilation errors: __try / __except

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Tom Ritter [:tjr] (OOTO until 9/2)]

MinGW doesn't support __try / __except.

Tor has successfully shipped Tor Browser replacing __try with if(true) and commenting out all __except blocks.

Can I take this approach? Would you accept patches to this effect bob? (Step after that will be trying to get it into chromium...)

Comment 1

[Bob Owen (:bobowen)]

I thought the patches I saw were using __try1 and removing blocks that were needed only for debug or ignored exceptions that possibly shouldn't be.
(I'm guessing this is something they might not take upstream, but we could possibly do it with a force included header.)

Did that approach not work out?

Comment 2

[Tom Ritter [:tjr] (OOTO until 9/2)]

(In reply to Bob Owen (:bobowen) from comment #1)
> I thought the patches I saw were using __try1 and removing blocks that were
> needed only for debug or ignored exceptions that possibly shouldn't be.
> Did that approach not work out?

That was a guesswork approach. It might work, but we haven't tested it. What I'm doing is trying to port the patches Tor Browser has tested (and is running with) into -central keeping the other builds working. (Their patch is https://gitweb.torproject.org/tor-browser.git/patch/?id=258b15f9 FWIW)  They didn't use the __try1 approach.  

Right now, our MinGW build of -central isn't actually running, we're working on tracking that down Bug 1411401

> (I'm guessing this is something they might not take upstream, but we could
> possibly do it with a force included header.)

Yea, this part will probably involve some patches we have to carry, but I'll try to minimize it.

Comment 3

[Bob Owen (:bobowen)]

OK, I see.

So if we add a force included file like [1], but just for MinGW, with those two #defines I guess that should work.
Probably ought to get a build peer review on that as well.

[1] https://searchfox.org/mozilla-central/source/mozilla-config.h.in#59

Comment 4

[Tom Ritter [:tjr] (OOTO until 9/2)]

Attachment: Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

Review commit: https://reviewboard.mozilla.org/r/214908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214908/

Comment 5

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214908/diff/1-2/

Comment 6

[Tom Ritter [:tjr] (OOTO until 9/2)]

Attachment: Bug 1431803 Disable a specific __try block on MinGW

Review commit: https://reviewboard.mozilla.org/r/214910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214910/

Comment 7

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214908/diff/2-3/

Comment 8

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944751 [details]
Bug 1431803 Disable a specific __try block on MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214910/diff/1-2/

Comment 9

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214908/diff/3-4/

Comment 10

[Tom Ritter [:tjr] (OOTO until 9/2)]

Okay bob, I reviewed the cases more carefully and I think this is ready for review now.

Comment 11

[Bob Owen (:bobowen)]

Comment on attachment 8944751 [details]
Bug 1431803 Disable a specific __try block on MinGW

https://reviewboard.mozilla.org/r/214910/#review221344

Patch file as well please.

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review221342

We should probably have a build peer review as well because of mozilla-config.h.in changes.
Maybe glandium.

::: security/sandbox/chromium-shim/base/win/mingw.h:1
(Diff revision 4)
> +#ifdef __MINGW32__

emacs and vim lines, license header and #if header guard please.

Also, a comment explaining how this is included and how it can potentially be used for other MinGW build issues.

Comment 13

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944751 [details]
Bug 1431803 Disable a specific __try block on MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214910/diff/2-3/

Comment 14

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214908/diff/4-5/

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review221384

Thanks.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review222260

A cursory grep __try says there are some outside security/sandbox. Are all of those in msvc-only sections?

::: security/sandbox/chromium-shim/base/win/mingw.h:26
(Diff revision 5)
> +#define __try if(true)
> +#define __except(x) else

Why not simply set that in mozilla-include.h.in?

Comment 17

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214908/diff/5-6/

Comment 18

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944751 [details]
Bug 1431803 Disable a specific __try block on MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214910/diff/3-4/

Comment 19

[Tom Ritter [:tjr] (OOTO until 9/2)]

(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8944748 [details]
> Bug 1431803 Force-include a header to turn __try into if(true) and __except
> into else
> 
> https://reviewboard.mozilla.org/r/214908/#review222260
> 
> A cursory grep __try says there are some outside security/sandbox. Are all
> of those in msvc-only sections?

Yes (otherwise the MinGW build wouldn't build.)

> ::: security/sandbox/chromium-shim/base/win/mingw.h:26
> (Diff revision 5)
> > +#define __try if(true)
> > +#define __except(x) else
> 
> Why not simply set that in mozilla-include.h.in?

Moved!

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

https://reviewboard.mozilla.org/r/214908/#review222628

::: commit-message-4c8d4:1
(Diff revision 6)
> +Bug 1431803 Force-include a header to turn __try into if(true) and __except into else r?bobowen

You should adjust the commit message.

Comment 21

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944748 [details]
Bug 1431803 Turn __try into if(true) and __except into else in the chromium sandbox code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214908/diff/6-7/

Comment 22

[Tom Ritter [:tjr] (OOTO until 9/2)]

Comment on attachment 8944751 [details]
Bug 1431803 Disable a specific __try block on MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214910/diff/4-5/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0de1027fbda
Turn __try into if(true) and __except into else in the chromium sandbox code r=bobowen,glandium
https://hg.mozilla.org/integration/autoland/rev/5506a531ce36
Disable a specific __try block on MinGW r=bobowen

Comment 24

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c0de1027fbda
https://hg.mozilla.org/mozilla-central/rev/5506a531ce36