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

assert: deepEqual/deepStrictEqual throws on equivalent typed arrays #8001

Closed
feross opened this issue Aug 6, 2016 · 1 comment
Closed

assert: deepEqual/deepStrictEqual throws on equivalent typed arrays #8001

feross opened this issue Aug 6, 2016 · 1 comment
Labels
assert Issues and PRs related to the assert subsystem.

Comments

@feross
Copy link
Contributor

feross commented Aug 6, 2016

  • Version: 6.3.1
  • Platform: macOS 10.11.6
  • Subsystem: assert

The following code throws an AssertionError, when it should not.

var a = new Uint8Array([1, 2, 3, 4]).subarray(1) // slice of parent buffer
var b = new Uint8Array([2, 3, 4])

console.log(a) // Uint8Array [ 2, 3, 4 ]
console.log(b) // Uint8Array [ 2, 3, 4 ]

assert.deepEqual(a, b) // NOT OK -- throws!
assert.deepStrictEqual(a, b) // NOT OK -- throws!

The error thrown is AssertionError: Uint8Array [ 2, 3, 4 ] deepEqual Uint8Array [ 2, 3, 4 ].

This bug is caused by the optimization introduced in step 7.4 of the _deepEqual algorithm where typed arrays are converted to Buffers for performance reasons. The error is that the typed array's underlying ArrayBuffer is used without respecting it's .byteOffset or .byteLength (i.e. position within the parent ArrayBuffer).

When Buffer is used instead of typed arrays, both assert.deepEqual and assert.deepStrictEqual work as expected without throwing an exception.

var c = Buffer.from([1, 2, 3, 4]).slice(1) // slice of parent buffer
var d = Buffer.from([2, 3, 4])

console.log(c) // <Buffer 02 03 04>
console.log(d) // <Buffer 02 03 04>

assert.deepEqual(c, d) // OK -- no exception
assert.deepStrictEqual(c, d) // OK -- no exception

cc @trevnorris @bnoordhuis @jasnell @addaleax


Step 7.1 of the _deepEqual algorithm has a special case for objects that are instanceof Buffer so this issue hasn't manifested for the Buffer tests in core, even though they're also typed arrays. However, this bug causes issues in the browser buffer test suite, because our Buffer class is not considered instanceof Buffer by the assert package, since it's not a Node Buffer.

@feross
Copy link
Contributor Author

feross commented Aug 6, 2016

Here's an attempt at a PR to fix the issue: #8002

@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Aug 7, 2016
feross added a commit to feross/buffer that referenced this issue Aug 8, 2016
@jasnell jasnell closed this as completed in 387ab62 Aug 9, 2016
cjihrig pushed a commit that referenced this issue Aug 10, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this issue Nov 23, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: nodejs#8001
PR-URL: nodejs#8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 13, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
The typed array's underlying ArrayBuffer is used in `Buffer.from`.
Let's respect it's .byteOffset or .byteLength (i.e. position within the
parent ArrayBuffer).

Fixes: #8001
PR-URL: #8002
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
itu2n1i8w added a commit to itu2n1i8w/buffer that referenced this issue Aug 5, 2024
honsor18 added a commit to honsor18/buffer that referenced this issue Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants