Closed Bug 1480012 Opened 3 years ago Closed 3 years ago

Make JS shell SAB mailbox accept Wasm memories and modules as well

Categories

(Core :: Javascript: WebAssembly, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(2 files, 3 obsolete files)

The shell uses a single-element mailbox mechanism for transmitting a SAB from one agent to others, see GetSharedArrayBuffer and SetSharedArrayBuffer in shell/js.cpp.

This mechanism is currently restricted to SharedArrayBuffer values but it should be generalized to WebAssembly memory objects so that we can run cross-thread wasm tests in the shell.

(Structured clone already handles memory objects.)
Structured cloning can already handle WebAssembly modules as well (not in the JS shell, but via browser hooks).  The mailbox mechanism should really handle modules too.
Summary: Make JS shell SAB mailbox accept Wasm memories as well → Make JS shell SAB mailbox accept Wasm memories and modules as well
The paranoia around reference count overflow exhibited in this patch is a result of a security issue we had where it was possible, through nefarious use of structured cloning, to overflow the refcount on the SharedArrayRawBuffer.  In principle it is possible to overflow the refcount on the Module by just reading the mailbox a large number of times, creating a lot of WasmModuleObjects.  Since we're in the shell this does not matter, but in the interest of hygiene I've done the error checking anyway.
Attachment #8996670 - Flags: review?(luke)
Attached file worker-threads.js (obsolete) —
Well, this crashes the shell so something must be running.  The problem seems to be looking up wasm protos in the shell worker.  Investigating.

In general we've had problems in the past with shell workers because they are just running in the same memory as the parent thread; they don't always have proper mutual exclusion on data structures that would not be shared in the browser but are shared here.
Comment on attachment 8996670 [details] [diff] [review]
bug1480012-generalize-mailbox.patch

OK, cancelling review because there are several gremlins on the shell worker side having to do with proto initialization and probably other things.
Attachment #8996670 - Flags: review?(luke)
Triggered a minor footgun: must remember to explicitly ensureConstructor for the WebAssembly object to ensure that prototypes are created.  Now works as it should with test case attached to this bug (for illustration).
Attachment #8996670 - Attachment is obsolete: true
Attachment #8996712 - Flags: review?(luke)
Attached file worker-threads.js
TC evolved and cleaned up; will land under different bug#; for illustration only.
Attachment #8996691 - Attachment is obsolete: true
Comment on attachment 8996712 [details] [diff] [review]
bug1480012-generalize-mailbox-v2.patch

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

::: js/public/RefCounted.h
@@ +76,5 @@
> +            uint32_t oldCount = mRefCnt;
> +            uint32_t newCount = oldCount + 1;
> +            if (newCount > INT32_MAX)
> +                return false;
> +            if (mRefCnt.compareExchange(oldCount, newCount))

Since MozRefCountType is a uintptr_t, I think the ref-count shouldn't be able to overflow.  If that's right, then let's remove this hunk.

::: js/src/shell/js.cpp
@@ +5740,5 @@
> +//
> +// These object types are shareable:
> +//
> +//   - SharedArrayBuffer
> +//   - WasmMemoryObject (only when mapping shared memory)

nit: perhaps "(when constructed with shared:true)"

@@ +5854,5 @@
> +            if (mbx->tag == MailboxTag::SharedArrayBuffer) {
> +                newObj = maybesab;
> +            } else {
> +                if (!GlobalObject::ensureConstructor(cx, cx->global(), JSProto_WebAssembly))
> +                    return false;

missing dropReference() on this failure path.  How about doing it instead with a MakeScopeExit() instead?

@@ +7079,5 @@
> +"  may be null.  setSharedObject performs an ordering memory barrier.\n"),
> +
> +    JS_FN_HELP("getSharedArrayBuffer", GetSharedObject, 0, 0,
> +"getSharedArrayBuffer()",
> +"  Obsolete alias for getSharedObject().\n"),

Are there so many test-cases that they can't be updated to the new name?
Attachment #8996712 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 8996712 [details] [diff] [review]
> bug1480012-generalize-mailbox-v2.patch
> 
> Review of attachment 8996712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/RefCounted.h
> @@ +76,5 @@
> > +            uint32_t oldCount = mRefCnt;
> > +            uint32_t newCount = oldCount + 1;
> > +            if (newCount > INT32_MAX)
> > +                return false;
> > +            if (mRefCnt.compareExchange(oldCount, newCount))
> 
> Since MozRefCountType is a uintptr_t, I think the ref-count shouldn't be
> able to overflow.  If that's right, then let's remove this hunk.

It is indeed uintptr_t, but observe that AddRef() checks that it never overflows int32_t, so that inconsistency needs to be resolved too.

Even then "shouldn't be able to overflow" is a weak defense.  The refcount on the SharedArrayRawBuffer shouldn't have been able to overflow either, but even so a bug elsewhere (a missing decref along an error path) allowed it to.

> @@ +7079,5 @@
> > +"  may be null.  setSharedObject performs an ordering memory barrier.\n"),
> > +
> > +    JS_FN_HELP("getSharedArrayBuffer", GetSharedObject, 0, 0,
> > +"getSharedArrayBuffer()",
> > +"  Obsolete alias for getSharedObject().\n"),
> 
> Are there so many test-cases that they can't be updated to the new name?

Some of the code using the old names is upstream in the test262 repo, in the SpiderMonkey support for atomics tests.  I think what we need to do here is land the new names and then start looking for places we can update, it may take a while to make the transition.  I will create a couple more patches here to update the names locally, but upstreaming the change will require some care.
Addresses all review comments except for uses in test262-related files of the old names.

Asking for feedback on the updates to the refcounting code.  I've added a static_assert on the size and cleaned up the run-time assertions a bit, and removed the CAS loop.  Let me know what you think.
Attachment #8996712 - Attachment is obsolete: true
Attachment #8996986 - Flags: feedback?(luke)
Comment on attachment 8996986 [details] [diff] [review]
bug1480012-generalize-mailbox-v3.patch

Carrying Luke's r+ from previous version.
Attachment #8996986 - Flags: review+
Comment on attachment 8996986 [details] [diff] [review]
bug1480012-generalize-mailbox-v3.patch

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

Ah, great catch and fix regarding lingering i32 assumptions.  Didn't know about getSharedArrayObject() in test262; makes sense.

::: js/src/shell/js.cpp
@@ +5845,5 @@
>              if (!buf->addReference()) {
>                  JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_SC_SAB_REFCNT_OFLO);
>                  return false;
>              }
> +            auto dropBuf = MakeScopeExit([buf]() { buf->dropReference(); });

I just recently learned (from grepping existing MakeScopeExit() uses to see if it was still the "turn a lambda into an RAII guard" of the day) you can elide empty parens of lambdas.
Attachment #8996986 - Flags: feedback?(luke) → feedback+
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61310ac63cfd
Generalize JS shell mailbox to Wasm memories and modules.  r=luke
https://hg.mozilla.org/mozilla-central/rev/61310ac63cfd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1480991
Depends on: 1495573
You need to log in before you can comment on or make changes to this bug.