Preventing ArrayBufferViews backed by SharedArrayBuffers from being used in Blink methods

[Ben Smith]

Hi blink-dev,

I'm working on implementing SharedArrayBuffers for Blink. It's currently working, behind a flag. The spec currently states that the ArrayBufferView hierarchy is shared for ArrayBuffers and SharedArrayBuffers. So:

var sab = new SharedArrayBuffer(128);

var u32a = new Uint32Array(sab);

This means that currently an ArrayBufferView backed by a SharedArrayBuffer can be passed to any API that takes an ArrayBufferView:

var xhr = new XMLHttpRequest();

xhr.open(...)

xhr.send(u32a); // currently OK

We may allow some methods to do this, but I'd like that list to be opt-in instead of opt-out. This issue has a list of the affected interfaces I've found so far.

I'm working on a CL to fix this: https://codereview.chromium.org/1526183004/. This CL modifies the generated binding code to introduce an additional check to methods that take an ArrayBufferView. Is this the right approach?

Anyway, it seems to work, and I'd like to add some Layout Tests for it. Currently I have a virtual test suite to pass the additional flags needed to enable SharedArrayBuffers. But I'd like to add these tests to all affected APIs, and some of them require considerable setup to use (WebCrypto, for example). So it seems like it would be best to use the established testing infrastructure/harnesses for those tests. According to the WebKit LayoutTests page, the alternative to a virtual test suite is a flag-specific builder. Seems like overkill for this case, but perhaps there isn't a better option.

What do you all think?

Thanks,

-Ben

[Domenic Denicola]

From: blin...@chromium.org [mailto:blin...@chromium.org] On Behalf Of Ben Smith

> We may allow some methods to do this, but I'd like that list to be opt-in instead of opt-out.

This seems like an important question for the relevant standards, probably most pressingly Web IDL. Have you solicited opinions from the editors of those standards?

My initial reaction is that this seems strange and incorrect, as Web IDL already specifies that all passed-in typed arrays are copied into a new backing store before they are operated on by spec algorithms, so there is reason to disallow SharedArrayBuffer-backed typed arrays. (Note: sometimes the passed-in typed array is neutered before being used, which allows the implementation to make the unobservable optimization of omitting the spec's copy.)

The fact that SharedArrayBuffer was designed to work with the same type hierarchy of typed arrays, instead of a separate one, also indicates it would be better to leave them be. Otherwise you are inserting some implicit non-type-based overloading into every typed-array-accepting method.

[Jeffrey Yasskin]

I don't see the notion of a new backing store in https://heycam.github.io/webidl/#es-buffer-source-types. I see "get a copy of the bytes held by the buffer source" and "get a reference to the bytes held by the buffer source", and it might be good to indicate which to use, in the IDL of any given operation, since the former is safe on shared array buffers, while the second is likely to be unsafe. +1 to bringing this up with the Web IDL editors.

Jeffrey

[Ben Smith]

On Tuesday, December 15, 2015 at 3:28:36 PM UTC-8, Domenic Denicola wrote:

From: blin...@chromium.org [mailto:blin...@chromium.org] On Behalf Of Ben Smith

> We may allow some methods to do this, but I'd like that list to be opt-in instead of opt-out.

This seems like an important question for the relevant standards, probably most pressingly Web IDL. Have you solicited opinions from the editors of those standards?

Not yet, thanks for the suggestion.

 

My initial reaction is that this seems strange and incorrect, as Web IDL already specifies that all passed-in typed arrays are copied into a new backing store before they are operated on by spec algorithms, so there is reason to disallow SharedArrayBuffer-backed typed arrays. (Note: sometimes the passed-in typed array is neutered before being used, which allows the implementation to make the unobservable optimization of omitting the spec's copy.)

I assume you mean there is "no" reason to disallow? This may be specified (I found this, is that what you mean?), but taking a quick look at a few methods that take ArrayBuffers in Blink, they seem to be reading directly from the buffer. It makes some sense: they wouldn't need to copy data into a new backing store if they don't use the ArrayBuffer contents after control is returned to JavaScript, since the contents can't be modified. But as soon as you have a SharedArrayBuffer, this is an unsafe assumption to make since the contents could be modified by another thread.

 

The fact that SharedArrayBuffer was designed to work with the same type hierarchy of typed arrays, instead of a separate one, also indicates it would be better to leave them be. Otherwise you are inserting some implicit non-type-based overloading into every typed-array-accepting method.

Well, it was originally designed with a split hierarchy, but some folks pushed back against that so Lars modified the spec. He also modified Firefox to provide similar checks, see: https://github.com/lars-t-hansen/ecmascript_sharedmem/issues/38.

[Kenneth Russell]

On Tue, Dec 15, 2015 at 4:03 PM, Ben Smith <bi...@chromium.org> wrote:

On Tuesday, December 15, 2015 at 3:28:36 PM UTC-8, Domenic Denicola wrote:

From: blin...@chromium.org [mailto:blin...@chromium.org] On Behalf Of Ben Smith

> We may allow some methods to do this, but I'd like that list to be opt-in instead of opt-out.

This seems like an important question for the relevant standards, probably most pressingly Web IDL. Have you solicited opinions from the editors of those standards?

Not yet, thanks for the suggestion.

 

My initial reaction is that this seems strange and incorrect, as Web IDL already specifies that all passed-in typed arrays are copied into a new backing store before they are operated on by spec algorithms, so there is reason to disallow SharedArrayBuffer-backed typed arrays. (Note: sometimes the passed-in typed array is neutered before being used, which allows the implementation to make the unobservable optimization of omitting the spec's copy.)

I assume you mean there is "no" reason to disallow? This may be specified (I found this, is that what you mean?), but taking a quick look at a few methods that take ArrayBuffers in Blink, they seem to be reading directly from the buffer. It makes some sense: they wouldn't need to copy data into a new backing store if they don't use the ArrayBuffer contents after control is returned to JavaScript, since the contents can't be modified. But as soon as you have a SharedArrayBuffer, this is an unsafe assumption to make since the contents could be modified by another thread.

Yes, most APIs taking ArrayBuffer or ArrayBufferView read directly from the incoming buffer; this is deliberate, for the reasons you've pointed out. SharedArrayBuffer does present a new concurrency problem for these APIs.

Adding an implicit check to the binding generator doesn't sound like a desirable approach since:

 - SharedArrayBuffer support is still behind a flag

 - Presumably, we'll have to think through the implications of SABs for all of these APIs before it's turned on by default

 - It'll add undesired overhead to all of these API calls in the interim

Rather than adding an implicit check, I'd like to suggest a couple of other possibilities:

 - Adding an extended attribute to the binding generator which would add the new checks for the specific APIs where they are wanted (and make them no-ops if SharedArrayBuffers aren't actually turned on). (I realize this doesn't allow your opt-in approach, but again, this functionality is still behind a flag.)

 - Adding code to the C++ methods themselves which would do these checks and raise an exception if a SAB, or an ArrayBufferView backed by an SAB, is passed in (and make these checks no-ops if SharedArrayBuffers aren't turned on)

 - Adding code to the C++ methods themselves which would copy the data out of the SAB (which would become a no-op if SABs aren't turned on). This obviously wouldn't work for APIs that pass in an ArrayBuffer or ArrayBufferView to be filled.

-Ken

[Anne van Kesteren]

On Wed, Dec 16, 2015 at 12:58 AM, Jeffrey Yasskin <jyas...@chromium.org> wrote:
> I don't see the notion of a new backing store in
> https://heycam.github.io/webidl/#es-buffer-source-types. I see "get a copy
> of the bytes held by the buffer source" and "get a reference to the bytes
> held by the buffer source", and it might be good to indicate which to use,
> in the IDL of any given operation, since the former is safe on shared array
> buffers, while the second is likely to be unsafe. +1 to bringing this up
> with the Web IDL editors.

Note that there's is also
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28798 which somewhat
morphed into this request too. That we clearly distinguish in IDL
whether we copy bytes or reference them. And since views are already
somewhat special in that they can be detached, it doesn't seem that
weird to also have some syntactic form for whether you accept views
with a shared buffer.


--
https://annevankesteren.nl/

[Ben Smith]

On Tuesday, December 15, 2015 at 5:41:39 PM UTC-8, Kenneth Russell wrote:

On Tue, Dec 15, 2015 at 4:03 PM, Ben Smith <bi...@chromium.org> wrote:

On Tuesday, December 15, 2015 at 3:28:36 PM UTC-8, Domenic Denicola wrote:

From: blin...@chromium.org [mailto:blin...@chromium.org] On Behalf Of Ben Smith

> We may allow some methods to do this, but I'd like that list to be opt-in instead of opt-out.

This seems like an important question for the relevant standards, probably most pressingly Web IDL. Have you solicited opinions from the editors of those standards?

Not yet, thanks for the suggestion.

 

My initial reaction is that this seems strange and incorrect, as Web IDL already specifies that all passed-in typed arrays are copied into a new backing store before they are operated on by spec algorithms, so there is reason to disallow SharedArrayBuffer-backed typed arrays. (Note: sometimes the passed-in typed array is neutered before being used, which allows the implementation to make the unobservable optimization of omitting the spec's copy.)

I assume you mean there is "no" reason to disallow? This may be specified (I found this, is that what you mean?), but taking a quick look at a few methods that take ArrayBuffers in Blink, they seem to be reading directly from the buffer. It makes some sense: they wouldn't need to copy data into a new backing store if they don't use the ArrayBuffer contents after control is returned to JavaScript, since the contents can't be modified. But as soon as you have a SharedArrayBuffer, this is an unsafe assumption to make since the contents could be modified by another thread.

Yes, most APIs taking ArrayBuffer or ArrayBufferView read directly from the incoming buffer; this is deliberate, for the reasons you've pointed out. SharedArrayBuffer does present a new concurrency problem for these APIs.

Adding an implicit check to the binding generator doesn't sound like a desirable approach since:

 - SharedArrayBuffer support is still behind a flag

 

Right, but whatever method we use to prevent using SAB views will have to be added before the flag is default-enabled anyway.

 - Presumably, we'll have to think through the implications of SABs for all of these APIs before it's turned on by default

 - It'll add undesired overhead to all of these API calls in the interim

OK, so you're concerned about that APIs that will end up supporting SAB views (so they ultimately won't need the check) but will currently not be opted-in, so they'll have an additional check. Are there any good perf tests I can run to measure the effect of this additional check?

 

Rather than adding an implicit check, I'd like to suggest a couple of other possibilities:

 - Adding an extended attribute to the binding generator which would add the new checks for the specific APIs where they are wanted (and make them no-ops if SharedArrayBuffers aren't actually turned on). (I realize this doesn't allow your opt-in approach, but again, this functionality is still behind a flag.)

Well, SharedArrayBuffers are a runtime flag, so they can't be disabled in the generated binding code. Are you saying to check the SharedArrayBuffer flag rather than the isShared() flag on the ArrayBuffer object? The isShared() check should be nearly as cheap (it's just an enum comparison) though it does require some pointer indirection. If we found that it was slow, we could cache the isShared flag higher up -- it can't ever change after being set initially.

 

 - Adding code to the C++ methods themselves which would do these checks and raise an exception if a SAB, or an ArrayBufferView backed by an SAB, is passed in (and make these checks no-ops if SharedArrayBuffers aren't turned on)

 - Adding code to the C++ methods themselves which would copy the data out of the SAB (which would become a no-op if SABs aren't turned on). This obviously wouldn't work for APIs that pass in an ArrayBuffer or ArrayBufferView to be filled.

Why would handling it in the C++ methods be better? It seems like it would be more likely to be forgotten and would ultimately end up doing the same check.

[Ben Smith]

On Wed, Dec 16, 2015 at 2:38 PM, Ben Smith <bi...@chromium.org> wrote:

On Tuesday, December 15, 2015 at 5:41:39 PM UTC-8, Kenneth Russell wrote:

On Tue, Dec 15, 2015 at 4:03 PM, Ben Smith <bi...@chromium.org> wrote:

On Tuesday, December 15, 2015 at 3:28:36 PM UTC-8, Domenic Denicola wrote:

From: blin...@chromium.org [mailto:blin...@chromium.org] On Behalf Of Ben Smith

> We may allow some methods to do this, but I'd like that list to be opt-in instead of opt-out.

This seems like an important question for the relevant standards, probably most pressingly Web IDL. Have you solicited opinions from the editors of those standards?

Not yet, thanks for the suggestion.

FYI: I filed a bug here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=29388