<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Comment 1

•

8 months ago

Looks like it is a barrier verifier issue -- it is confused about why instance reference disappeared between verify runs (see https://searchfox.org/firefox-main/source/js/src/wasm/WasmPI.cpp#109 ). The solution is to track instances in location(s) that will be "pre-barried" and update these on moribund/unwind state.

It only fails with gczeal(4) mode
A real crash would only be possible if a WasmInstanceObject had no other roots, which would be practically impossible since a caller closure will have a reference to the instance.

Proposed by AI fix, which has a limit on amount of instances to track:

  1. Track: during TraceSuspendableStack (when we have access to the stack), store unique instance objects in TracedInstance  
  fixed slots on SuspenderObject — but only when called from a non-marking tracer (the barrier verifier), not from GCMarker
  2. Pre-barrier on moribund: when setMoribund/unwind is called, setFixedSlot(slot, NullValue()) fires HeapSlot::pre() →      
  ValuePreWriteBarrier for the old value (the instance) → instance gets its mark bit → verifier sees it as marked → no crash

  The barrier fires at exactly the right moment: between startVerify (which recorded the SuspenderObject → WasmInstanceObject
  edge) and endVerify (which checks that edge).

Assignee

Comment 2

•

8 months ago

Why is an instance edge special here? Wouldn't this fail for any kind of GC thing traced by TraceSuspendableStack?

Flags: needinfo?(ydelendik)

Comment 3

•

8 months ago

Reduced test case with the same error:

gczeal(4,1);
const inst1 = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(`
  (module
    (import "m" "f" (func))
    (func call 0)
    (export "w0" (func 1))
  )
`)), { m: { f: () => {} } });
WebAssembly.promising(inst1.exports.w0)();

Wouldn't this fail for any kind of GC thing traced by TraceSuspendableStack?

It looks like it may, trying to construct a test case for that

Assignee

Comment 4

•

8 months ago

Just to confirm, this sounds like the barrier verifier needs to be updated to understand wasm suspendable stacks? Not that there is a bug in our implementation.

Comment 5

•

8 months ago

(In reply to Ryan Hunt [:rhunt] from comment #4)

Just to confirm, this sounds like the barrier verifier needs to be updated to understand wasm suspendable stacks? Not that there is a bug in our implementation.

That was my first impression, but now I doubt that since I don't really get how switching stack plays here. We have "tick" between interpreter run which does not expect some stack roots manipulation trickery. Also it looks like direct call via CallOnMain to JS does not help here, that does one of the verifier runs, before crashing.

Probably sfink or jonco will have better picture here.

Comment 6

•

8 months ago

Here is few findings:

Stack slots never need pre-barriers, based on endVerifyPreBarriers NextNode() use
We are creating an edge to verify because SuspenderObject::trace() -> TraceSuspendableStack() nesting, verifier tracks via the curnode

There are several alternatives to address the issue:

Track suspenders somewhere, but it needs to be done "weakly" to allow stacks to be GC'ed
add isStackLike flag on the EdgeValue struct
some other trickery with curnode

So it looks like it is more bug in the verifier working with suspendable stack, and not security issue with barriers on suspendable stack frames.

Please feel free to correct my assumption -- I'm trying to learn how prebarriers verifier works atm.

Flags: needinfo?(ydelendik)

Comment 7

•

8 months ago

What I understand is happening here:

The pre-barrier verifier snapshots the heap, calling TraceSuspendableStack when it is in state CalledOnMain (so it is on the heap but referenced by the active stack).
The stack transitions CalledOnMain -> Active -> Moribund
This calls leaveSuspendableStack(), which sets activeSuspender_ = nullptr, which is a pre-barriered write that marks the SuspenderObject and pushes it on the mark stack for traversal.
releaseStackMemory() then frees the stack memory, which is when the stack's contents are truly no longer reachable.
When the SuspenderObject's children are traversed, it's basically empty.
The verifier runs and finds an edge (SuspenderObject -> WA.Instance) where the target is not marked.

The CalledOnMain -> Active transition seems sketchy. This can be thought of as equivalent to taking heap data and pushing it onto the stack, then nulling out the heap data. This nulling would be pre-barriered, which is important here because the live stack is part of the root set and thus does not require barriers, because the live stack is treated as "temporary" storage of things that were known to be live when they were pushed. They can be popped off freely, as the original references either still exist or were pre-barriered when they were cleared or modified and so will have been marked and kept alive.

If the CalledOnMain -> Active transition were treated as a "resume", then the stack frames' contents would have been pre-barriered. Instead, they were lost when they were made active and popped.

The fix proposed in comment 1 seems like it would work. It's another way of ensuring that references on an Active stack are not last references. But it seems kind of clunky and redundant.

Another fix would be to barrier trace the stack frame on CalledOnMain -> Active transitions. In release, this would only take time during incremental GCs. But this could still happen quite a lot so could slow down iGC?

Then again, Yury said in comment 1 that we're already guaranteed to have another reference, which means this is a verifier-only problem. But if that's the case, then why aren't those references being found when doing EndVerifyBarriers()? Where are those caller closures stored? In the test, it seems like by the time the suspendable wasm function finishes, the calling script is complete and no longer holding onto inst1? Would the Promise hold the Instance for some reason?

Why is an instance edge special here? Wouldn't this fail for any kind of GC thing traced by TraceSuspendableStack?

It seems like it. TraceSuspendableStack traces the instance edges and calls instance->traceFrame(), which traces AnyRefs and a bunch of other stuff. Either those are reachable via other edges, or the Instance was merely the first thing to be reported? Hm, in the test case, ignoring the Instance failure finds nothing else. Then again, it looks like the only AnyRef in the Instance I looked at is a backref to the SuspenderObject pointer, which we know is marked. So maybe if the test could add an AnyRef that is the last reference to some GC thing? I don't know how to do that.

Jon Coppeard (:jonco)

Comment 8

•

8 months ago

(In reply to Yury Delendik (:yury) from comment #1)
I'm jumping in without looking at this in detail, but I think this is unlikely to be a verifier issue. The verifier has uses the same machinery as the rest of the GC and has worked fine for a long time. It's more likely to be a missing barrier somewhere.

I saw that while activeSuspender_ has a barrier suspenders_ does not. This should probably be at a GCHashSet containing HeapPtr<SuspenderObject*>.

I don't remember how the suspended stack system works exactly but I think sometimes the stacks are weak and sometimes they are treated as roots, is that right? In that case a missing barrier when going between these states seems likely, as Steve suggested above.

Comment 9

•

8 months ago

I don't remember how the suspended stack system works exactly but I think sometimes the stacks are weak and sometimes they are treated as roots, is that right? In that case a missing barrier when going between these states seems likely, as Steve suggested above.

We are overdoing tracing of the CalledOnMain -> Active (there was GC bug 1966549 related to that). On another hand, we have to keep stack+suspenders weakly references (see bug 1915677). We decided:

During minor GC, the SuspenderContext::traceRoots traces all suspended stacks frames.
During major GC, only reachable SuspenderObject (and suspended) stacks frames are traced.

Notice that the latter traced as non-root, but as edges of suspender. And verifier has a problem with that: it suppose to skip roots for undocumented reason. Is it correct that the stacks frames slots and instances has to be considered as roots (as on main stack) and be ignored by verifier?

Jon Coppeard (:jonco)

Comment 10

•

8 months ago

(In reply to Yury Delendik (:yury) from comment #9)

We are overdoing tracing of the CalledOnMain -> Active (there was GC bug 1966549 related to that). On another hand, we have to keep stack+suspenders weakly references (see bug 1915677). We decided:

During minor GC, the SuspenderContext::traceRoots traces all suspended stacks frames.
During major GC, only reachable SuspenderObject (and suspended) stacks frames are traced.

Right, we need to update pointers to promoted nursery objects in minor GC. In major GC we should only trace things that are reachable.

Notice that the latter traced as non-root, but as edges of suspender. And verifier has a problem with that: it suppose to skip roots for undocumented reason. Is it correct that the stacks frames slots and instances has to be considered as roots (as on main stack) and be ignored by verifier?

I'm not sure what you mean. The verifier is pretty simple: it periodically makes a copy of the heap graph, enables barriers, and then some time later checks that anything removed from the heap we marked by a barrier. This verifies the snapshot at the beginning invariant which we depend on to make incremental marking work.

For this case, AIUI the active stack is a root and the non-active stacks are not (and are traced as part of their SuspenderObject). Data structures holding pointers to any of these things (e.g. activeSuspender_ and suspenders_) should have barriers unless they are always traced as part of root marking.

Comment 11

•

8 months ago

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

I'm not sure what you mean. The verifier is pretty simple: it periodically makes a copy of the heap graph, enables barriers, and then some time later checks that anything removed from the heap we marked by a barrier. This verifies the snapshot at the beginning invariant which we depend on to make incremental marking work.

I'm looking at https://searchfox.org/firefox-main/source/js/src/gc/Verifier.cpp#377-382 which skips all roots. If I understand activation tracing correctly, the TraceJitActivation on the main stack traces instances and stack slots as roots. We do the same at wasm::Context::traceRoots during minor GC. Means verifier just skips them (?). I'm just curious if we shall skip the same exact instances and stack slots as roots (found during major GC) in the verifier validation.

Comment 12

•

8 months ago

I don't actually know why the endVerifyBarriers pass doesn't look at the roots, but examining them would only trigger more failures if I'm not mistaken? You'd still have a (non-root) edge from a reachable SuspenderObject to an unmarked Instance that would cause the assertion here.

I don't think barriering suspenders_ is going to help here. (It's probably correct to barrier it, I just don't think it'll affect this bug.) Something is being removed from the heap without being barriered, but it's not because its owner (the SuspenderObject) is being removed. In fact, it is only being removed semantically; the memory doesn't get copied or overwritten or anything. A state field in SuspenderObject is changed which makes the suspended (well, CalledOnMain) stack active. So we take a snapshot while the stack is suspended, trace it, and find the Instances. Then the stack is made active and considered part of the root set and thus unbarriered, which means the Instances have vanished from the heap without a barrier.

The CalledOnMain -> Active transition needs to be barriered somehow. The straightforward way would be to do pretty much the same thing as is done when a stack goes from suspended -> resumed. Note that it looks like the Instance doesn't directly store its "children"; they're on the suspendable stack itself. So they also disappear from the heap without being barriered. It would be nice to have a test case where the Instance is holding the last reference to something on its stack. Anyway, that means the barrier needs to include everything that is disappearing from the heap (so not just mark the InstanceObjects, but also call instance->traceFrame() for all the frames). The barrier would still do nothing if barriers are not currently needed.

More speculatively, you could try to keep another copy of the stack contents (here, the Instance pointers). This could be considered to be a "preemptive pre-barrier": imagine you "barrier" the CalledOnMain -> Active transition by scanning the stack and copying all Instances to a known-reachable part of the heap. That's different from the usual pre-write barrier, but it still achieves snapshot-at-the-beginning. Then, you use twisted logic to say that you're doing that barrier in advance (probably during the preceding TraceSuspendableStack), so you can argue that the barrier will have run by the time a snapshot is consulted (by either the verifier or actual incremental GC). But this route is risky, since any mutation of the set of Instances would need to be barriered as well. Also, the Instances probably aren't enough, since you need to handle everything else on the stack that is reachable through those Instances.

Comment 13

•

8 months ago

We talked about this during our GC meeting, and I think I came up with a way of clarifying part of this:

When suspended/CalledOnMain, this stack data is on the heap. When it transitions to Active, it's tempting to think of it as both on the heap and the stack -- and in fact, it's reachable both ways. Anything that scans the heap (like incremental GC) can reach that memory, so the snapshot from the beginning is complete.

The problem is that popping a pointer off the stack is unbarriered. So if you're considering this memory to be on both the stack and the heap, that means you're removing something from the heap without a barrier.

That's actually what my first attempt at fixing this ran into: I did a TraceSuspendableStack in setMoribund, which asserted because there were no jit activations to scan. Everything had already been popped off the stack so it was too late to find it either via scanning the stack for roots or scanning the heap.

This clearly violates snapshot at the beginning: if you start an incremental GC while the SuspenderObject is CalledOnMain, then the Instance object is live. If you then activate that stack during the iGC, and that stack gets fully popped, then the Instance will never get marked and will get collected.

Now, we could say that we could loosen snapshot-at-the-beginning semantics in this case and allow the Instance to die. It's only a problem if the Instance pointer is written to an already-marked object. But I don't see anything preventing that from happening! As in, you're running arbitrary JS code on a stack containing the Instance, and so that code could cause the Instance to be stored somewhere else that might already be marked. Perhaps that's not a problem with Instances, but it still seems likely that anything reachable through that Instance, things that are also sitting on the stack, would have the same problem and would be much easier to store elsewhere. Those create really bad UAFs. (Also, I think concurrent marking also depends on snapshot-at-the-beginning, but I haven't worked through the scenario there.)

Assignee

Comment 14

•

8 months ago

I haven't been able to read all of the above yet. I just need to say that I have patches that rewrite most of the JS-PI implementation in bug 2023217, including the tracing and barriers code. It might be worth deferring some of this discussion until that's being reviewed.

Assignee

Updated

•

8 months ago

Updated

•

8 months ago

status-firefox150: --- → disabled

Comment 15

•

8 months ago

(In reply to Ryan Hunt [:rhunt] from comment #14)

I haven't been able to read all of the above yet. I just need to say that I have patches that rewrite most of the JS-PI implementation in bug 2023217, including the tracing and barriers code. It might be worth deferring some of this discussion until that's being reviewed.

Fwiw, you can probably just read comment 13 and skip the rest. I wrote enough confusing crap in other comments that I'm going to have the whole thread printed, bound, and then burned in a historically accurate Victorian era fireplace.

I haven't looked at bug 2023217 enough to gauge whether it might fix the problem.

Assignee

Comment 16

•

7 months ago

I ran the test case on the new patches and it no longer crashes.

Reading comment 13, the new patches try to maintain snapshot at the beginning by adding a 'resume barrier' before resuming a suspended stack:

if (zone->needsMarkingBarrier()) {
  stack->traceNow();
}

This should prevent any of the unbarriered reads/writes to the wasm locals on the stack from moving an untraced thing to somewhere that's already been traced.

Assignee

Updated

•

7 months ago

Depends on: 2023217

Updated

•

7 months ago

Duplicate of this bug: 2023286

Steven DeTar [:sdetar]

Comment 18

•

7 months ago

Move to S3 because is disabled

Severity: S2 → S3

Daniel Veditz [:dveditz]

Updated

•

7 months ago

Blocks: wasm-jspi