1333436 - SharedArrayBuffer.prototype.slice needs to check "Shared Data Block values" are different

Status: RESOLVED FIXED

Description

[André Bargull [:anba]]

Test case:
---
var sab = new SharedArrayBuffer(1 * Int32Array.BYTES_PER_ELEMENT);
sab.constructor = {
  [Symbol.species]: function(length) {
    return getSharedArrayBuffer();
  }
};

setSharedArrayBuffer(sab);

sab.slice();
---

This is actually a spec error, but also leads to UB when memcpy is called in js::jit::AtomicOperations::memcpySafeWhenRacy.

Comment 1

[Lars T Hansen [:lth]]

Andre, is the call to setSharedArrayBuffer necessary here?  (If so it's a shell-only problem, and probably not s-s.)

Comment 2

[André Bargull [:anba]]

I don't know if workers (or other html APIs) allow to pass SharedArrayBuffers and also support bidirectional transfer. If that's not the case, we can open this bug.

Comment 3

[Lars T Hansen [:lth]]

Well, it's always possible to end up with two separate SharedArrayObject objects that reference the same buffer, whether in the shell or in the browser.  I can send a SAB x to my worker and it can send it back to me, and I would get a different SAB object y, but x and y reference the same shared memory data block.

I see what you're saying.  The test that the two objects are not the same in Step 14 of the slice algorithm is insufficient to make the call to CopyDataBlock safe, because CopyDataBlock stipulates that its blocks are different.  (And it stipulates that in order to allow us to use memcpy in the implementation, and memcpy is UB for overlapping ranges.)

It's probably safe to open this since it's not like memcpy will scribble onto memory outside the block, I think - we're not reallocating anything.  But I'll look into fixing it before I make the call on that.

It will be useful to assert the non-overlapping condition in memcpySafeWhenRacy too.

Comment 4

[Lars T Hansen [:lth]]

Created attachment 8829918 [details] [diff] [review]
bug1333436-slice-same-memory.patch

(Not a fix.)

The test case, restructured and in its right place, along with assertions in the C++ code that are triggered by the test case.

Comment 5

[Lars T Hansen [:lth]]

Created attachment 8829921 [details] [diff] [review]
bug1333436-assertions.patch

Assertions in all the platform layers.

Comment 6

[Lars T Hansen [:lth]]

Created attachment 8829926 [details] [diff] [review]
bug1333436-test-and-fix.patch

Test case and fix.  Here I've opted to leave the test that's currently in the spec -- testing the identity of the SAB objects -- in place even though it is subsumed by the subsequent test on the identity of the memory blocks.  We'll clean this up once there's a spec fix, if necessary.

Comment 7

[Benjamin Bouvier [:bbouvier]]

Comment on attachment 8829921 [details] [diff] [review]
bug1333436-assertions.patch

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

::: js/src/jit/x86-shared/AtomicOperations-x86-shared.h
@@ +339,5 @@
>  inline void
>  js::jit::AtomicOperations::memcpySafeWhenRacy(void* dest, const void* src, size_t nbytes)
>  {
> +    MOZ_ASSERT(!((char*)dest <= (char*)src && (char*)src < (char*)dest+nbytes));
> +    MOZ_ASSERT(!((char*)src <= (char*)dest && (char*)dest < (char*)src+nbytes));

Would it make sense to make these MOZ_RELEASE_ASSERT? (in that case, it'd be nice to store (char*)dest and (char*)src in local variables to make these two asserts easier to read)

Comment 8

[Benjamin Bouvier [:bbouvier]]

Comment on attachment 8829926 [details] [diff] [review]
bug1333436-test-and-fix.patch

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

::: js/src/vm/SelfHosting.cpp
@@ +1046,5 @@
> +    if (!obj1) {
> +        ReportAccessDenied(cx);
> +        return false;
> +    }
> +    JSObject* obj2 = CheckedUnwrap(&args[1].toObject());

Could you add a comment at the top that it's expected that both passed arguments are actually SAB objects or wrappers? (in our case, step 13 does the check for us)

also  uber-nit: s/obj1/lhs/ and s/obj2/rhs/?

Comment 9

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Comment on attachment 8829921 [details] [diff] [review]
> bug1333436-assertions.patch
> 
> Review of attachment 8829921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/x86-shared/AtomicOperations-x86-shared.h
> @@ +339,5 @@
> >  inline void
> >  js::jit::AtomicOperations::memcpySafeWhenRacy(void* dest, const void* src, size_t nbytes)
> >  {
> > +    MOZ_ASSERT(!((char*)dest <= (char*)src && (char*)src < (char*)dest+nbytes));
> > +    MOZ_ASSERT(!((char*)src <= (char*)dest && (char*)dest < (char*)src+nbytes));
> 
> Would it make sense to make these MOZ_RELEASE_ASSERT?

I expect these functions to be fairly hot.  I could be wrong, but until there's proof to the contrary: no.

Comment 10

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e8bedb9a623

Comment 11

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c652f44f57ad2171df24eb6d5bc2c3f32495f92
Bug 1333436 - Add assertions about overlaps to the safe-for-races memcpy blocks. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/7083b7989fb108bd5fd6e952f7efd6644edbdf59
Bug 1333436 - Guard against slicing onto the same shared memory block. r=bbouvier

Comment 12

[Lars T Hansen [:lth]]

Gary, this is not s-s, but none of us are able to remove the flag.  Can you fix?  Thanks.

Comment 13

[Lars T Hansen [:lth]]

Comment on attachment 8829921 [details] [diff] [review]
bug1333436-assertions.patch

Approval Request Comment

See comment on the other patch.

Comment 14

[Lars T Hansen [:lth]]

Comment on attachment 8829926 [details] [diff] [review]
bug1333436-test-and-fix.patch

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: Standards compliance will be sub-par
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: In progress
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Other patch on this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: It places only a limit on current behavior, and no real-life code exists yet that touches that limt
[String changes made/needed]: None

Comment 15

[Gary Kwong [:gkw] [:nth10sd]]

I can't do it either, passing the baton to Al.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c652f44f57a
https://hg.mozilla.org/mozilla-central/rev/7083b7989fb1

and unhiding per comment #12

Comment 17

[Liz Henry (:lizzard) (needinfo? me)]

Is 52 also affected?

Comment 18

[Liz Henry (:lizzard) (needinfo? me)]

Comment on attachment 8829926 [details] [diff] [review]
bug1333436-test-and-fix.patch

Fix for standards compliance, let's uplift for aurora 53.

Comment 19

[Liz Henry (:lizzard) (needinfo? me)]

Comment on attachment 8829921 [details] [diff] [review]
bug1333436-assertions.patch

Uplift assertions patch to aurora too.

Comment 20

[Lars T Hansen [:lth]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #17)
> Is 52 also affected?

It is, but the SharedArrayBuffer API is off-by-default in 52 and earlier.  IMO we should not worry about fixing it for that reason.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47435662c2ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/005919b07171