Add assertions to block spinning the event loop if known to be dangerous (block JS from re-entering itself)

NEW
Assigned to

Status

()

Core
XPCOM
5 years ago
10 months ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({sec-want})

Trunk
sec-want
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 733786 [details] [diff] [review]
Block event-loop recursion (WIP)

A frequent(?) cause of hard-to-detect and often hard-to-diagnose problems is code spinning the event loop from within contexts where this is unsafe (such as in code called from JS - this can cause Javascript to re-enter itself).

This adds a debug-only mechanism to mark when we're in a state where spinning the event loop would not be allowed, and assert on it.  It allows for nesting of blocks.

I've built a patch that catches at least the primary dispatch point for JS code, though I can't say I know if that's the only spot that should be protected.  I'd appreciate pointers to other spots (in JS or not) that could benefit from this.

I've verified that if I change certain DISPATCH_NORMALs to DISPATCH_SYNC, it correctly asserts.
Attachment #733786 - Flags: feedback?(benjamin)

Comment 2

5 years ago
Comment on attachment 733786 [details] [diff] [review]
Block event-loop recursion (WIP)

Won't this assert every time somebody calls alert() or confirm() or synchronous XHR or calls a plugin method which spins a nested event loop (which are relatively common)?

There are times that we absolutely must not spin a nested event loop (especially during layout), but I'm skeptical that we ought to try to prevent this from content JS when there are so many cases where we do it on purpose.
Attachment #733786 - Flags: feedback?(benjamin) → feedback-
And in fact, for content JS there are places where the spec and web compat _requires_ the event loop to spin (c.f. showModalDialog).
(Assignee)

Comment 4

5 years ago
Created attachment 734085 [details] [diff] [review]
Block event-loop recursion (WIP #2)

now suspends blocks in nsXMLHttpRequest::Send(), ShowAsModal dialogs and CreateNewContentWindow
(Assignee)

Updated

5 years ago
Attachment #733786 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
From IRC:

[09:29]	bsmedberg	jesup: the problem is really plugins, because they can spin the event loop almost anywhere
[09:30]	bsmedberg	jesup: which sucks, but I don't know if there's anything we can do about it. Instead I think it would be much more interesting to identify the critical sections where we *shouldn't* spin an event loop
...
[09:31]	bsmedberg	it should be safe to spin the event loop any time we're running content JS
[09:31]	bsmedberg	it might lead to weird reentrance behavior, but not security bugs/memory corruption/crashes
[09:31]	jesup	bsmedberg: The patch adds a (debug-only) runtime abort when it believes we're spinning and shouldn't. We can set (or unset) the blocked-from-spinning state at any point we need
[09:32]	bsmedberg	jesup: yeah, I'm saying this should 1) probably be release-mode and not just debug-mode 2) we should pick more interesting targets
[09:32]	bsmedberg	like "whenever we're enumerating a hashtable"
...
[09:43]	jesup	bsmedberg: so I see two related questions: 1) where is it absolutely unsafe to allow the event loop to spin (and we may want to block even in release), 2) Where is it bad/weird/causes-random-behavior to spin the loop in that it allows re-entrance?
[09:44]	jesup	My patch was targeted at the second, but could easily be extended to cover the first as well
[09:44]	bsmedberg	jesup: yes; I don't think we should spend time on #2 at the moment
[09:44]	bsmedberg	the web is just crazy enough that "anytime" pretty much covers it
[09:46]	jesup	bsmedberg: ok, though it makes me sad in that we (repeatedly) check in code that does this (and breaks things in the process). Perhaps I'll put a guard against DISPATCH_SYNC from MainThread (at least from within JS)?
[09:46]	bsmedberg	jesup: ah yeah, I can't think of *any* reason we should DISPATCH_SYNC from the main thread
[09:47]	bsmedberg	but of course, I've been surprised before
[09:47]	derf	I suspect the difficulty there will be finding all the places we actually do it and fixing them before you can _leave_ that guard in.

Comment 6

4 years ago
> the problem is really plugins, because they can spin the event loop almost anywhere

For fuzzing, I'd be more than happy with an assertion that ignores the plugin case. Even if it's as an exclusion as wide as MOZ_ASSERT_IF(!everLoadedAnyPlugin, ...).
Keywords: sec-want
(Assignee)

Updated

a year ago
See Also: → bug 1331036
You need to log in before you can comment on or make changes to this bug.