1314169 - Blacklist WasmFaultHandler from ASan

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(Away)]

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.

Comment 1

[(Away)]

Attachment: patch

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8806162 [details] [diff] [review]
patch

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

Thanks!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d34efa945a20
Blacklist WasmFaultHandler from ASan on Windows. r=luke

Comment 4

[Phil Ringnalda (:philor)]

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

Comment 5

[(Away)]

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

Comment 6

[Carsten Book [:Tomcat]]

backout from m-c too
https://hg.mozilla.org/mozilla-central/rev/ea14d2a7c5dc

Comment 7

[(Away)]

Attachment: 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?

Comment 8

[(Away)]

Attachment: 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?

Comment 9

[Luke Wagner [: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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b9fbbc9710
Leave the first VectoredExceptionHandler slot open for ASan. r=luke

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38992729&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e1adfb080c

Comment 12

[(Away)]

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?

Comment 13

[Luke Wagner [:luke]]

Actually, with the bug that led to the addition of MemoryProtectionExceptionhandler fixed, do we still need it?

Comment 14

[Emanuel Hoogeveen [:ehoogeveen]]

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

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

> 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?

Comment 16

[(Away)]

> 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.

Comment 17

[(Away)]

Attachment: patch v3

Third time's the charm?

Comment 18

[Luke Wagner [: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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/888f093ce015
Move the WasmFaultHandler to last priority on Windows ASan builds. r=luke

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/888f093ce015