1533932 - Assertion failure: [barrier verifier] Unmarked edge: Object 0x1c84012cd040 'vector element' edge to Object 0x1c84012ce060, at js/src/gc/Verifier.cpp:382

Status: VERIFIED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 54ed5eac2abc (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --wasm-gc):

function wasmEvalText(str, imports) {
    let binary = wasmTextToBinary(str);
        m = new WebAssembly.Module(binary);
    return new WebAssembly.Instance(m, imports);
}
gczeal(4, 8);
evaluate(`
    let ins = wasmEvalText(\`
        (module
           (gc_feature_opt_in 3)
           (table (export "t") 10 anyref)
           (func (export "set_anyref") (param i32) (param anyref)
             (table.set (get_local 0) (get_local 1)))
           (func (export "set_null") (param i32)
             (table.set (get_local 0) (ref.null)))
        )
    \`);
    let x = {};
    ins.exports.set_anyref(3, x);
    ins.exports.set_null(3);
`);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::gc::GCRuntime::endVerifyPreBarriers (this=this@entry=0x7ffff5f1c6b8) at js/src/gc/Verifier.cpp:383
#1  0x0000555556066136 in js::gc::GCRuntime::maybeVerifyPreBarriers (this=0x7ffff5f1c6b8, always=<optimized out>) at js/src/gc/Verifier.cpp:428
#2  0x00005555558e3a22 in Interpret (cx=0x7ffff5f17000, state=...) at js/src/vm/Interpreter.cpp:4384
#3  0x00005555558e9e66 in js::RunScript (cx=0x7ffff5f17000, state=...) at js/src/vm/Interpreter.cpp:420
#4  0x00005555558ed50d in js::ExecuteKernel (cx=<optimized out>, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x7ffff4dd70a0) at js/src/vm/Interpreter.cpp:779
#5  0x00005555558ed959 in js::Execute (cx=<optimized out>, cx@entry=0x7ffff5f17000, script=script@entry=..., envChainArg=..., rval=rval@entry=0x7ffff4dd70a0) at js/src/vm/Interpreter.cpp:813
#6  0x0000555555a02c73 in ExecuteScript (cx=0x7ffff5f17000, scope=scope@entry=..., script=script@entry=..., rval=rval@entry=0x7ffff4dd70a0) at js/src/vm/CompilationAndEvaluation.cpp:438
#7  0x0000555555a5409f in ExecuteScript (cx=<optimized out>, envChain=..., scriptArg=..., rval=0x7ffff4dd70a0) at js/src/vm/CompilationAndEvaluation.cpp:458
#8  0x000055555585e6a7 in Evaluate (cx=<optimized out>, cx@entry=0x7ffff5f17000, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:2245
#9  0x00005555558f87f9 in CallJSNative (cx=0x7ffff5f17000, native=0x55555585da50 <Evaluate(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:440
[...]
#23 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:10958
rax	0x555557c41240	93825033048640
rbx	0x7fffffffb4b0	140737488336048
rcx	0x7ffff6c1c2dd	140737333281501
rdx	0x0	0
rsi	0x555556ac36d8	93825014707928
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffb8f0	140737488337136
rsp	0x7fffffffb3e0	140737488335840
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x0	0
r11	0x0	0
r12	0x1c84012cd040	31353280974912
r13	0x555556ac90c5	93825014730949
r14	0x7ffff0a54330	140737230750512
r15	0x1c84012ce060	31353280979040
rip	0x5555560606d4 <js::gc::GCRuntime::endVerifyPreBarriers()+1556>
=> 0x5555560606d4 <js::gc::GCRuntime::endVerifyPreBarriers()+1556>:	movl   $0x0,0x0
   0x5555560606df <js::gc::GCRuntime::endVerifyPreBarriers()+1567>:	ud2

Marking s-s until investigated if this is in the experimental wasm-gc or the regular wasm-gc code.

Comment 1

[Lars T Hansen [:lth]]

Will look at this tomorrow with Julian, appreciate any attention by GC folks before then.

Comment 2

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8085449bd426
user:        Lars T Hansen
date:        Fri Mar 01 09:55:31 2019 +0100
summary:     Bug 1488205 - Repurpose the --wasm-gc switch for GC. r=jseward

This iteration took 522.237 seconds to run.

Comment 3

[Julian Seward [:jseward]]

A couple of basic observations:

• this fails for both 32- and 64-bit x86 builds
• it fails for both --wasm-compiler=baseline and =ion
• it fails even with no command line flags to the shell

Comment 4

[Lars T Hansen [:lth]]

Also, (gc_feature_opt_in 3) is not required, nor --wasm-gc, as this is a reftypes feature and they are on by default in nightly.

Note this is a nightly-only problem, the feature is disabled by #ifdef in other versions.

Comment 5

[Lars T Hansen [:lth]]

Just run this in a linux64 debug shell without any options:

function wasmEvalText(str) {
    return new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(str)));
}
gczeal(4, 8);
let ins = wasmEvalText(
    `(module
       (table (export "t") 10 anyref)
       (func (export "set_anyref") (param i32) (param anyref)
         (table.set (get_local 0) (get_local 1))))`);
ins.exports.set_anyref(3, {});
ins.exports.set_anyref(3, null);

Comment 6

[Lars T Hansen [:lth]]

This is because the GCVector<T> that is used to hold table entries does not perform a prebarrier, contrary to what I had thought (given its name and the comments around its definition about "simplifying pointer operations").

Comment 7

[Lars T Hansen [:lth]]

Attachment: Bug 1533932 - Handle prebarriers properly for wasm tables. r?bbouvier

Three problems needed to be fixed actually:

• JS::Heap<> only has a postbarrier, not a prebarrier also; this is
  what triggered the original error report. The type in
  table-of-anyref therefore needs to be HeapPtr, so that we get both
  barriers. (It can't be GCPtr because the vector storage may be
  moved as the table grows.)

• GCVector<> can't deal with null values, it unconditionally traces
  all of its elements even if they are null, and this crashes quickly.
  So the vector type for table-of-anyref must be Vector, and we must
  walk it explicitly, checking for null, when we trace a Table.

• (Fix for this is WIP.)

  The API for table.get returns a pointer into underlying vector
  storage or null; if not null, the pointer must be dereferenced to
  obtain the actual value. Since the vector storage may move during
  GC, the pointer is really only valid until the next operation that
  might trigger GC.

  In the WIP patch we document this by slightly changing the name of
  the function that returns the pointer, and by adding documentation,
  but this is not really good enough. Given optimizations that may
  move code, it's not completely obvious that marking the node
  unmoveable will be enough, but given that we're currently making
  calls for all heap allocations and all JS imports, it's probably OK.

  It would be better to fix it, but part of the problem is that the
  API is shared between Baldr and Rabaldr, and they want different
  constraints, probably.

Comment 8

[Lars T Hansen [:lth]]

The fix is fine as far as it goes but I'm pondering whether to try to fix the third problem while I'm here. Marking the deref node as non-moveable for table operations might be good enough for now.

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Lars T Hansen [:lth] from comment #7)

• JS::Heap<> only has a postbarrier, not a prebarrier also

JS::Heap<> is designed to be used externally for roots in the C++ heap so it doesn't have a prebarrier. HeapPtr should be used for general pointers to GC things in the JS engine heap. I guess this is pretty confusing.

• GCVector<> can't deal with null values, it unconditionally traces
  all of its elements even if they are null

That's not meant to be the case. GCVector traces everything but GCPointerPolicy::trace() has a null check. I'm not sure what went wrong here.

Comment 10

[Lars T Hansen [:lth]]

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

• GCVector<> can't deal with null values, it unconditionally traces
  all of its elements even if they are null

That's not meant to be the case. GCVector traces everything but GCPointerPolicy::trace() has a null check. I'm not sure what went wrong here.

Yes, that is mysterious. It was not a one-off thing. I am reasonably sure I tried GCVector<HeapPtr<>> also, before making my own loop. I can test again, just to be sure.

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Lars T Hansen [:lth] from comment #10)

Oh, GCPolicy<js::HeapPtr<T>> doesn't have a null check and calls TraceEdge() (as does the one for JS::Heap). These could call TraceNullableEdge() instead.

Comment 12

[Lars T Hansen [:lth]]

Jon, will you file + fix or shall I?

Comment 13

[Jon Coppeard (:jonco)]

Lars agreed to fix this in bug 1536106.

Comment 14

[Lars T Hansen [:lth]]

Comment on attachment 9051271 [details]
Bug 1533932 - Handle prebarriers properly for wasm tables. r?bbouvier

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's hard - the feature is unavailable on non-Nightly and disabled by default on Nightly. Even when enabled we're likely to crash very quickly. sec-high is probably overstating the matter.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: None
• If not all supported branches, which bug introduced the flaw?: Bug 1508559
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Will not cause regressions; the feature is experimental + behind a flag, the fix is very local.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9051271 [details]
Bug 1533932 - Handle prebarriers properly for wasm tables. r?bbouvier

sec-approval+ for trunk.

Comment 16

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision b2f1edb41241).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38c2cb5142eb

Based on comment 14, it doesn't sound like this needs uplift anywhere since it's a Nightly-only feature.

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.