Repository navigation
fix(res.send): preserve DataView byte ranges - #7511
nikhiljangid120 wants to merge 2 commits into
Conversation
ToniAdreal
left a comment
ToniAdreal
left a comment
There was a problem hiding this comment.
Independently verified the root cause and the fix on Node v24.20.0:
- Root cause confirmed. In
res.send, a DataView lands in theArrayBuffer.isView(chunk)arm, which only defaults the Content-Type tobin— the actual bufferization happens later, when the Content-Length/ETag path runsBuffer.from(chunk, encoding).Buffer.fromhas no DataView overload and returns an empty Buffer (Buffer.from(new DataView(buf)).length === 0, reproduced locally), so bothContent-Length: 0and the empty body are explained. The asymmetry with typed arrays is worth naming:Buffer.from(Uint16Array([0x0102, 0x0304]))performs an element-truncated copy (<02 04>), which is exactly why every non-DataView view never hit this bug — DataView is the one view typeBuffer.fromsilently drops. - Fix shape is correct.
Buffer.from(chunk.buffer, chunk.byteOffset, chunk.byteLength)goes through the ArrayBuffer overload and copies exactly the view's byte window. Usingchunk.buffer.byteLengthhere would be wrong — it would leak bytes outside the window for sliced views — and the sliced-view regression case pins this. - The
isDataViewdiscriminant is load-bearing, not stylistic.instanceof DataViewfails across realms (vm.runInNewContext('new DataView(...)') instanceof DataView === false, verified), whileutil.types.isDataViewis realm-agnostic. The included cross-realm test case would fail with aninstanceofcheck, so the import choice is doing real work. - Placement is right. Converting inside the
isViewarm, before the Content-Type default, keepsapplication/octet-streamfor DataView bodies (asserted by the tests) and hands the lateretagFn(chunk)a real Buffer, so weak-ETag generation matches Buffer-body behavior. - SAB edge closed too. A SharedArrayBuffer-backed DataView also produced an empty body on the old path (
Buffer.from(sdv).length === 0), and converts without throwing on the fixed path (Buffer.from(sab, off, len)shares memory) — verified on Node 24.
The 11 new cases split cleanly: 8 are true regression cases (full/sliced/cross-realm views, the Content-Type override, HEAD Content-Length — all fail on the old code), and 3 are non-regression guards that pass on both versions, including the Uint16Array element-conversion behavior. LGTM.
|
One question on scope: the test "should preserve element-based conversion for non-byte typed arrays" locks in Since } else if (ArrayBuffer.isView(chunk)) {
if (!Buffer.isBuffer(chunk)) {
chunk = Buffer.from(chunk.buffer, chunk.byteOffset, chunk.byteLength);
}This would change the output for multi-byte typed arrays, so it's a maintainer call whether that belongs in this PR or a separate one. I'd just suggest not adding a test that asserts the truncation. |
3 participants
Fixes #7510.
res.send(DataView)currently returns an empty body becauseBuffer.from(dataView)discards its contents. Convert only DataViews from their backing buffer, byte offset, and byte length.node:util.types.isDataViewalso recognizes cross-realm views.This revisits #7433, which its author closed without merging. The fix preserves the existing Buffer, Uint8Array, and non-byte typed-array paths, without deciding the desired wire representation of multi-byte typed arrays. Bare ArrayBuffer support (#7362) is outside this change.
Verification on Node 24.14.1 / Linux:
npm run test-ci: 1,271 passing.npm run lint: passed.AI assistance was used for investigation, implementation, and local validation.