995649 - protect the GC heap when we are not running in the JS engine

Status: RESOLVED WONTFIX

(Core :: JavaScript: GC, enhancement, P3)

Description

[Terrence Cole [:terrence]]

We suspect that some of our consistent low volume crashes may be coming from some buggy code elsewhere in firefox accidentally scribbling over the GC heap. We could find these quite quickly by setting the GC heap PROT_NONE when we are not in a request.

This should be pretty easy to implement: loop over the chunk-set and chunk-pool and mprot the chunks in JSEnter/LeaveRequest. It would be rather slow, but should very quickly tell us if this is actually the problem. Moreover, it would point us right at the offending code.

Comment 1

[Terrence Cole [:terrence]]

Attachment: mprotect_the_gcheap-v0.diff

This patch works; a little too well, even. It's identified places where we read from the normal heap from off-main-thread. These are all safe accesses to the script -- it is protected from both collection and from execution by the background compilation -- or to constant data on the runtime. Still, we need to fix these before we can implement the technique here.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: mprotect the gc heap (rebase on gecko 32)

Comment 3

[Terrence Cole [:terrence]]

Note that I'm not actually working on this. There are way too many places where we have ridiculous special cases that allow access outside a request. I think it would be a nice boon to have this: it would probably have caught several cases where Ion's OMT compile thread was reading and writing from the main heap concurrent with GC. On the other hand, TSan also caught this case immediately and without us having to do 6 months of difficult, performance sensitive rewriting and debugging. So I guess we could do this, but it's a super, super low priority.

Comment 4

[Jon Coppeard (:jonco)]

This could be useful to us to track down some of our crashes.

Continually protecting/unprotecting the whole heap will be slow, so we could unprotect pages on demand if we detect an access to them in the fault handler.  Also, we might want to only write-protect pages.

Note that this can only detect access to GC things themselves, not associated malloc memory.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

If we do this, we should register each protected region with MemoryProtectionExceptionHandler so attempts to access them are annotated in crash stats.

Unprotecting on demand sounds nice, but how would the fault handler differentiate between legitimate accesses and corruption?

Comment 6

[Lars T Hansen [:lth]]

With shared memory there can be concurrent faults from multiple threads, so probably a special case?

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug995649-mprotect-gc-heap

Initial attempt at rebasing the patch.  The code to protect empty (unused) chunks is commented out.  10% of test fail.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #5)
> Unprotecting on demand sounds nice, but how would the fault handler
> differentiate between legitimate accesses and corruption?

We'd store a flag somewhere to say if access was allowed, i.e. we're running inside the engine.

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Hmm, I guess it is mostly outside influences we're worried about. We could store a list of pages that were unprotected while the flag was set, and reprotect them when we clear it.

Comment 10

[Jon Coppeard (:jonco)]

It seems there is a new feature for Intel CPUs called "memory protection keys" that could be very useful here: https://lwn.net/Articles/643797/

AIUI with this we wouldn't have to mprotect every time we enter or leave the JS engine, only change the permissions for the current thread.

Comment 11

[Steve Fink [:sfink] [:s:]]

Whoa, that's pretty cool. Seems useful for Spectre mitigations too. Looks like it's still described as only available in "future Skylake server CPUs."

Comment 12

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #4)

This could be useful to us to track down some of our crashes.

Continually protecting/unprotecting the whole heap will be slow, so we could
unprotect pages on demand if we detect an access to them in the fault
handler. Also, we might want to only write-protect pages.

It could be done on some builds only to catch bugs, and enabled for short periods if we like. But that won't help protect against exploits.

Point is, it's useful to have this capability even if we don't use it all that much.

Comment 13

[Jon Coppeard (:jonco)]

We suspect that some of our consistent low volume crashes may be coming from some buggy code elsewhere in firefox accidentally scribbling over the GC heap

We don't really suspect this any more and don't have plans to implement this.