Blacklist WasmFaultHandler from ASan

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dmajor (offline), Assigned: dmajor (offline))

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

a year ago
Win64 ASan relies on a VectoredExceptionHandler to create shadow memory regions on demand. If WasmFaultHandler gets the exception first, it can itself fault while looking at shadow memory, leading to an infinite recursion.

The easy fix is to MOZ_ASAN_BLACKLIST the fault handler. Alternatively we could put WasmFaultHandler at the end of the chain during its AddVectoredExceptionHandler, but it seems risky to make a product behavior change for the sake of a testing tool.
(Assignee)

Comment 1

a year ago
Created attachment 8806162 [details] [diff] [review]
patch
Attachment #8806162 - Flags: review?(luke)
Comment on attachment 8806162 [details] [diff] [review]
patch

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

Thanks!
Attachment #8806162 - Flags: review?(luke) → review+

Comment 3

a year ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d34efa945a20
Blacklist WasmFaultHandler from ASan on Windows. r=luke

Comment 4

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d34efa945a20
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 5

a year ago
This patch wasn't enough :( We still had asan instrumentation running several frames deep. I don't think we want to chase after the entire call tree with a MOZ_ASAN_BLACKLIST hammer, so I guess we'll have to alter the handler ordering after all.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea14d2a7c5dc
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

a year ago
Created attachment 8809458 [details] [diff] [review]
Change the handler ordering

Well, I don't feel great about changing this just for the sake of Asan. I suppose I could put it behind #ifdef MOZ_ASAN but I think I'd feel even worse about that. Luke, what do you think?
Attachment #8806162 - Attachment is obsolete: true
Attachment #8809458 - Flags: review?(luke)
(Assignee)

Comment 8

a year ago
Created attachment 8809460 [details] [diff] [review]
Change the handler ordering

...and that was totally the wrong patch. Let's try this again.

Well, I don't feel great about changing this just for the sake of Asan. I suppose I could put it behind #ifdef MOZ_ASAN but I think I'd feel even worse about that. Luke, what do you think?
Attachment #8809458 - Attachment is obsolete: true
Attachment #8809458 - Flags: review?(luke)
Attachment #8809460 - Flags: review?(luke)
Comment on attachment 8809460 [details] [diff] [review]
Change the handler ordering

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

The reason I originally set this to 'first' is that I was under the impression that 'first' handlers should be all the filters that attempt to recover-and-resume while 'last' handlers assume the ship is going down and do stuff like crash reporting.  Now, it appears that noone else (other than this one other use in js/src/ds/ that uses 'first' also) is using vectored exception handlers, so I guess this patch is fine for now and certainly simpler than other options I can imagine.
Attachment #8809460 - Flags: review?(luke) → review+

Comment 10

a year ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b9fbbc9710
Leave the first VectoredExceptionHandler slot open for ASan. r=luke
(Assignee)

Comment 12

a year ago
So, there are potentially three handlers in play, registered in this order:
ShadowExceptionHandler, MemoryProtectionExceptionHandler, WasmFaultHandler

My push in comment 10 made us call the MemoryProtectionExceptionHandler before the WasmFaultHandler, which was bad because the MPEH is of the 'ship is going down' category.

Ideally we'd want them to be called in this order:
ShadowExceptionHandler, WasmFaultHandler, MemoryProtectionExceptionHandler
but I don't see a way to achieve that with only 'first' and 'last' operations. :(

I suppose I could register the WasmFaultHandler last only when under MOZ_ASAN, and then also disable the MemoryProtectionHandler as it's only a crash reporting convenience. 

So under ASan we'd have: ShadowExceptionHandler, WasmFaultHandler
and otherwise we'd have: WasmFaultHandler, MemoryProtectionExceptionHandler

Does that seem reasonable?
Flags: needinfo?(dmajor)
Actually, with the bug that led to the addition of MemoryProtectionExceptionhandler fixed, do we still need it?
Flags: needinfo?(emanuel.hoogeveen)
Turns out we don't know it's actually fixed - the annotations weren't working because of bug 1309573 (which just landed), and I messed up my crash stats searches (forget to set the upper limit of the range, so it defaulted to a week from the initial range). We may also want to reuse the mechanism for bug 1309846, so for now at least I think it'll stick around.

Disabling the MemoryProtectionExceptionHandler when running under ASAN should be fine, you can probably just add a check at [1] (I added that for bhackett to use for his web replay prototype, but there's no reason it can't serve as a generic mechanism).

[1] https://dxr.mozilla.org/mozilla-central/rev/d38d06f85ef59c5dbb5d4a1a8d895957a78714de/js/src/ds/MemoryProtectionExceptionHandler.cpp#98
Flags: needinfo?(emanuel.hoogeveen)
> I suppose I could register the WasmFaultHandler last only when under MOZ_ASAN, and then also disable the MemoryProtectionHandler as it's only a crash reporting convenience.

I think this makes sense. I don't think there's a way to make this work otherwise without moving MemoryProtectionExceptionHandler::install() to JSRuntime creation, which would then have to special case Windows. I assume ASAN is fine with the current state of things on other operating systems?
(Assignee)

Comment 16

a year ago
> I assume ASAN is fine with the current state of things on other operating systems?

Yeah, on-demand shadow mapping is just to work around a Windows limitation and isn't used on other OS.
(Assignee)

Comment 17

a year ago
Created attachment 8809996 [details] [diff] [review]
patch v3

Third time's the charm?
Attachment #8809460 - Attachment is obsolete: true
Attachment #8809996 - Flags: review?(luke)
Comment on attachment 8809996 [details] [diff] [review]
patch v3

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

I can imagine more ideal solutions that involve prospective handlers registering themselves with MFBT with some ordering enum, but this seems good enough for now.  Thanks for investigating.

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +1340,5 @@
>      // Install a SIGSEGV handler to handle safely-out-of-bounds asm.js heap
>      // access and/or unaligned accesses.
>  # if defined(XP_WIN)
> +#  if !defined(MOZ_ASAN)
> +    const bool firstHandler = true;

Can you flip the order of the two branches so you can #if defined(MOZ_ASAN)?  I'd also add a comment to the !MOZ_ASAN case explaining that it must go first so that other handlers, such as the MemoryProtectionExceptionHandler, can go last and assume we are crashing.
Attachment #8809996 - Flags: review?(luke) → review+

Comment 19

a year ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/888f093ce015
Move the WasmFaultHandler to last priority on Windows ASan builds. r=luke

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/888f093ce015
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
status-firefox52: fixed → ---
Target Milestone: mozilla52 → mozilla53
You need to log in before you can comment on or make changes to this bug.