1342564 - Sandbox causes crash in NoteIntentionalCrash() due to blocked fopen

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Tom Ritter [:tjr]]

Okay this one is kind of complicated.  privateNoteIntentionalCrash() is a function called in mochitests when you're purposely crashing the browser. It's used to write out a note to the log saying "I am intentionally crashing this". The C implementation is here: https://dxr.mozilla.org/mozilla-central/source/xpcom/base/IntentionalCrash.h?q=NoteIntentionalCrash&redirect_type=direct#25

At the bottom of that function it fopen's the file. With sandboxing... it can't open the file. And it crashes the content process. I've only tested this on Windows FWIW.

BUT! privateNoteIntentionalCrash() was only called when we INTENDED to crash the content process so... the tests expected crashes and privateNoteIntentionalCrash() delivered. So (I believe) everything worked out just fine as far as the few tests that use it were concerned. 

Anyway, to reproduce this in an enviroment, do this:

1. ./mach mochitest toolkit/content/tests/browser/browser_crash_previous_frameloader.js

Confirm this runs and passes. 

2. Edit toolkit/content/tests/browser/browser_crash_previous_frameloader.js:

a. Right before 'yield ContentTask.spawn(browser, null, function() {' put:
dump("PID: " + browser.frameLoader.tabParent.osPid + "\n");

b. Right before 'privateNoteIntentionalCrash();' put:
yield new Promise((resolve) => setTimeout(resolve, 15000));

3. ./mach mochitest toolkit/content/tests/browser/browser_crash_previous_frameloader.js

Run this and after it outputs the PID attach your debugger to the PId it outputs and set a breakpoint on mozilla:NoteIntentionalCrash

4. Step through and observe.

Comment 1

[(Away)]

The fopen is getting blocked by the sandbox's NtCreateFile hook, so we get a null FILE* and kaboom.

gcp: What's the right way to deal with issues like this? Do you let the operation through the sandbox, or broker it through somebody, or...?

Comment 2

[(Away)]

It's worth mentioning that although the mochitests are kind of ok today by happenstance, this crash is blocking upcoming work in bug 1235982 comment 31.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

(In reply to David Major [:dmajor] from comment #1)
> gcp: What's the right way to deal with issues like this? Do you let the
> operation through the sandbox, or broker it through somebody, or...?

You have a bunch of options:

1) Disable the sandbox for this test (not the best idea, I think).
2) Write the file in the chrome process via some IPC (this always works everywhere).
3) There's generally a content-specific "tempdir" available where you do have write permissions, so you can write it there. I think this currently works well on Windows and Mac via a directory provider (but the name escapes me, Bob Owen should know). Linux is lacking support for that but will allow /tmp.

However, looking at the code, it's trying to write into an existent log file, so (3) doesn't look like an option. You'll need to do 2, I'm afraid.

Comment 4

[Bob Owen (:bobowen)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> (In reply to David Major [:dmajor] from comment #1)

> 3) There's generally a content-specific "tempdir" available where you do
> have write permissions, so you can write it there. I think this currently
> works well on Windows and Mac via a directory provider (but the name escapes
> me, Bob Owen should know). Linux is lacking support for that but will allow
> /tmp.

The key is NS_APP_CONTENT_PROCESS_TEMP_DIR.

I'm guessing we're seeing this issue on opt tests, it appears this writes to the xpcom memory bloat log for some reason.
This is only normally used for debug builds I believe and we allowed write access to the normal windows temp directory in debug builds for this, before we could turn on any sandboxing, as an easy fix.

I think this (and other issues I've seen) highlight a general problem in our logging, it would be great if we had some sort of lower level logging API that everything could use (MOZ_LOG, mem bloat, etc.), that just worked with sandboxing.
I guess the two options are streaming the information and writing in the parent or just opening the file handle in the parent and writing in the child.
The second one seems preferable from a performance / reliability point if view to me.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Another thought is that perhaps this is better fixed on another level: some metadata that lies with the test description and explains to the test driver that this test is expected to crash.

The only downside is that you lose the fine-grained record that you crashed exactly where you intended to, but that doesn't look like a big issue in practice, especially as you can divide the test up when you expect that to be a problem.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

On macOS (and, I believe, on Linux with a patch currently in review) we add a rule to the sandbox to allow writes to |dirname(PR_GetEnv("XPCOM_MEM_BLOAT_LOG"))|, since that's where this function writes to. But (at least on macOS) this is guarded by |#ifdef DEBUG|, so the only case this should matter is in opt builds.

However, the mochitest runner only cares about these intentional crash notes when firefox actually created |PR_GetEnv("XPCOM_MEM_BLOAT_LOG")| (the crash notes are written to a sibbling of this path). Creating that file is done by |InitTraceLog| in |nsTraceRefcnt.cpp|. Near as I can tell it only does that in DEBUG builds (or when you build with |--enable-logrefcnt|). So basically all the existing users of this log file are fine with it only working in opt builds :-)

:tjr, do you need this to work in opt builds, or is it sufficient for your use to just only run that test in debug? We should probably also make |NoteIntentionalCrash| not crash when it can't open the file :-)

If we really need this for opt builds we can think about what the right way to make this work is.

Comment 7

[Tom Ritter [:tjr]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #6)
> On macOS (and, I believe, on Linux with a patch currently in review) we add
> a rule to the sandbox to allow writes to
> |dirname(PR_GetEnv("XPCOM_MEM_BLOAT_LOG"))|, since that's where this
> function writes to. But (at least on macOS) this is guarded by |#ifdef
> DEBUG|, so the only case this should matter is in opt builds.
> 
> However, the mochitest runner only cares about these intentional crash notes
> when firefox actually created |PR_GetEnv("XPCOM_MEM_BLOAT_LOG")| (the crash
> notes are written to a sibbling of this path). Creating that file is done by
> |InitTraceLog| in |nsTraceRefcnt.cpp|. Near as I can tell it only does that
> in DEBUG builds (or when you build with |--enable-logrefcnt|). So basically
> all the existing users of this log file are fine with it only working in opt
> builds :-)
> 
> :tjr, do you need this to work in opt builds, or is it sufficient for your
> use to just only run that test in debug? We should probably also make
> |NoteIntentionalCrash| not crash when it can't open the file :-)
> 
> If we really need this for opt builds we can think about what the right way
> to make this work is.

Uhhhh.... //passes to Mike!

I am not sure. I presume we can restrict the test to only running in debug builds, but I don't know the implications of that or if that's acceptable.

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1342564 - Don't crash the process when we fail to open the bloat log

NoteIntentionalCrash is supposed to leave a marker that a process has crashed
intentionally, but if it can't open the target file it will crash the process
itself. This can cause difficulty debugging and false-positive results for
tests expecting crashes.

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

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Uploaded the trivial patch to make it at least not crash when it can't open the file. Both nfroyd and erahm are on PTO, so unless there's another XPCOM reviewer it'll have to hold for a bit :-)

Comment 10

[(Away)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #9)
> Uploaded the trivial patch to make it at least not crash when it can't open
> the file. Both nfroyd and erahm are on PTO, so unless there's another XPCOM
> reviewer it'll have to hold for a bit :-)

I don't want Tom's work to be blocked on this... I've spent enough time around crash code that I'll rubber-stamp it and take the blame if anyone complains.

Comment 11

[(Away)]

Comment on attachment 8933445 [details]
Bug 1342564 - Don't crash the process when we fail to open the bloat log

https://reviewboard.mozilla.org/r/204364/#review209916

Comment 12

[(Away)]

Comment on attachment 8933445 [details]
Bug 1342564 - Don't crash the process when we fail to open the bloat log

https://reviewboard.mozilla.org/r/204364/#review209918

PS, change tabs to spaces please :-)

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8933445 [details]
Bug 1342564 - Don't crash the process when we fail to open the bloat log

https://reviewboard.mozilla.org/r/204364/#review209918

I don't see any tabs. Am I just losing it?

(Is it possible the MozReview UI for "added a level of indentation" is throwing you off?)

Comment 14

[(Away)]

Entirely possible!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/178d6dc61941
Don't crash the process when we fail to open the bloat log r=dmajor

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/178d6dc61941

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tom Ritter [:tjr] from comment #7)
> 
> Uhhhh.... //passes to Mike!
> 

I don't think I can answer the question in comment 6 with much certainty. Perhaps ted has an opinion?

Comment 18

[(not currently active) Ted Mielczarek]

(In reply to Bob Owen (:bobowen) from comment #4)
> the xpcom memory bloat log for some reason.
> This is only normally used for debug builds I believe and we allowed write
> access to the normal windows temp directory in debug builds for this, before
> we could turn on any sandboxing, as an easy fix.

So the whole point of this is that in builds with leak logging enabled (which is only debug builds nowadays, although you can ostensibly --enable-logrefcnt in an opt build), we need a way to signal to the code that checks for shutdown leaks in our test harnesses that this process is intentionally crashing so that it can ignore it for the purpose of leak checking:
https://dxr.mozilla.org/mozilla-central/rev/79d3e25106f8eb369dde6a47199547d131af8d3d/testing/mozbase/mozleak/mozleak/leaklog.py#47

We only use the shutdown leak checking in debug builds, so `NoteIntentionalCrash` doesn't actually do anything useful in opt builds:
https://dxr.mozilla.org/mozilla-central/rev/79d3e25106f8eb369dde6a47199547d131af8d3d/testing/mochitest/runtests.py#2715

We do always run `mozleak.process_leak_log`:
https://dxr.mozilla.org/mozilla-central/rev/79d3e25106f8eb369dde6a47199547d131af8d3d/testing/mochitest/runtests.py#2790

but that code just warns if the leak log is missing (I assume this warning shows up in all of our opt mochitest logs!):
https://dxr.mozilla.org/mozilla-central/rev/79d3e25106f8eb369dde6a47199547d131af8d3d/testing/mozbase/mozleak/mozleak/leaklog.py#164

However, we always *set* `XPCOM_MEM_BLOAT_LOG` in the environment for no apparent reason, so `NoteIntentionalCrash` will *try* to write that info even when it's not useful:
https://dxr.mozilla.org/mozilla-central/rev/79d3e25106f8eb369dde6a47199547d131af8d3d/testing/mochitest/runtests.py#1654

Given all of that, I think the best fix here would be to wrap the guts of `NoteIntentionalCrash` in `#ifdef NS_BUILD_REFCNT_LOGGING` (which comes from nscore.h), or `#ifdef MOZ_DEBUG`, given the comments above suggesting that the sandbox-loosening is only available in debug builds.

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

Attachment: 1342564.patch

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8935798 [details] [diff] [review]
1342564.patch

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

Thanks! Sorry for letting this simple patch sit (especially after I wrote such a long comment describing the problem)!

Comment 21

[Alex Gaynor [:Alex_Gaynor]]

Thanks! And no worries, it wasn't a blocker anymore, so no harm.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d491aa5379
Don't attempt to write out intentional crash files in opt builds. r=ted

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e4d491aa5379