diff --git a/changelog.d/8083-dataview-aliases-typed-array-storage.md b/changelog.d/8083-dataview-aliases-typed-array-storage.md new file mode 100644 index 0000000000..5ce27f08f7 --- /dev/null +++ b/changelog.d/8083-dataview-aliases-typed-array-storage.md @@ -0,0 +1,27 @@ +### Fixed + +- **runtime: a `DataView` did not see writes made through a multi-byte typed array over the same `ArrayBuffer`.** `new Uint32Array(ab)` followed by `w[0] = 0x01020304` left `new DataView(ab)` reading the bytes that were there when the DataView was *constructed* — `dv.getUint8(0..3)` summed to `0` where node says `10`. No throw, no null: just numbers from before the write. This is the DataView half of the #7219 aliasing family; the typed-array-to-typed-array half shipped as `test_gap_typedarray_buffer_aliasing_7219.ts`. + + **Root cause: the DataView read its own snapshot, not the backing store.** `js_data_view_new` (`buffer/from.rs:894`) allocates the view its own `BufferHeader`, copies the window's bytes in, and registers it in the buffer view registry. That local copy is refreshed only by writes that route *through* the registry — `js_buffer_set`, `js_buffer_write`, a sibling DataView's `set*`. A `Uint16Array`/`Uint32Array`/`Float64Array` element store does not: `typedarray_view::register_view_meta` makes `typedarray::data_ptr_mut` resolve straight into the backing `ArrayBuffer`, and the store lands there with nothing mirroring it into the DataView. `read_bytes` in `buffer/dataview.rs` then read the DataView's inline bytes, so every `get*` returned the snapshot. + + **The fix is one line**: `read_bytes` resolves through `view::resolve_data_ptr`, the canonical view-resolving accessor `read_buffer_byte` has used for `Uint8Array`/`Buffer` receivers since #1205 and every native-span consumer since #6515. Writes were already correct — `write_bytes` mirrors into the backing via `propagate_written_range_from_receiver` — which is why the bug was direction-specific. + + Three things made it read like something else, and all three are now covered by tests: + + - **Element width looked like the trigger.** A `Uint8Array` writer worked, so `DataView` looked fine. It was not the width: a Perry `Uint8Array` over an `ArrayBuffer` is a `BufferHeader` view whose element writes go through `js_buffer_set`, which mirrors into every registered view. Only the kinds that get a `TypedArrayHeader` bypass the registry. + - **Construction order looked like the trigger.** Writing before `new DataView(ab)` worked, because the constructor copies the bytes present at that moment. + - **Buffer identity was already right.** `w.buffer === dv.buffer`, `byteOffset` and `byteLength` all reported correctly, which is why this reads as a correctness bug rather than a modelling one. + + The DataView keeps its own storage — the codegen path that `gep`s against a buffer pointer still needs it, and `write_bytes` still writes it so a `set*` is visible to anything reading the view's inline bytes. What changed is only which copy is *authoritative* on the read. + +- **runtime: `TextDecoder.prototype.decode(dataView)` read the same stale snapshot.** Found while bounding the above, and fixed with it because it is the same line of reasoning: `js_text_decoder_decode_llvm` (`text.rs`) built its byte slice as `buf + sizeof(BufferHeader)` under a comment asserting the bytes are "stored inline". They are not, for a registered view. `decode(dv)` of an `ArrayBuffer` a `Uint32Array` had just written returned `"\0\0\0\0"` where node returns `"ABCD"`, while `decode(ab)` on the same buffer was correct — the tell that the receiver, not the bytes, was the problem. It now resolves through `buffer::resolve_span_data_ptr` like every other native-span consumer, which also fixes the `Buffer.from(ab)` / `subarray` receivers that share the branch. + +### Testing + +- `test-files/test_gap_dataview_buffer_aliasing_7219.ts` — byte-exact against `node --experimental-strip-types` 26.5.1 across every element width (`Uint8`/`Uint16`/`Int32`/`Float64`), both directions, DataView constructed before *and* after the writes, a windowed `new DataView(ab, 4, 8)`, `getInt16`/`getInt32`/`getFloat64` with and without the little-endian flag (DataView defaults to big-endian while a typed array is platform-endian — the asymmetry a byte-swapping "fix" would break), two DataViews over one buffer, a DataView over a typed array's lazily-materialized `.buffer`, module scope as well as function scope, and a loop-carried read/write mix. At base, 8 of its 11 lines were wrong. +- Three unit tests, all `cargo-test`-visible (per #5960) and all watched fail with their fix reverted: `data_view_reads_multi_byte_typed_array_writes` and `multi_byte_typed_array_reads_windowed_data_view_writes` in `crates/perry-runtime/src/buffer/mod.rs` (`DataView byte 0 lags the typed-array write: left 0.0, right 2.0`), and `text_decoder_reads_backing_store_of_a_data_view` in `crates/perry-runtime/src/text.rs` (`left "\0\0\0\0", right "ABCD"`). Each asserts the typed-array store actually reached the `ArrayBuffer` before comparing, so a store that never landed cannot pass it vacuously (`0 == 0`). + +### Known limitations (unchanged by this change) + +- A `DataView` over a **detached** buffer throws `RangeError`, where node throws `TypeError`. Detach zeroes every registered view's length (`buffer/detach.rs:76-83`), so the read is rejected as out-of-bounds before it can report the detach. +- **Resizable `ArrayBuffer`s are still unimplemented** (`ab.resize` is not a function), so a length-tracking DataView over one has nothing to track. diff --git a/crates/perry-runtime/src/buffer/dataview.rs b/crates/perry-runtime/src/buffer/dataview.rs index 3eaa14ce0f..46241ce3d6 100644 --- a/crates/perry-runtime/src/buffer/dataview.rs +++ b/crates/perry-runtime/src/buffer/dataview.rs @@ -151,6 +151,18 @@ fn to_number(value: f64) -> f64 { /// Read `width` bytes starting at `offset` from a DataView's backing storage. /// Throws `RangeError` (`ERR_OUT_OF_BOUNDS`) when the range escapes the view. +/// +/// Resolves through the view registry rather than reading the DataView's own +/// inline bytes: `js_data_view_new` seeds that storage with a *snapshot* of the +/// backing taken at construction, and only writes routed through the registry +/// (`js_buffer_set`, `js_buffer_write`, a sibling DataView's `set*`, …) refresh +/// it. A typed array aliasing the same `ArrayBuffer` writes its elements +/// straight into the backing store (`typedarray::data_ptr_mut` — see +/// `typedarray_view::view_backing_data_ptr`), so nothing refreshes the +/// snapshot and every `get*` returned pre-write bytes. `read_buffer_byte` has +/// resolved the backing for `Uint8Array`/`Buffer` views since #1205 for exactly +/// this reason — which is why a `Uint8Array` writer appeared to work while a +/// `Uint16Array`/`Uint32Array`/`Float64Array` writer silently did not. unsafe fn read_bytes(buf: *const BufferHeader, offset: i64) -> [u8; N] { if buf.is_null() || offset < 0 { throw_dataview_oob(); @@ -159,7 +171,7 @@ unsafe fn read_bytes(buf: *const BufferHeader, offset: i64) -> [ if offset + (N as i64) > len { throw_dataview_oob(); } - let base = buffer_data(buf).add(offset as usize); + let base = super::view::resolve_data_ptr(buf).add(offset as usize); let mut out = [0u8; N]; ptr::copy_nonoverlapping(base, out.as_mut_ptr(), N); out diff --git a/crates/perry-runtime/src/buffer/mod.rs b/crates/perry-runtime/src/buffer/mod.rs index bf0aebefdb..16d41ab4b4 100644 --- a/crates/perry-runtime/src/buffer/mod.rs +++ b/crates/perry-runtime/src/buffer/mod.rs @@ -488,6 +488,111 @@ mod tests { } } + /// Bytes currently in an `ArrayBuffer`'s own storage — the shared truth a + /// DataView and a typed array over it must both agree with. + fn backing_bytes(ab: *const BufferHeader, len: usize) -> Vec { + unsafe { std::slice::from_raw_parts(buffer_data(ab), len).to_vec() } + } + + /// A `DataView` and a MULTI-BYTE typed array over the same `ArrayBuffer` + /// must observe each other's writes in both directions. + /// + /// A DataView owns a `BufferHeader` seeded from the backing at construction + /// (`js_data_view_new`), and only writes routed through the view registry + /// refresh that snapshot. A `Uint16Array`/`Uint32Array`/`Float64Array` + /// element store goes straight into the backing store + /// (`typedarray::data_ptr_mut`), so nothing refreshed the snapshot and every + /// `get*` returned pre-write bytes — silently, with no throw. `Uint8Array` + /// masked the bug: its element writes go through `js_buffer_set`, which + /// mirrors into every registered view. + #[test] + fn data_view_reads_multi_byte_typed_array_writes() { + let undef = f64::from_bits(crate::value::TAG_UNDEFINED); + for (kind, elem_size) in [ + (crate::typedarray::KIND_UINT16, 2usize), + (crate::typedarray::KIND_UINT32, 4), + (crate::typedarray::KIND_FLOAT64, 8), + ] { + let ab = js_array_buffer_new(elem_size as i32); + let ab_value = f64::from_bits(crate::value::JSValue::pointer(ab as *const u8).bits()); + let ta = + crate::typedarray_view::js_typed_array_view(kind as i32, ab_value, undef, undef); + assert!(!ta.is_null(), "kind={kind}: typed-array view"); + // Constructed BEFORE the write, so its snapshot is all zeroes and a + // stale read is unambiguous. + let dv = js_data_view_new(ab_value, undef, undef); + + crate::typedarray::js_typed_array_set(ta, 0, 258.0); + + // Subject-liveness: without a store that actually reaches the + // ArrayBuffer, every byte comparison below would pass vacuously + // (0 == 0). + let backing = backing_bytes(ab, elem_size); + assert_ne!( + backing, + vec![0u8; elem_size], + "kind={kind}: typed-array store never reached the ArrayBuffer, \ + so this test proves nothing" + ); + + for i in 0..elem_size { + assert_eq!( + js_data_view_get(dv, i as f64, DataViewKind::Uint8, false), + backing[i] as f64, + "kind={kind}: DataView byte {i} lags the typed-array write" + ); + } + } + } + + /// The other direction, and the windowed (`new DataView(ab, 4, 8)`) shape: + /// a DataView write must land in the backing at `byteOffset + offset` where + /// the typed array reads it. + #[test] + fn multi_byte_typed_array_reads_windowed_data_view_writes() { + let undef = f64::from_bits(crate::value::TAG_UNDEFINED); + let ab = js_array_buffer_new(16); + let ab_value = f64::from_bits(crate::value::JSValue::pointer(ab as *const u8).bits()); + let ta = crate::typedarray_view::js_typed_array_view( + crate::typedarray::KIND_UINT32 as i32, + ab_value, + undef, + undef, + ); + assert!(!ta.is_null()); + // Window covering elements 1 and 2 of the Uint32Array. + let dv = js_data_view_new(ab_value, 4.0, 8.0); + + // DataView -> typed array. Big-endian (the DataView default) so the + // byte order is fixed regardless of host endianness. + js_data_view_set(dv, 0.0, 0x0102_0304u32 as f64, DataViewKind::Uint32, false); + assert_eq!( + backing_bytes(ab, 16)[4..8], + [0x01, 0x02, 0x03, 0x04], + "windowed DataView write must land at byteOffset 4 of the backing" + ); + assert_eq!( + crate::typedarray::js_typed_array_get(ta, 1), + u32::from_ne_bytes([0x01, 0x02, 0x03, 0x04]) as f64, + "typed array must read the DataView's bytes" + ); + + // typed array -> windowed DataView, at the window's far end. + crate::typedarray::js_typed_array_set(ta, 2, 0xDEAD_BEEFu32 as f64); + let backing = backing_bytes(ab, 16); + for i in 0..8usize { + assert_eq!( + js_data_view_get(dv, i as f64, DataViewKind::Uint8, false), + backing[4 + i] as f64, + "windowed DataView byte {i} must mirror backing byte {}", + 4 + i + ); + } + // Bytes outside the window stay untouched and unreachable. + assert_eq!(&backing[0..4], &[0u8; 4], "element 0 must be untouched"); + assert_eq!(&backing[12..16], &[0u8; 4], "element 3 must be untouched"); + } + /// Same SSO value, but decoded under the `hex` encoding tag (1): the /// short string holds hex digits and must produce the decoded bytes, /// proving the SSO branch routes through the shared encoding helper diff --git a/crates/perry-runtime/src/text.rs b/crates/perry-runtime/src/text.rs index ae13be192b..d5ce51919c 100644 --- a/crates/perry-runtime/src/text.rs +++ b/crates/perry-runtime/src/text.rs @@ -523,11 +523,15 @@ pub extern "C" fn js_text_decoder_decode_llvm(handle: f64, value: f64) -> i64 { || crate::buffer::is_registered_buffer(ptr_usize) { // DataView, (Shared)ArrayBuffer, or a registered Buffer/Uint8Array - // — all BufferHeader-backed with the bytes stored inline. + // — all BufferHeader-backed. Their bytes are not necessarily + // INLINE, though: a registered view (a DataView, a `Buffer.from(ab)` + // window, a subarray) keeps a construction-time copy that only + // registry-routed writes refresh, so a multi-byte typed array over + // the same backing decoded as pre-write bytes. Resolve the window + // the way every other native-span consumer does (#6515). let buf = ptr_usize as *const BufferHeader; let len = (*buf).length as usize; - let data = (buf as *const u8).add(std::mem::size_of::()); - std::slice::from_raw_parts(data, len) + std::slice::from_raw_parts(crate::buffer::resolve_span_data_ptr(buf), len) } else { // Plain arrays, plain objects, strings — reject like Node. throw_invalid_decode_input(); @@ -668,3 +672,61 @@ pub(crate) unsafe fn text_handle_property( } None } + +#[cfg(test)] +mod tests { + use super::*; + + /// `TextDecoder.decode(dataView)` must read the backing store, not the + /// DataView's construction-time snapshot. A `Uint32Array` over the same + /// `ArrayBuffer` writes straight into the backing, so the snapshot decoded + /// as the pre-write bytes (four NULs for a freshly allocated buffer) while + /// decoding the `ArrayBuffer` itself produced the right text — the same + /// stale-view class as the numeric accessors, and the reason every + /// native-span consumer resolves through `buffer::resolve_span_data_ptr` + /// (#6515). + #[test] + fn text_decoder_reads_backing_store_of_a_data_view() { + let undef = f64::from_bits(crate::value::TAG_UNDEFINED); + let ab = crate::buffer::js_array_buffer_new(4); + let ab_value = f64::from_bits(crate::value::JSValue::pointer(ab as *const u8).bits()); + let words = crate::typedarray_view::js_typed_array_view( + crate::typedarray::KIND_UINT32 as i32, + ab_value, + undef, + undef, + ); + assert!(!words.is_null()); + let dv = crate::buffer::js_data_view_new(ab_value, undef, undef); + + // "ABCD" on a little-endian host, "DCBA" on a big-endian one — the + // assertion compares against the backing rather than a fixed literal. + crate::typedarray::js_typed_array_set(words, 0, 0x4443_4241u32 as f64); + let expected = unsafe { + std::str::from_utf8(std::slice::from_raw_parts( + crate::buffer::buffer_data(ab), + 4, + )) + .expect("ASCII bytes") + .to_string() + }; + assert_ne!( + expected, "\0\0\0\0", + "the typed-array store must have reached the ArrayBuffer" + ); + + // An unregistered handle decodes as non-fatal utf-8 — the default this + // test wants, and no decoder registry setup. + let decoded = js_text_decoder_decode_llvm(0.0, dv); + let s = decoded as *const StringHeader; + assert!(!s.is_null()); + let got = unsafe { + let len = (*s).byte_len as usize; + let data = (s as *const u8).add(std::mem::size_of::()); + std::str::from_utf8(std::slice::from_raw_parts(data, len)) + .expect("ASCII bytes") + .to_string() + }; + assert_eq!(got, expected, "decode(DataView) must see the backing bytes"); + } +} diff --git a/test-files/test_gap_dataview_buffer_aliasing_7219.ts b/test-files/test_gap_dataview_buffer_aliasing_7219.ts new file mode 100644 index 0000000000..41b09934e6 --- /dev/null +++ b/test-files/test_gap_dataview_buffer_aliasing_7219.ts @@ -0,0 +1,197 @@ +// The DataView half of the #7219 aliasing family (the typed-array-to-typed-array +// half is `test_gap_typedarray_buffer_aliasing_7219.ts`). +// +// A DataView owns a BufferHeader seeded from the backing ArrayBuffer at +// construction, and only writes routed through the buffer view registry +// refreshed that snapshot. A Uint16Array/Uint32Array/Float64Array element store +// goes straight into the backing store, so nothing refreshed the snapshot and +// every `get*` returned the bytes present when the DataView was built — no +// throw, no null, just stale numbers. +// +// A Uint8Array writer masked it (its element writes go through the registry and +// mirror into every view), which is why the bug read as "DataView is fine". + +// The issue's reproduction. The sum is endian-independent: the four bytes are +// 1, 2, 3 and 4 in either order. +function u32Writer(): number { + const ab = new ArrayBuffer(4); + const words = new Uint32Array(ab); + const dv = new DataView(ab); + words[0] = 0x01020304; + return dv.getUint8(0) + dv.getUint8(1) + dv.getUint8(2) + dv.getUint8(3); +} +console.log("u32 writer:", u32Writer()); + +// Every element width, including the one-byte case that always worked. +function everyWidth(): string { + const parts: number[] = []; + { + const ab = new ArrayBuffer(4); + const w = new Uint8Array(ab); + const dv = new DataView(ab); + w[0] = 1; + w[3] = 4; + parts.push(dv.getUint8(0) + dv.getUint8(3)); + } + { + const ab = new ArrayBuffer(4); + const w = new Uint16Array(ab); + const dv = new DataView(ab); + w[0] = 0x0102; + w[1] = 0x0304; + parts.push(dv.getUint8(0) + dv.getUint8(1) + dv.getUint8(2) + dv.getUint8(3)); + } + { + const ab = new ArrayBuffer(4); + const w = new Int32Array(ab); + const dv = new DataView(ab); + w[0] = -1; + parts.push(dv.getUint8(0) + dv.getUint8(1) + dv.getUint8(2) + dv.getUint8(3)); + } + { + const ab = new ArrayBuffer(8); + const w = new Float64Array(ab); + const dv = new DataView(ab); + w[0] = 1.5; + let sum = 0; + for (let i = 0; i < 8; i++) sum += dv.getUint8(i); + parts.push(sum); + } + return parts.join(" "); +} +console.log("every width:", everyWidth()); + +// The reverse direction (DataView write, typed-array read) always worked +// because the DataView setter mirrors into the backing; it must stay working. +function dataViewWriter(): string { + const ab = new ArrayBuffer(8); + const words = new Uint32Array(ab); + const doubles = new Float64Array(new ArrayBuffer(8)); + const dv = new DataView(ab); + const dv2 = new DataView(doubles.buffer); + dv.setUint32(0, 0x01020304, true); + dv2.setFloat64(0, -3.25, true); + return `${words[0]} ${doubles[0]}`; +} +console.log("DataView writer:", dataViewWriter()); + +// Constructing the DataView AFTER the write always worked — the snapshot +// captured the bytes already there — which is what made this look like a +// construction-order quirk rather than a lost alias. Writes after that point +// must keep flowing. +function constructedAfter(): string { + const ab = new ArrayBuffer(4); + const words = new Uint32Array(ab); + words[0] = 0x01020304; + const dv = new DataView(ab); + const first = dv.getUint8(0) + dv.getUint8(1) + dv.getUint8(2) + dv.getUint8(3); + words[0] = 0x05060708; + const second = dv.getUint8(0) + dv.getUint8(1) + dv.getUint8(2) + dv.getUint8(3); + return `${first} ${second}`; +} +console.log("constructed after:", constructedAfter()); + +// A windowed DataView reads its own [byteOffset, +byteLength) slice of the +// backing, and writes land there — the window must not shift by the resolution. +function windowedView(): string { + const ab = new ArrayBuffer(16); + const words = new Uint32Array(ab); + const dv = new DataView(ab, 4, 8); + words[0] = 0x11111111; + words[1] = 0x01020304; + words[2] = 0x05060708; + words[3] = 0x22222222; + let sum = 0; + for (let i = 0; i < 8; i++) sum += dv.getUint8(i); + dv.setUint8(0, 0xff); + return `${sum} ${dv.byteLength} ${dv.byteOffset} ${words[1] !== 0x01020304}`; +} +console.log("windowed view:", windowedView()); + +// DataView is big-endian by default while a typed array is platform-endian, so +// the two disagree on purpose. Reading the SAME bytes both ways is what pins +// the fix to "alias the storage" rather than "byte-swap somewhere". +function endianness(): string { + const ab = new ArrayBuffer(8); + const bytes = new Uint8Array(ab); + const dv = new DataView(ab); + bytes[0] = 0x01; + bytes[1] = 0x02; + const i16 = `${dv.getInt16(0)} ${dv.getInt16(0, true)}`; + const words = new Int32Array(ab); + words[0] = -66052; // 0xFFFEFDFC + const i32 = `${dv.getInt32(0)} ${dv.getInt32(0, true)}`; + const doubles = new Float64Array(ab); + doubles[0] = -3.25; + const f64 = `${dv.getFloat64(0, true)}`; + return `${i16} ${i32} ${f64}`; +} +console.log("endianness:", endianness()); + +// Two DataViews over one buffer, one of them offset: both must track the +// backing and each other. +function twoViews(): string { + const ab = new ArrayBuffer(8); + const words = new Uint16Array(ab); + const whole = new DataView(ab); + const tail = new DataView(ab, 2); + words[1] = 0xbeef; + const seen = `${whole.getUint16(2)} ${tail.getUint16(0)}`; + tail.setUint16(0, 0x1234); + return `${seen} ${words[1]} ${whole.getUint16(2)}`; +} +console.log("two views:", twoViews()); + +// Module scope, not inside a function: the same shapes must hold when the +// bindings are module-level (a different codegen tier). +const mAb = new ArrayBuffer(8); +const mWords = new Uint32Array(mAb); +const mDv = new DataView(mAb); +mWords[0] = 0x01020304; +mWords[1] = 0x05060708; +console.log( + "module scope:", + mDv.getUint8(0) + mDv.getUint8(1) + mDv.getUint8(2) + mDv.getUint8(3), + mDv.getUint32(4, true) === 0x05060708 || mDv.getUint32(4) === 0x05060708, +); + +// A DataView over a typed array's lazily-materialized `.buffer` aliases the +// same storage in both directions. +function overMaterializedBuffer(): string { + const words = new Uint32Array(2); + const dv = new DataView(words.buffer); + words[0] = 0x01020304; + const read = dv.getUint8(0) + dv.getUint8(1) + dv.getUint8(2) + dv.getUint8(3); + dv.setUint8(4, 1); + dv.setUint8(5, 2); + dv.setUint8(6, 3); + dv.setUint8(7, 4); + return `${read} ${words[1] !== 0}`; +} +console.log("over materialized buffer:", overMaterializedBuffer()); + +// A loop that writes through the typed array and reads through the DataView — +// the shape any codegen fast path would take over. +function loopedMix(): string { + const ab = new ArrayBuffer(64); + const words = new Uint32Array(ab); + const dv = new DataView(ab); + for (let i = 0; i < 16; i++) words[i] = i * 0x01010101; + let sum = 0; + for (let i = 0; i < 64; i++) sum += dv.getUint8(i); + for (let i = 0; i < 16; i++) dv.setUint32(i * 4, i + 1, true); + let total = 0; + for (let i = 0; i < 16; i++) total += words[i]; + return `${sum} ${total}`; +} +console.log("looped mix:", loopedMix()); + +// Buffer identity and metadata were already right — only the bytes were not +// shared, which is why this reads as a correctness bug, not a modelling one. +function identity(): string { + const ab = new ArrayBuffer(16); + const words = new Uint32Array(ab, 4); + const dv = new DataView(ab, 4, 8); + return `${words.buffer === ab} ${dv.buffer === ab} ${dv.byteLength} ${dv.byteOffset} ${words.byteOffset}`; +} +console.log("identity:", identity());