Discussion: Implications of shared memory for DOM/browser APIs

lars-t-hansen commented 9 years ago

In an aside in Issue #1, @jfbastien writes:

"How does SAB affect the web API surface area? Most APIs weren't designed to be called from multiple threads, and have led to exploits in the past e.g. (pwn2own 2015)[https://code.google.com/p/chromium/issues/detail?id=468936]."

It's probably open to discussion exactly what that question means, whether it pertains to confusing shared and unshared memory and thus exposing oneself to TOCTOTOU bugs, or if it pertains to thread-safety of DOM APIs in general.

For the former issue, Firefox has not really had to confront it because there has been a separate type hierarchy for shared views (SharedInt32Array, etc), thus there has been no space for confusion internally in the browser - everyone has had to opt in to shared memory, and that code can be reviewed carefully when that happens. There's some experimental code for WebGL that is about to land. Going forward, Firefox will implement the specification and get rid of the shared views. Internally in DOM, memory will be represented as a fat pointer that does not allow shared memory to be used accidentally in order to prevent this problem, but it's early days still for that.

For the latter issue, this seems like a problem for any API accessible from a worker as well as from the main thread, so is not really specific to SAB.

(More comments / thoughts are welcome.)

jfbastien commented 9 years ago

FWIW our plans will involve heavy use of tsan.

binji commented 8 years ago

I recently grepped through Blink's IDL files for ArrayBufferView (and BufferSource) to find WebAPIs that may be affected. Here's what I found:

FontFace (http://dev.w3.org/csswg/css-font-loading/#fontface-interface) ./Source/core/css/FontFace.idl:44: Constructor(DOMString family, (DOMString or ArrayBuffer or ArrayBufferView) source, optional FontFaceDescriptors descriptors),

FileAPI (https://w3c.github.io/FileAPI) ./Source/core/fileapi/Blob.idl:36: Constructor(sequence<(ArrayBuffer or ArrayBufferView or Blob or DOMString)> blobParts, optional BlobPropertyBag options), ./Source/core/fileapi/File.idl:31: Constructor(sequence<(Blob or DOMString or ArrayBufferView or ArrayBuffer)> fileBits, DOMString fileName, optional FilePropertyBag options),

WebUSB (http://reillyeon.github.io/webusb/#idl-def-usbintransferresult) ./Source/modules/webusb/USBDevice.idl:44: [CallWith=ScriptState] Promise<USBOutTransferResult> controlTransferOut(USBControlTransferParameters setup, optional BufferSource data); ./Source/modules/webusb/USBDevice.idl:47: [CallWith=ScriptState] Promise<USBOutTransferResult> transferOut(octet endpointNumber, BufferSource data);

MediaSource (https://w3c.github.io/media-source/#sourcebuffer) ./Source/modules/mediasource/SourceBuffer.idl:64: [RaisesException] void appendBuffer(ArrayBufferView data);

WebSocket (https://html.spec.whatwg.org/multipage/comms.html#the-websocket-interface) ./Source/modules/websockets/WebSocket.idl:69: [RaisesException] void send(ArrayBufferView data);

WebCrypto (http://www.w3.org/TR/WebCryptoAPI) ./Source/modules/crypto/Crypto.idl:33: [RaisesException, MeasureAs=CryptoGetRandomValues] ArrayBufferView getRandomValues(ArrayBufferView array); ./Source/modules/crypto/SubtleCrypto.idl:47: [CallWith=ScriptState, MeasureAs=SubtleCryptoImportKey] Promise importKey(KeyFormat format, (ArrayBuffer or ArrayBufferView or Dictionary) keyData, AlgorithmIdentifier algorithm, boolean extractable, sequence<KeyUsage> keyUsages); ./Source/modules/crypto/SubtleCrypto.idl:41: [CallWith=ScriptState, MeasureAs=SubtleCryptoEncrypt] Promise encrypt(AlgorithmIdentifier algorithm, CryptoKey key, BufferSource data); ./Source/modules/crypto/SubtleCrypto.idl:42: [CallWith=ScriptState, MeasureAs=SubtleCryptoDecrypt] Promise decrypt(AlgorithmIdentifier algorithm, CryptoKey key, BufferSource data); ./Source/modules/crypto/SubtleCrypto.idl:43: [CallWith=ScriptState, MeasureAs=SubtleCryptoSign] Promise sign(AlgorithmIdentifier algorithm, CryptoKey key, BufferSource data); ./Source/modules/crypto/SubtleCrypto.idl:44: [CallWith=ScriptState, ImplementedAs=verifySignature, MeasureAs=SubtleCryptoVerify] Promise verify(AlgorithmIdentifier algorithm, CryptoKey key, BufferSource signature, BufferSource data); ./Source/modules/crypto/SubtleCrypto.idl:45: [CallWith=ScriptState, MeasureAs=SubtleCryptoDigest] Promise digest(AlgorithmIdentifier algorithm, BufferSource data); ./Source/modules/crypto/SubtleCrypto.idl:52: [CallWith=ScriptState, MeasureAs=SubtleCryptoUnwrapKey] Promise unwrapKey(KeyFormat format, BufferSource wrappedKey, CryptoKey unwrappingKey, AlgorithmIdentifier unwrapAlgorithm, AlgorithmIdentifier unwrappedKeyAlgorithm, boolean extractable, sequence<KeyUsage> keyUsages);

WebGL (http://www.khronos.org/registry/webgl/specs/latest/1.0/) void bufferData(GLenum target, ArrayBufferView data, GLenum usage); void bufferSubData(GLenum target, GLintptr offset, [FlexibleArrayBufferView] ArrayBufferView data); GLsizei width, GLsizei height, GLint border, ArrayBufferView? data); GLsizei width, GLsizei height, GLenum format, ArrayBufferView? data); void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, ArrayBufferView? pixels); GLenum format, GLenum type, ArrayBufferView? pixels); GLenum format, GLenum type, ArrayBufferView? pixels); void uniform1fv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Float32Array v); void uniform1iv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Int32Array v); void uniform2fv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Float32Array v); void uniform2iv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Int32Array v); void uniform3fv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Float32Array v); void uniform3iv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Int32Array v); void uniform4fv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Float32Array v); void uniform4iv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Int32Array v); void texImage3D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLsizei depth, GLint border, GLenum format, GLenum type, ArrayBufferView? pixels); void texSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, GLenum format, GLenum type, ArrayBufferView? pixels); void compressedTexImage3D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLsizei depth, GLint border, ArrayBufferView data); void compressedTexSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, GLenum format, ArrayBufferView data); void uniform1uiv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Uint32Array v); void uniform2uiv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Uint32Array v); void uniform3uiv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Uint32Array v); void uniform4uiv(WebGLUniformLocation? location, [FlexibleArrayBufferView] Uint32Array v);

WebAudio (http://www.w3.org/TR/webaudio/) ./Source/modules/webaudio/AudioContext.idl:60: [RaisesException, MeasureAs=AudioContextDecodeAudioData] void decodeAudioData(ArrayBuffer audioData, AudioBufferCallback successCallback, optional AudioBufferCallback errorCallback);

WebBluetooth (https://webbluetoothchrome.github.io/web-bluetooth) ./Source/modules/bluetooth/BluetoothGATTCharacteristic.idl:23: [CallWith=ScriptState] Promise<void> writeValue(BufferSource value);

PushMessaging (http://w3c.github.io/push-api) ./Source/modules/push_messaging/PushEventInit.idl:8:typedef (ArrayBuffer or ArrayBufferView or USVString) PushMessageDataInit;

WebNFC (https://w3c.github.io/web-nfc/#the-nfc-interface) ./Source/modules/nfc/NFC.idl:7:typedef (DOMString or ArrayBuffer or NFCMessage) NFCPushMessage;

EncryptedMedia (https://w3c.github.io/encrypted-media/#mediaencryptedevent) ./Source/modules/encryptedmedia/MediaKeys.idl:38: [CallWith=ScriptState] Promise<void> setServerCertificate(BufferSource serverCertificate); ./Source/modules/encryptedmedia/MediaKeySession.idl:38: [CallWith=ScriptState] Promise<void> generateRequest(DOMString initDataType, BufferSource initData); ./Source/modules/encryptedmedia/MediaKeySession.idl:42: [CallWith=ScriptState] Promise<void> update(BufferSource response); ./Source/modules/encryptedmedia/MediaKeyStatusMap.idl:21: readonly maplike <BufferSource, MediaKeyStatus>;

Beacon (http://w3c.github.io/beacon/) ./Source/modules/beacon/NavigatorBeacon.idl:8: [CallWith=ExecutionContext, MeasureAs=SendBeacon, RaisesException] boolean sendBeacon(DOMString url, optional (ArrayBufferView or Blob or DOMString or FormData)? data = null);

WebRTC (http://w3c.github.io/webrtc-pc/) ./Source/modules/mediastream/RTCDataChannel.idl:52: [RaisesException] void send(ArrayBufferView data);

Presentation API (https://w3c.github.io/presentation-api) ./Source/modules/presentation/PresentationConnection.idl:29: [RaisesException, MeasureAs=PresentationConnectionSend, LegacyInterfaceTypeChecking] void send(ArrayBufferView data);

Fetch (http://fetch.spec.whatwg.org) ./Source/modules/fetch/Response.idl:10:typedef (Blob or ArrayBuffer or ArrayBufferView or FormData or USVString) BodyInit;

Encoding (https://encoding.spec.whatwg.org/#interface-textdecoder) ./Source/modules/encoding/TextDecoder.idl:41: [RaisesException, MeasureAs=TextDecoderDecode] DOMString decode(optional BufferSource input, optional TextDecodeOptions options);

XmlHttpRequest (https://xhr.spec.whatwg.org/#interface-xmlhttprequest) ./Source/core/xmlhttprequest/XMLHttpRequest.idl:69: [RaisesException] void send(optional (ArrayBuffer or ArrayBufferView or Blob or Document or DOMString or FormData)? body = null);

lars-t-hansen commented 8 years ago

That mirrors my experience in Firefox, more or less. I just landed the changes to Firefox to get rid of the SharedTypedArrays and had to deal with all the DOM and browser APIs that use TypedArray to make sure they did not accidentally opt in to using shared memory. (https://bugzil.la/1176214) As I wrote earlier, we handled this on the high level with a fat pointer that explicitly represents sharedness, and on the lower level by requiring that pointer to be unwrapped specially if the memory is shared. (At this time, only a small number of WebGL/Canvas APIs opt in to shared memory in Firefox.)

binji commented 8 years ago

I started a discussion on blink-dev about this: https://groups.google.com/a/chromium.org/d/topic/blink-dev/EsX3S43nm-0/discussion

They suggested we talk to the WebIDL editors about a mechanism for allowing/preventing SAB views from being used.

The comment here suggests that in addition to annotating which methods copy/reference ArrayBuffer contents, there could be annotation specifying which methods accept SAB views: https://groups.google.com/a/chromium.org/d/msg/blink-dev/EsX3S43nm-0/AAgH8_JWBAAJ.

lars-t-hansen commented 8 years ago

Thanks for starting that. I've received similar advice privately, that it would be best if the WebIDL APIs could opt-in to shared memory (https://bugzil.la/1231687). Mostly this was about improving error messages; for safety reasons we'd want to keep the fat pointers internally in the Firefox C++ code in any case.

lars-t-hansen commented 8 years ago

@binji filed this issue: https://www.w3.org/Bugs/Public/show_bug.cgi?id=29388

erights commented 8 years ago

@lars-t-hansen I don't understand how any of this protects the JavaScript programmer in general. The FF fat pointers are at the C++ level and the w3 bug is webidl specific, and therefore web specific. Aside from the narrow scope of these solutions, they seem like the right kind of thing. How do we generally prevent the kind of confusion that these prevent in those specific cases?

lars-t-hansen commented 8 years ago

@erights, not sure what you are thinking about when you ask about protecting the JS programmer. This particular bug probably pertains only to how we integrate the shared memory work into a specific embedding (the browser) in a safe way, ie, what we talk about here probably will not end up in ES, but will perhaps inform the discussion within TC39.

It's very desirable to opt-in to shared memory in any host API, of course.

I suppose there could be a broader discussion whether we should do anything generic within ES to distinguish between shared and non-shared memory for host functions. Host functions can extract the buffer from any TypedArray argument and check whether it is shared memory (this is what the webidl fix amounts to). Do we need anything more than that? Can we?

Or were you thinking of something else?

lars-t-hansen commented 7 years ago

There's what probably amounts to a consensus position for the WebIDL syntax here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=29388#c2

The DOM companion spec (https://tc39.github.io/ecmascript_sharedmem/dom_shmem.html) should probably be updated to this syntax. Before I do that I am going to be investigating its utility and performance in Firefox, https://bugzil.la/1231687.

binji commented 7 years ago

Thanks for reviving this, Lars. I have a similar CL from a while back where I was exploring the same thing in Blink: https://codereview.chromium.org/1526183004/.

lars-t-hansen commented 7 years ago

WebGL 2.0 bindings are also becoming an issue, but after some disussion with @juj I don't think there's anything difficult going on here, just work. Below are the APIs he's requested be included. I think what we're proposing for WebIDL (see my comment above) will cover these reasonably well.

Some of these also seem to be present in 1.0.2 and are listed in @binji's comment above.

void bufferData(GLenum target, ArrayBufferView srcData, GLenum usage, GLuint srcOffset, optional GLuint length = 0); void bufferSubData(GLenum target, GLintptr dstByteOffset, ArrayBufferView srcData, GLuint srcOffset, optional GLuint length = 0); void getBufferSubData(GLenum target, GLintptr srcByteOffset, ArrayBufferView dstData, optional GLuint dstOffset = 0, optional GLuint length = 0); void texImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, ArrayBufferView srcData, GLuint srcOffset); void texImage3D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLsizei depth, GLint border, GLenum format, GLenum type, ArrayBufferView? srcData); void texImage3D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLsizei depth, GLint border, GLenum format, GLenum type, ArrayBufferView srcData, GLuint srcOffset); void texSubImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLsizei width, GLsizei height, GLenum format, GLenum type, ArrayBufferView srcData, GLuint srcOffset); void texSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, GLenum format, GLenum type, ArrayBufferView? srcData, optional GLuint srcOffset = 0); void compressedTexImage2D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLint border, ArrayBufferView srcData, optional GLuint srcOffset = 0, optional GLuint srcLengthOverride = 0); void compressedTexImage3D(GLenum target, GLint level, GLenum internalformat, GLsizei width, GLsizei height, GLsizei depth, GLint border, ArrayBufferView srcData, optional GLuint srcOffset = 0, optional GLuint srcLengthOverride = 0); void compressedTexSubImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLsizei width, GLsizei height, GLenum format, ArrayBufferView srcData, optional GLuint srcOffset = 0, optional GLuint srcLengthOverride = 0); void compressedTexSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset, GLint zoffset, GLsizei width, GLsizei height, GLsizei depth, GLenum format, ArrayBufferView srcData, optional GLuint srcOffset = 0, optional GLuint srcLengthOverride = 0); void uniform{1,2,3,4}fv(WebGLUniformLocation? location, Float32List data, optional GLuint srcOffset = 0, optional GLuint srcLength = 0); void uniform{1,2,3,4}iv(WebGLUniformLocation? location, Int32List data, optional GLuint srcOffset = 0, optional GLuint srcLength = 0); void uniform{1,2,3,4}uiv(WebGLUniformLocation? location, Uint32List data, optional GLuint srcOffset = 0, optional GLuint srcLength = 0); void uniformMatrix{2,3x2,4x2,2x3,3,4x3,2x4,3x4,4}fv(WebGLUniformLocation? location, GLboolean transpose, Float32List data, optional GLuint srcOffset = 0, optional GLuint srcLength = 0); void readPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, ArrayBufferView dstData, GLuint dstOffset); void clearBufferfv(GLenum buffer, GLint drawbuffer, Float32List values, optional GLuint srcOffset = 0); void clearBufferiv(GLenum buffer, GLint drawbuffer, Int32List values, optional GLuint srcOffset = 0); void clearBufferuiv(GLenum buffer, GLint drawbuffer, Uint32List values, optional GLuint srcOffset = 0);

lars-t-hansen commented 7 years ago

@domenic, @binji -- some discussion with Boris Zbarsky (WebIDL editor) in https://bugzil.la/1231687, re what the appropriate solution is in WebIDL. If you have opinions this is a good time to voice them.

binji commented 7 years ago

So it sounds like his suggestion is to duplicate all ArrayBuffer, TypedArray, etc. types differentiating them by whether the callee will copy or reference. Seems a bit strange to me that the type is specifying what the function will do with the value, rather than what the value is, but perhaps that is common elsewhere in WebIDL?

Also: I'm a little concerned that we'll have a mismatch between the actual types (e.g. Int8Array w/ backing SharedArrayBuffer) vs the WebIDL types (e.g. MaybeSharedInt8Array). In some ways I suppose it could be viewed as a union type, but it also has additional semantics.

Either way, I don't think it will be a problem for Blink's IDL generator.

domenic commented 7 years ago

Seems a bit strange to me that the type is specifying what the function will do with the value, rather than what the value is, but perhaps that is common elsewhere in WebIDL?

Nah, that's not the case in Web IDL. There types serve several purposes, one of the most important of which is saying what happens when they are passed as arguments. Cf. USVString or ByteString.

Also: I'm a little concerned that we'll have a mismatch between the actual types

Yes, I am also a bit concerned there; see my paragraph at the bottom of https://bugzilla.mozilla.org/show_bug.cgi?id=1231687#c16

annevk commented 7 years ago

@domenic maybe you should update this issue with the revised plans for [AllowShared]? I guess the plan still is to only use that extended attribute in WebGL?

annevk commented 7 years ago

Maybe this is just a duplicate of #168 at this point.

tc39 / proposal-ecmascript-sharedmem

Discussion: Implications of shared memory for DOM/browser APIs #38