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

bad Buffer conversion behaviour due to Uint8Array inheritance #28725

Closed
Fishrock123 opened this issue Jul 17, 2019 · 3 comments · Fixed by #48274
Closed

bad Buffer conversion behaviour due to Uint8Array inheritance #28725

Fishrock123 opened this issue Jul 17, 2019 · 3 comments · Fixed by #48274
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 17, 2019

tl;dr - TypedArrays of lower-order truncate high-order values, which is is confusing (and hard to work around).

Ok so Buffer.from(new Uint32Array([0x4701c993])) returns a buffer with a length of 1 and a value in index 0 of 0x93.

This is a bad conversion and seems obviously wrong to me. An explicit 32-bit type was provided, and not just [0x4701c993] (for which the docs says interpreted as octets).

I don't possibly see how this could be the desired behavior, the resulting values are nothing like the input at all. But, here's the spec: https://www.ecma-international.org/ecma-262/6.0/#sec-touint8

Also, the Buffer#write<U><Type><Size><endian>() APIs help little, since they are awkwardly designed when used for such a purpose. (They return bytes written and not the buffer.) Presumably, if I had a whole array of int32s I'd have to write some kind of custom converter, which seems wrong considering we are converting to a runtime standard object from a primitive type.

Suggestion: fix this somehow, or, provide a nicer API to do a reasonable conversion.

@Fishrock123 Fishrock123 added the buffer Issues and PRs related to the buffer subsystem. label Jul 17, 2019
@devsnek
Copy link
Member

devsnek commented Jul 17, 2019

Buffer.from(new Uint32Array([0x4701c993]).buffer) 😄

@gfx
Copy link
Contributor

gfx commented Jul 19, 2019

How about introducing a new API like Buffer.fromBufferView(bufferView: ArrayBufferView) to a shorthand of Buffer.from(bufferView.buffer, bufferView.byteOffset, bufferView.byteLength)?

I saw a lot of Buffer.from(uint8array) for Uint8Array-to-Buffer conversion, but because it copies the underlying buffer it is not the best solution for the most cases.

(ArrayBufferView here is an object that ArrayBuffer.isView() returns true).

@Fishrock123
Copy link
Contributor Author

Also, maybe we could emit a warning for this? That's platform independent and if someone does Buffer.from(new Uint32Array(...)) they almost certainly have a bug.

gfx added a commit to gfx/node that referenced this issue Aug 13, 2019
It creates a new Buffer that shares ArrayBuffer with another
ArrayBufferView. A shortcut for `Buffer.from(view.buffer,
view.byteOffset, view.byteLength)` but it requires no temporary var.

refs: nodejs#28725
bengl added a commit to bengl/node that referenced this issue May 31, 2023
The code for Buffer.from() treats non-Buffer and non-Uint8Array
Array-likes as Arrays. This creates some confusion when passing various
TypedArrays to Buffer.from(). The documentation now reflects the actual
behavior.

Fixes: nodejs#28725
nodejs-github-bot pushed a commit that referenced this issue Jun 2, 2023
The code for Buffer.from() treats non-Buffer and non-Uint8Array
Array-likes as Arrays. This creates some confusion when passing various
TypedArrays to Buffer.from(). The documentation now reflects the actual
behavior.

Fixes: #28725
PR-URL: #48274
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
targos pushed a commit that referenced this issue Jun 4, 2023
The code for Buffer.from() treats non-Buffer and non-Uint8Array
Array-likes as Arrays. This creates some confusion when passing various
TypedArrays to Buffer.from(). The documentation now reflects the actual
behavior.

Fixes: #28725
PR-URL: #48274
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
The code for Buffer.from() treats non-Buffer and non-Uint8Array
Array-likes as Arrays. This creates some confusion when passing various
TypedArrays to Buffer.from(). The documentation now reflects the actual
behavior.

Fixes: #28725
PR-URL: #48274
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
The code for Buffer.from() treats non-Buffer and non-Uint8Array
Array-likes as Arrays. This creates some confusion when passing various
TypedArrays to Buffer.from(). The documentation now reflects the actual
behavior.

Fixes: nodejs#28725
PR-URL: nodejs#48274
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
The code for Buffer.from() treats non-Buffer and non-Uint8Array
Array-likes as Arrays. This creates some confusion when passing various
TypedArrays to Buffer.from(). The documentation now reflects the actual
behavior.

Fixes: nodejs#28725
PR-URL: nodejs#48274
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
The code for Buffer.from() treats non-Buffer and non-Uint8Array
Array-likes as Arrays. This creates some confusion when passing various
TypedArrays to Buffer.from(). The documentation now reflects the actual
behavior.

Fixes: nodejs#28725
PR-URL: nodejs#48274
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants