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

"byte length of a buffer source type" needs updating for resizable and detached buffers #1385

Open
bakkot opened this issue Jan 21, 2024 · 9 comments

Comments

@bakkot
Copy link
Contributor

bakkot commented Jan 21, 2024

What is the issue with the Web IDL Standard?

As of this proposal / PR, the [[ByteLength]] slot on TypedArrays and DataViews can be ~auto~ instead of an integer. Consumers of the "byte length of a buffer source" operation (like encodeInto) probably want TypedArrayByteLength or GetViewByteLength instead of the raw value of the [[ByteLength]] slot.

Also, when getting the length of an ArrayBuffer, this should probably use the ArrayBufferByteLength abstract operation, not raw access of the slot.

There's some other raw uses of the [[ByteLength]] slot, like in "write a byte sequence bytes into an ArrayBufferView", which should probably also be updated.

cc @syg

@annevk
Copy link
Member

annevk commented Jan 22, 2024

encodeInto() doesn't currently accept resizable buffers though, right? It does make sense to update these algorithms for when we get methods that want to accept them though.

@bakkot
Copy link
Contributor Author

bakkot commented Jan 22, 2024

Huh, yeah, I didn't realize that "Uint8Array" meant specifically a Uint8Array backed by a non-resizable buffer by default, so this is not actually an issue at the moment. I'll close this but feel free to reopen if you want to use this to track.

@bakkot bakkot closed this as completed Jan 22, 2024
@bakkot bakkot reopened this Jan 22, 2024
@bakkot
Copy link
Contributor Author

bakkot commented Jan 22, 2024

Hm, actually, this is already an issue because of detached buffers. Reading the [[ByteLength]] slot of a (> 0 size) TypedArray which is backed by a detached ArrayBuffer will give you a positive integer, which means that the "byte length" will be a positive integer, which breaks at least encodeInto.

Consider writing the first byte into a Uint8Array backed by a detached buffer.

  • "destination’s byte length − written is greater than or equal to the number of bytes in result" is true
  • So we "Write the bytes in result into destination"
  • which will "Write bytes into arrayBuffer with startingOffset set to jsView.[[ByteOffset]] + startingOffset."
  • which asserts that "bytes’s length ≤ jsArrayBuffer.[[ArrayBufferByteLength]] − startingOffset."
  • which assertion is violated: the [[ArrayBufferByteLength]] if a detached buffer is zero.

@bakkot bakkot changed the title "byte length of a buffer source type" needs updating for resizable buffers "byte length of a buffer source type" needs updating for resizable and detached buffers Jan 22, 2024
@annevk
Copy link
Member

annevk commented Jan 22, 2024

If destination's byte length - written is false we'd break and return, no? Anyway, keeping this open to tidy all these things up seems worth it.

@bakkot
Copy link
Contributor Author

bakkot commented Jan 22, 2024

Sorry, that should read "true". On the first byte, written is 0, destination's byte length the original size of the array, and the number of bytes in result is 1 between and 4 (I assume; anyway it's some finite number). So as long as the TA originally had at least 4 bytes, that check passes and we don't break.

@syg
Copy link
Contributor

syg commented Jan 23, 2024

Good catch. These should be checked for using https://tc39.es/ecma262/#sec-istypedarrayoutofbounds for simpler reasoning at the boundary, even if resizable buffers are currently disallowed.

@bakkot
Copy link
Contributor Author

bakkot commented Jan 23, 2024

Is the intention to treat detached buffers as zero-length? That's kind of weird. I've copied that behavior into .fromBase64Into, but maybe it would be better to throw, like TA.prototype.set.

@annevk
Copy link
Member

annevk commented Jan 24, 2024

I think throwing a TypeError would be better. Not sure if encodeInto() can still be changed, but I'd be willing to try. It seems somewhat unlikely folks rely on it not throwing, but who knows.

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

No branches or pull requests

3 participants