diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..9d85dca --- /dev/null +++ b/PR.md @@ -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. diff --git a/index.js b/index.js index a964561..65db480 100644 --- a/index.js +++ b/index.js @@ -6,6 +6,12 @@ function copyBuffer (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) } diff --git a/test/index.js b/test/index.js index 5bdd9ac..de4e4db 100644 --- a/test/index.js +++ b/test/index.js @@ -283,6 +283,33 @@ function types (clone, label) { 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') }) + 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 }) => { const map = new Map([['a', 1]]) same(Array.from(clone(map)), [['a', 1]], 'same value')