Idea for [AllowShared] (handling SharedArrayBuffer safely)

[annevk]

We'll likely soon have a model for enabling SharedArrayBuffer in all browsers again (see whatwg/html#4175 for details). The way this works is somewhat opt-in via an internal slot on agent clusters (e.g., [[AllowHighResolutionTimers]] or some such). SharedArrayBuffer would always be present, but messaging it to a dedicated worker would not work unless that flag is set.

Since it's always available there might be APIs that can take a SharedArrayBuffer and do something "safe" with them (i.e., something that does not create a timer). However, given the potentially unsafe nature it would be nice to be able to easily find and vet those.

One problem is that currently you can bypass [AllowShared] with any and object. Perhaps we should revisit that?

Then, bypassing [[AllowHighResolutionTimers]] on the IDL layer should perhaps require a name with clearer intent, e.g., [UnsafeAllowShared].

So I was thinking, perhaps we keep [AllowShared], but also require it for any and object, and make it branch on the encompassing agent's agent cluster's [[AllowHighResolutionTimers]]. And then [UnsafeAllowShared] would not.

cc @arturjanc @cdumez @csreis @linclark @lukewagner @mystor @rniwa @domenic @bzbarsky

[annevk]

(One observable aspect of this is that passing a SharedArrayBuffer to postMessage() would throw a TypeError rather than a "DataCloneError" DOMException as IDL would catch [[AllowHighResolutionTimers]] being false. That seems positive as all APIs would get equivalent error handling for SharedArrayBuffer. A negative is the additional branching required for any/object, though implementations could create fast paths here and there as needed...)

[bzbarsky]

though implementations could create fast paths here and there as needed

How, exactly? If the semantics are that this has to happen at argument conversion time, then it needs to happen at argument conversion time, right?

Past that, I'm not quite clear on the problem we are trying to solve. SAB can create a timer if we can hand it to another execution thread, right? An API that takes "object" or "any" would have to explicitly check for it being an SAB to do that (because other objects are not safe to hand over), so the safety check seems like it should be at that check point...

[annevk]

I was thinking that if it happens as the last step of conversion it could perhaps also be done elsewhere.

I'm interested in these issues:

• Making it easier to audit places where SharedArrayBuffer is used. ([UnsafeAllowShared] is easier to search for and jumps out more at reviewers of new APIs. The status quo is that any/object/[AllowShared] are potentially problematic.)
• Provide a way for using SharedArrayBuffer that is known to be "safe" (i.e., checks the [[AllowHighResolutionTimers]] internal slot).

(We also need to do this audit, but that could be a separate issue as well.)

[bzbarsky]

I was thinking that if it happens as the last step of conversion

For that one argument, or for all arguments? That is, is it a step added to https://heycam.github.io/webidl/#es-to-object or is it a step added to https://heycam.github.io/webidl/#es-operations after step 2.1.5?

In the former case, it can only be optimized out if the thing you're dealing with is the "last argument" or something.

In the latter case, implementing it is pretty complicated (think sequence<SomeDictionaryType> where the dictionary contains a member of type record<USVString, sequence<object>> or something).

• Making it easier to audit places where SharedArrayBuffer is used.

OK. Doing this on the spec level is a bit of a pain, I agree. Doing it on the code level in browsers is not too bad. For Firefox, say, you'd look at https://searchfox.org/mozilla-central/search?q=symbol:_ZNK7mozilla3dom15TypedArray_base15DataAllowSharedEv&redirect=false and https://searchfox.org/mozilla-central/search?q=IsSharedArrayBuffer&case=true and probably https://searchfox.org/mozilla-central/search?q=symbol:_Z26JS_GetTypedArraySharednessP8JSObject&redirect=false

[littledan]

So I was thinking, perhaps we keep [AllowShared], but also require it for any and object, and make it branch on the encompassing agent's agent cluster's [[AllowHighResolutionTimers]]. And then [UnsafeAllowShared] would not.

I'm a little nervous about this idea. In my limited experience writing specifications with WebIDL, I've already felt the need to use any or object as an escape hatch, to get around the conversions and checks that would otherwise take place.

The JS standard library in particular has many places where values are simply being passed around, and not interpreted. It seems excessively conservative to ask authors to place an extended attribute for such an "uninterpreted value" case.

I wonder if there's some other spec convention we could use, where we're somehow checking for safety at the point of use, rather than the point of taking something in as a parameter.

[lukewagner]

Maybe silly question but: while obviously any and object can receive SABs and views on SABs, the race hazard is only present if the method then proceeds to use that any/object value as a view to racily read the SAB's contents. E.g., if the value is just stored to hand back to content as a callback argument, that'd be fine. But are methods that are using their arguments as views really using any/object instead of an explicit BufferSource (possibly as one disjunct of an or)?

Note also that for the any used by postMessage(), it's not enough to just look at the any value, but all the transitively-reached objects during structured serialization, so this is not a case that Web IDL could help. I wonder if perhaps all the interesting other cases of using any similarly require ad hoc checks in the method spec.

[It looks like Dan and I just raced to make a similar point]

[annevk]

Yeah, you're right. We cannot meaningfully "protect" any/object. So what remains:

1. Audit of existing any/object/[AllowShared] entry points to see if they do anything problematic.
2. Do we want to split [AllowShared] into a "safe" and "unsafe" variant?
   1. If not, assuming we keep the existing processing model, do we want to rename it to indicate the danger more clearly?

[annevk]

I'm closing this as this is a little too vague to act on and the original idea cannot be done in a performant way.