Closed Bug 1342564 Opened 3 years ago Closed 2 years ago

Sandbox causes crash in NoteIntentionalCrash() due to blocked fopen

Categories

(Core :: Security: Process Sandboxing, defect)

x86
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: tjr, Assigned: Alex_Gaynor)

References

(Blocks 2 open bugs)

Details

(Whiteboard: sb+)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1235982
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...?
Flags: needinfo?(gpascutto)
Summary: Sandboxes causes crash in NoteIntentionalCrash() → Sandbox causes crash in NoteIntentionalCrash() due to blocked fopen
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.
(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.
Flags: needinfo?(gpascutto)
(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.
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.
See Also: → 1345046
Whiteboard: sb+
Blocks: sb-log
No longer blocks: 1235982
Blocks: sb-test
Blocks: 1235982
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.
Flags: needinfo?(tom)
(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.
Flags: needinfo?(tom) → needinfo?(mconley)
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 :-)
Keywords: leave-open
(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.
Attachment #8933445 - Flags: review?(dmajor)
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
Attachment #8933445 - Flags: review?(dmajor) → review+
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 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?)
Entirely possible!
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/178d6dc61941
Don't crash the process when we fail to open the bloat log r=dmajor
Keywords: checkin-needed
(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?
Flags: needinfo?(mconley) → needinfo?(ted)
(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.
Flags: needinfo?(ted)
Attached patch 1342564.patchSplinter Review
Assignee: nobody → agaynor
Attachment #8933445 - Attachment is obsolete: true
Attachment #8935798 - Flags: review?(ted)
Keywords: leave-open
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)!
Attachment #8935798 - Flags: review?(ted) → review+
Thanks! And no worries, it wasn't a blocker anymore, so no harm.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d491aa5379
Don't attempt to write out intentional crash files in opt builds. r=ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e4d491aa5379
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.