1448589 - Add ASan/MSan/Valgrind instrumentation for GC and jit-code allocators

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Jan de Mooij [:jandem]]

We can use the MOZ_MAKE_MEM_UNDEFINED and MOZ_MAKE_MEM_NOACCESS macros for our GC and jit-code allocators.

For ASan, these macros work as follows:

MOZ_MAKE_MEM_NOACCESS - mark a memory region as poisoned (all reads/writes will be considered invalid by ASan)
MOZ_MAKE_MEM_UNDEFINED - unpoison a region

Furthermore, MSan and Valgrind can also use MOZ_MAKE_MEM_UNDEFINED to detect use of uninitialized values.

I added an argument to JS_POISON/Poison so all memory that is poisoned using these functions must be marked as no-access or undefined. I also added instrumentation to gc::MarkPagesUnused and the JIT code allocator.

The hope is that this will make ASan/MSan/Valgrind more useful for finding and tracking down GC hazards.

I'll attach a WIP patch that passes jit-tests locally with ASan, for fuzzing. It also has some improvements for nursery chunk poisoning: after sweeping a nursery chunk, we will now also poison the chunk trailer instead of initializing it again.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch for fuzzing

decoder, can we do some ASan fuzzing with this patch before I push it to Try?

Comment 2

[Jan de Mooij [:jandem]]

We could be more precise when it comes to GC memory (use MOZ_MAKE_MEM_NOACCESS more), but the main difficulty is that we can do GC allocation from JIT code. However, we don't do that if gc zeal is active, so I think we should consider doing the more precise thing only when gc zeal is enabled. I can try that as a follow-up.

Comment 3

[Jason Orendorff [:jorendorff]]

Fantastic.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Nursery poisoning changes

This splits poisonAndInit in separate poisonAndInit and poisonAfterSweep methods. poisonAfterSweep also poisons the chunk trailer.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8963506 [details] [diff] [review]
Part 1 - Nursery poisoning changes

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

Great, thanks for doing this.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Add instrumentation

When it comes to the GC + ASan, the effect is mostly poisoning GC arenas and chunks. Finer granularity (GC-thing level) is hard considering allocations can be done from JIT code. It will also require more instrumentation but we don't want to have too much instrumentation to the point where it would become a burden to developers. I'm still interested in trying finer granularity poisoning as a followup (probably behind a runtime flag) but will have to see how that turns out.

ExecutableAllocator is a pool allocator for JIT code. With this patch we mark code pools as inaccessible by default and sweeping/poisoning a JitCode* instance will mark that code region as inaccessible again.

If you can think of better names for the MemCheckKind enum and SetMemCheckKind function, please let me know - I'm not very happy with the current names but also couldn't think of anything better.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8963514 [details] [diff] [review]
Part 2 - Add instrumentation

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

::: js/src/gc/Allocator.cpp
@@ +678,5 @@
>  void
>  Chunk::init(JSRuntime* rt)
>  {
> +    /* The chunk may still have some regions marked as no-access. */
> +    MOZ_MAKE_MEM_UNDEFINED(this, ChunkSize);

It took me a while to realise that we needed to do this first just so that we could poison the chunk.

::: js/src/gc/GC.cpp
@@ +587,5 @@
>              firstThingOrSuccessorOfLastMarkedThing = thing + thingSize;
>              nmarked++;
>          } else {
>              t->finalize(fop);
> +            JS_POISON(t, JS_SWEPT_TENURED_PATTERN, thingSize, MemCheckKind::MakeUndefined);

We can't mark these as unmapped because we'd need to mark them as undefined again before allocating into them, right?  And that's hard because the JIT can allocate.  If you want to do this I think marking as undefined in ArenaLists::setFreeList should work.  That's called even for JIT allocations.

::: js/src/jit/ExecutableAllocator.cpp
@@ +344,5 @@
>              pool->mark();
>          }
>  
>          memset(ranges[i].start, JS_SWEPT_CODE_PATTERN, ranges[i].size);
> +        MOZ_MAKE_MEM_NOACCESS(ranges[i].start, ranges[i].size);

Could we use JS_POISON here?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #7)
> > +            JS_POISON(t, JS_SWEPT_TENURED_PATTERN, thingSize, MemCheckKind::MakeUndefined);
> 
> We can't mark these as unmapped because we'd need to mark them as undefined
> again before allocating into them, right?  And that's hard because the JIT
> can allocate.  If you want to do this I think marking as undefined in
> ArenaLists::setFreeList should work.  That's called even for JIT allocations.

That's a good point. Also, there are GC things we definitely won't allocate in JIT code (scripts, symbols, shapes, etc) and there we could be more strict. I'll keep this bug open and experiment with that. (For ASan even JIT code allocation is okay because it's not instrumented, but Valgrind won't accept that.)

> >          memset(ranges[i].start, JS_SWEPT_CODE_PATTERN, ranges[i].size);
> > +        MOZ_MAKE_MEM_NOACCESS(ranges[i].start, ranges[i].size);
> 
> Could we use JS_POISON here?

We want to poison JIT code in release builds too; I'll add a comment.

We could add JS_ALWAYS_POISON for this, but Poison also has this thing where in debug builds it tries to write an invalid ObjectValue and I don't think we want that for JIT code poisoning. This also applies to some other places where we use JS_POISON, we should probably make that behavior optional.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b24c2041026
part 1 - Refactor nursery poisoning a bit; poison chunk trailer after sweeping. r=jonco

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7b24c2041026

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/764a683dfa2a
part 2 - Add memory sanitizer instrumentation to GC and JIT allocators. r=jonco

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/764a683dfa2a

Comment 13

[Jan de Mooij [:jandem]]

Let's close this one to simplify bug tracking. We can file new bugs if needed.