fix: preserve byteLength when cloning a DataView
copyBuffer routed DataView through the TypedArray path, passing cur.length (undefined on a DataView) as the constructor's third argument. Since that argument is a byte length for DataView, a partial view was extended to the end of the buffer. Clone DataView explicitly using byteLength.
This commit is contained in:
1 parent
7fe17ef74e
commit
655cc5eca2
3 files changed
+110
No files matched your search
@@ -0,0 +1,77 @@
|
|||||||
|
# Fix DataView clones getting the wrong byteLength
|
||||||
|
|
||||||
|
## Problem
|
||||||
|
|
||||||
|
Cloning a `DataView` that covers only part of its underlying `ArrayBuffer`
|
||||||
|
produces a clone with the wrong `byteLength`: the copy spans from the original
|
||||||
|
`byteOffset` to the end of the buffer instead of keeping the original length.
|
||||||
|
|
||||||
|
```js
|
||||||
|
const clone = require('rfdc')()
|
||||||
|
|
||||||
|
const buffer = new ArrayBuffer(16)
|
||||||
|
const view = new DataView(buffer, 4, 8) // offset 4, length 8
|
||||||
|
|
||||||
|
const copy = clone({ a: view }).a
|
||||||
|
copy.byteLength // 12 (expected 8)
|
||||||
|
```
|
||||||
|
|
||||||
|
The value at the start of the view is copied correctly and the clone is
|
||||||
|
independent of the source, but the clone can now read 4 bytes past the end of
|
||||||
|
the original view.
|
||||||
|
|
||||||
|
## Root cause
|
||||||
|
|
||||||
|
`index.js`, in `copyBuffer`:
|
||||||
|
|
||||||
|
```js
|
||||||
|
return new cur.constructor(cur.buffer.slice(), cur.byteOffset, cur.length)
|
||||||
|
```
|
||||||
|
|
||||||
|
This path handles every `ArrayBuffer.isView` value, i.e. both TypedArrays and
|
||||||
|
`DataView`. It is correct for TypedArrays, whose third constructor argument is an
|
||||||
|
element count matching `.length`. A `DataView` is different:
|
||||||
|
|
||||||
|
- it has no `.length` property, so `cur.length` is `undefined`;
|
||||||
|
- its third constructor argument is a byte length, not an element count.
|
||||||
|
|
||||||
|
`new DataView(buffer, byteOffset, undefined)` extends the view to the end of the
|
||||||
|
buffer, so a partial view's length is lost. (A DataView over the whole buffer
|
||||||
|
happens to work, because "to the end of the buffer" equals its byteLength.)
|
||||||
|
|
||||||
|
## Fix
|
||||||
|
|
||||||
|
Handle `DataView` explicitly and size it with `byteLength`:
|
||||||
|
|
||||||
|
```js
|
||||||
|
if (cur instanceof DataView) {
|
||||||
|
return new DataView(cur.buffer.slice(), cur.byteOffset, cur.byteLength)
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
TypedArrays keep using the existing `.length`-based path.
|
||||||
|
|
||||||
|
## Test
|
||||||
|
|
||||||
|
Added tests (run for all four `rfdc` variants via the shared `types()` helper)
|
||||||
|
that clone a full-buffer DataView and a partial DataView, asserting `byteOffset`,
|
||||||
|
`byteLength`, the stored value, and independence from the source.
|
||||||
|
|
||||||
|
Counterfactual (revert the `copyBuffer` DataView branch): the partial-DataView
|
||||||
|
test fails with
|
||||||
|
|
||||||
|
```
|
||||||
|
not ok - byteLength preserved
|
||||||
|
is(cloned.byteLength, view.byteLength, 'byteLength preserved')
|
||||||
|
```
|
||||||
|
|
||||||
|
(expected 8, got 12), while the full-buffer case still passes.
|
||||||
|
|
||||||
|
## Gate
|
||||||
|
|
||||||
|
- Tests: `node test/index.js` -> `1..144`, 144 ok, 0 failing, exit 0.
|
||||||
|
The repository's `npm test` invokes the `tap` CLI, whose bundled `esm` loader
|
||||||
|
crashes on load under Node 22 (`Function.prototype.apply was called on
|
||||||
|
undefined`) independently of this change; running the test file directly
|
||||||
|
executes the same assertions.
|
||||||
|
- Lint: `standard index.js test/index.js` -> exit 0.
|
||||||
@@ -6,6 +6,12 @@ function copyBuffer (cur) {
|
|||||||
return Buffer.from(cur)
|
return Buffer.from(cur)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A DataView has no `length`; its third constructor argument is a byte
|
||||||
|
// length, so use `byteLength`. TypedArrays take an element count via `length`.
|
||||||
|
if (cur instanceof DataView) {
|
||||||
|
return new DataView(cur.buffer.slice(), cur.byteOffset, cur.byteLength)
|
||||||
|
}
|
||||||
|
|
||||||
return new cur.constructor(cur.buffer.slice(), cur.byteOffset, cur.length)
|
return new cur.constructor(cur.buffer.slice(), cur.byteOffset, cur.length)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -283,6 +283,33 @@ function types (clone, label) {
|
|||||||
deepEqual(Array.from(cloned.view2), [input2, input3], 'cloned value content is correct')
|
deepEqual(Array.from(cloned.view2), [input2, input3], 'cloned value content is correct')
|
||||||
deepEqual(Array.from(cloned.view3), [input1, 0, input2, input3], 'cloned value content is correct')
|
deepEqual(Array.from(cloned.view3), [input1, 0, input2, input3], 'cloned value content is correct')
|
||||||
})
|
})
|
||||||
|
test(`${label} copies a DataView over the full buffer correctly`, async ({ ok, is, isNot }) => {
|
||||||
|
const input = rnd(1000)
|
||||||
|
const buffer = new ArrayBuffer(8)
|
||||||
|
const view = new DataView(buffer)
|
||||||
|
view.setInt32(0, input)
|
||||||
|
const cloned = clone({ a: view }).a
|
||||||
|
ok(cloned instanceof DataView, 'cloned value is instance of DataView')
|
||||||
|
isNot(cloned, view, 'cloned value is not same as input value')
|
||||||
|
is(cloned.byteOffset, view.byteOffset, 'byteOffset preserved')
|
||||||
|
is(cloned.byteLength, view.byteLength, 'byteLength preserved')
|
||||||
|
is(cloned.getInt32(0), input, 'cloned value content is correct')
|
||||||
|
})
|
||||||
|
test(`${label} copies a partial DataView correctly`, async ({ ok, is, isNot }) => {
|
||||||
|
const input = rnd(100)
|
||||||
|
const buffer = new ArrayBuffer(16)
|
||||||
|
const view = new DataView(buffer, 4, 8) // offset 4, length 8 of a 16-byte buffer
|
||||||
|
view.setInt32(0, input)
|
||||||
|
const cloned = clone({ a: view }).a
|
||||||
|
ok(cloned instanceof DataView, 'cloned value is instance of DataView')
|
||||||
|
isNot(cloned, view, 'cloned value is not same as input value')
|
||||||
|
is(cloned.byteOffset, view.byteOffset, 'byteOffset preserved')
|
||||||
|
is(cloned.byteLength, view.byteLength, 'byteLength preserved')
|
||||||
|
is(cloned.getInt32(0), input, 'cloned value content is correct')
|
||||||
|
// Mutating the source must not affect the clone.
|
||||||
|
view.setInt32(0, input + 1)
|
||||||
|
is(cloned.getInt32(0), input, 'clone is independent of the source')
|
||||||
|
})
|
||||||
test(`${label} - maps`, async ({ same, isNot }) => {
|
test(`${label} - maps`, async ({ same, isNot }) => {
|
||||||
const map = new Map([['a', 1]])
|
const map = new Map([['a', 1]])
|
||||||
same(Array.from(clone(map)), [['a', 1]], 'same value')
|
same(Array.from(clone(map)), [['a', 1]], 'same value')
|
||||||
|
|||||||
Reference in new issue
Block a user