Closed Bug 1333436 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript: Standard Library, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- disabled
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: anba, Assigned: lth)

Details

Attachments

(2 files, 1 obsolete file)

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.
Andre, is the call to setSharedArrayBuffer necessary here?  (If so it's a shell-only problem, and probably not s-s.)
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.
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.
(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.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Assertions in all the platform layers.
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.
Attachment #8829918 - Attachment is obsolete: true
Attachment #8829921 - Flags: review?(bbouvier)
Attachment #8829926 - Flags: review?(bbouvier)
Priority: -- → P1
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)
Attachment #8829921 - Flags: review?(bbouvier) → review+
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/?
Attachment #8829926 - Flags: review?(bbouvier) → review+
(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.
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
Gary, this is not s-s, but none of us are able to remove the flag.  Can you fix?  Thanks.
Flags: needinfo?(gary)
Comment on attachment 8829921 [details] [diff] [review]
bug1333436-assertions.patch

Approval Request Comment

See comment on the other patch.
Attachment #8829921 - Flags: approval-mozilla-aurora?
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
Attachment #8829926 - Flags: approval-mozilla-aurora?
I can't do it either, passing the baton to Al.
Flags: needinfo?(gary) → needinfo?(abillings)
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Is 52 also affected?
Flags: needinfo?(lhansen)
Comment on attachment 8829926 [details] [diff] [review]
bug1333436-test-and-fix.patch

Fix for standards compliance, let's uplift for aurora 53.
Attachment #8829926 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8829921 [details] [diff] [review]
bug1333436-assertions.patch

Uplift assertions patch to aurora too.
Attachment #8829921 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(lhansen)
You need to log in before you can comment on or make changes to this bug.