Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Out-of-bounds TypedArrays should be rejected #40

Closed
anba opened this issue Jan 22, 2024 · 4 comments · Fixed by #43
Closed

Out-of-bounds TypedArrays should be rejected #40

anba opened this issue Jan 22, 2024 · 4 comments · Fixed by #43

Comments

@anba
Copy link
Contributor

anba commented Jan 22, 2024

The new functions should probably all call ValidateTypedArray(O, seq-cst) to ensure out-of-bounds TypedArrays are rejected. For example Uint8Array.fromBase64Into currently silently treats detached (and resizable out-of-bounds) TypedArrays as zero-length TypedArrays.

Also GetUint8ArrayBytes doesn't seem to work correctly when the input is a resizable TypedArray, because it directly reads [[ArrayLength]] instead of calling TypedArrayLength.

@bakkot
Copy link
Collaborator

bakkot commented Jan 22, 2024

The new functions should probably all call ValidateTypedArray(O, seq-cst) to ensure out-of-bounds TypedArrays are rejected.

Can you take a look at #36?

For example Uint8Array.fromBase64Into currently silently treats detached (and resizable out-of-bounds) TypedArrays as zero-length TypedArrays.

Accepting detached buffers is intentional; it matches TextEncoder's encodeInto:

x = new Uint8Array(10);
postMessage(null, '*', [x.buffer]); // detach x.buffer
console.log((new TextEncoder).encodeInto('xyz', x)); // read: 0, written: 0

I would usually prefer to match .set, but consistency seemed more important for this edge case.

@anba
Copy link
Contributor Author

anba commented Jan 22, 2024

For example Uint8Array.fromBase64Into currently silently treats detached (and resizable out-of-bounds) TypedArrays as zero-length TypedArrays.

Accepting detached buffers is intentional; it matches TextEncoder's encodeInto:

encodeInto doesn't seem to specify how detached and out-of-bounds resizable TypedArrays are handled:

  1. It uses its own byte length definition, which just reads from the [[ByteLength]] internal slot of the TypedArray. If the underlying ArrayBuffer is detached, [[ByteLength]] is larger than [[ArrayBufferByteLength]] (which is zero after detaching).
  2. Resizable TypedArrays which are out-of-bounds aren't yet handled at all. And there doesn't seem to an issue at https://github.com/whatwg/encoding/issues to support handling out-of-bounds resizable TypedArrays. AllowResizable in the WebIDL definition is required to accept resizable TypedArrays.

@bakkot
Copy link
Collaborator

bakkot commented Jan 22, 2024

Hm, yeah, it seems you're right that detached buffers are underspecified. I have whatwg/webidl#1385 to track.

But in all three major engines,

x = new Uint8Array(10);
postMessage(null, '*', [x.buffer]); // detach x.buffer
console.log((new TextEncoder).encodeInto('xyz', x));

prints "read: 0, written: 0", so it seems that web reality is to treat them as zero-length.

Resizable buffers are rejected by encodeInto, so it's true that in principle that when they are supported it might be done such that out-of-bounds buffers are rejected, but it would be kind of weird to treat them differently from detached buffers.

@anba
Copy link
Contributor Author

anba commented Jan 23, 2024

I don't know if it's necessary to accept detached buffers just to match TextEncoder, but simply because I don't know if the current TextEncoder behaviour of accepting detached buffers is required for web-compatibility. I don't have a strong opinion if it's better to match the existing TypedArray methods or to match TextEncoder and accept detached buffers. If detached buffers are accepted, we should clearly document it. For example I was prompted to write this issue report, because of this note in Uint8Array.from{Base64,Hex}Into:

NOTE: FromBase64 does not invoke any user code, so the ArrayBuffer backing into cannot have been detached or shrunk.

This was confusing to me, because it explicitly mentioned detached or shrunk buffers, but didn't check for that state before From{Base64,Hex}. Maybe we can add an additional note after the initial TypedArrayByteLength call to document that detached buffers are accepted:

Assert: If IsTypedArrayOutOfBounds(taRecord) is true, then byteLength is zero.
NOTE: This method accepts out-of-bounds TypedArrays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants