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

feat(ses): ArrayBuffer.p.transferToImmutable #2311

Closed
wants to merge 12 commits into from
Closed

Conversation

erights
Copy link
Contributor

@erights erights commented Jun 8, 2024

Closes: #XXXX
Refs: #1538 #1331 #2309

Description

Alternative to #2309 as suggested by @phoddie at #2309 (comment)

A step towards fixing #1331 , likely by restaging #1538 on this one and then fixing it.
By making ArrayBuffer.p.immutable as-if part of the language, in #1538 passStyleOf will be able to recognize an immutable ArrayBuffer even though it carries its own methods, avoiding the eval-twin problems that otherwise thwart such plans. This is based on one of the candidates explained at #1331 . That passStyleOf behavior, together with ArrayBuffer.p.transferToImmutable opens the door for marshal to serialize and unserialize these as additional ocapn Passables.

Additionally, by proposing it to tc39 as explained below, we'd enable immutable TypedArrays and DataViews as well, and XS could place all these in ROM cheaply, while conforming to the language spec. When also hardened, XS could judge these to be pure. Attn @phoddie @patrick-soquet

  • TODO Must treat the hidden prototype and its methods as hidden intrinsics, so they get hardened when they should, along with the others.

From the initial NEWS.md

  • Adds ArrayBuffer.p.immutable and ArrayBuffer.p.transferToImmutable as a shim for a future proposal. It makes an ArrayBuffer-like object whose contents cannot be mutated. However, due to limitations of the shim
    • Unlike ArrayBuffer and SharedArrayBuffer this shim's ArrayBuffer-like object cannot be transfered or cloned between JS threads.
    • Unlike ArrayBuffer and SharedArrayBuffer, this shim's ArrayBuffer-like object cannot be used as the backing store of TypeArrays or DataViews.
    • On Node >= 21 we use the builtin transferToFixed to transfer exclusive access to the array buffer contents. On Node <= 20, we emulate transferToFixedLength with structuredClone. On platforms with neither transferToFixedLength nor structuredClone, we use slice to copy the contents, but have no way to detach the original.
    • Even after the upcoming transferToImmutable proposal is implemented by the platform, the current code will still replace it with the shim implementation, in accord with shim best practices. See feat(ses): ArrayBuffer.p.transferToImmutable #2311 (comment) . It will require a later manual step to delete the shim, after manual analysis of the compat implications.

Security Considerations

The eval-twin problem explained at #1331 is a security problem. This PR is one candidate for solving that problem, unblocking #1538 so it can fix #1331. Further, if accepted into a future version of the language, the immutability it provides will generally help security.

Scaling Considerations

This shim implementation likely does more copying than even a naive native implementation would. A native implementation may even engage in copy-on-write tricks that this shim cannot. Use of the shim should beware of these "extra" copying costs. (Starting in Node 21, the shim's ArrayBuffer.p.transferToImmutable will no longer do an extra copy.)

Compatibility and Documentation Considerations

Generally we've kept hardened JS close to standard JS, and we've kept the ses-shim close to hardened JS. With this PR, we'd need to explain ArrayBuffer.p.transferToImmutable and ArrayBuffer.p.immutable as part of the hardened JS implemented by the ses-shim, even though we have not yet proposed it to tc39.

Testing Considerations

Ideally, we should identify the subset of test262 ArrayBuffer tests that should be applicable to immutable ArrayBuffers, and duplicate them for that purpose.

Upgrade Considerations

Nothing breaking.

NEWS.md updated

@erights erights self-assigned this Jun 8, 2024
@erights erights force-pushed the markm-byte-array-2 branch 3 times, most recently from f2adf5f to bf44e52 Compare June 8, 2024 01:47
@erights erights changed the title feat(ses): ArrayBuffer.transferToImmutable feat(ses): ArrayBuffer.p.transferToImmutable Jun 8, 2024
@erights erights force-pushed the markm-byte-array-2 branch from bf44e52 to 929c303 Compare June 8, 2024 02:01
@erights erights marked this pull request as ready for review June 8, 2024 02:02
@erights
Copy link
Contributor Author

erights commented Jun 8, 2024

@phoddie @patrick-soquet , please consider yourselves reviewers of this one instead. I look forward to your comments.

@erights erights force-pushed the markm-byte-array-2 branch from 929c303 to 8fdd326 Compare June 8, 2024 02:07
@mhofman
Copy link
Contributor

mhofman commented Jun 8, 2024

From #2309 (comment)

I just noticed another problem in shimming this though. Node 20, which we still support, has no way within the language to detach an ArrayBuffer. Therefore, on Node 20, transferToImmutable will copy (using slice) and leave the original undetached. Just another loss of fidelity for the shim that needs to be documented. Sigh. Still worth it to have a cleaner and more minimal proposal.

Actually Node.js supports structuredClone as a global since Node v17, so you definitely can transfer an ArrayBuffer in all supported versions

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions to better emulate transferToFixedLength.

packages/ses/src/commons.js Outdated Show resolved Hide resolved
packages/ses/NEWS.md Outdated Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Jun 8, 2024

From #2309 (comment)

I just noticed another problem in shimming this though. Node 20, which we still support, has no way within the language to detach an ArrayBuffer. Therefore, on Node 20, transferToImmutable will copy (using slice) and leave the original undetached. Just another loss of fidelity for the shim that needs to be documented. Sigh. Still worth it to have a cleaner and more minimal proposal.

Actually Node.js supports structuredClone as a global since Node v17, so you definitely can transfer an ArrayBuffer in all supported versions

Done, using Richard's suggested code.

@erights erights requested a review from gibson042 June 9, 2024 04:36
@erights erights requested a review from gibson042 June 9, 2024 20:22
Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transferToImmutable should detach the original buffer is possible

packages/ses/NEWS.md Outdated Show resolved Hide resolved
packages/ses/src/commons.js Outdated Show resolved Hide resolved
Comment on lines 11 to 12
// If it already exists, don't replace it with the shim.
if (!('transferToImmutable' in arrayBufferPrototype)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional shims for non stage 4 features have been seen as problematic by the community as there is a risk that the program would start relying on a shim behavior that does not match what the final spec says, resulting in breakage when the engine starts implementing the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Will do. Is there something I can cite?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is an article out there but I couldn't find a link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply cited your comment above. Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional shims for non stage 4 features have been seen as problematic by the community as there is a risk that the program would start relying on a shim behavior that does not match what the final spec says, resulting in breakage when the engine starts implementing the feature... I believe there is an article out there but I couldn't find a link

https://ponyfill.com (which conveniently also links to http://svdictionary.com/words/ponyfill and https://ponyfoo.com/articles/polyfills-or-ponyfills and https://kikobeats.com/polyfill-ponyfill-and-prollyfill/ ).

Summary: for work like this, we can export a function for use like transferToImmutable(arrayBuffer) and transferToImmutable(arrayBuffer, newLength), but SHOULD NOT add any new methods or properties to ArrayBuffer.prototype.

@erights erights requested a review from mhofman June 11, 2024 20:53
@erights erights force-pushed the markm-byte-array-2 branch from cf85382 to af1d346 Compare June 11, 2024 20:57
@erights erights marked this pull request as draft June 11, 2024 21:58
@erights
Copy link
Contributor Author

erights commented Jun 11, 2024

Converted to draft, because all the subtlety around resizing is

  • currently still wrong
  • needs tests.

@mhofman
Copy link
Contributor

mhofman commented Jun 12, 2024

Converted to draft, because all the subtlety around resizing is

Sorry I think I expressed myself poorly in previous comments.

  • If there is no "arraybuffer resize" feature in the platform, we shouldn't be in the business of providing a shim for it.
    • IMO it's acceptable to have transferToImmutable shimmed but not transferToFixedLength.
  • We can use either structuredClone or transferToFixedLength to implement detachment of array buffer. If neither is available, we fallback to slice, which is a shim fidelity issue only (and performance penalty)

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments to improve fidelity:

  • support for expansion to a bigger size, e.g. new ArrayBuffer(6).transferToImmutable(8) (which should also be tested)
  • proper receiver and argument checking
  • extraction and use of ArrayBuffer.prototype byteLength and resizable getters and %TypedArray.prototype% buffer getter

Comment on lines +229 to +231
// It might seem like we could avoid the extra copy by
// `newBuffer.resize(newLength)`. But `structuredClone`
// makes ArrayBuffers that are not resizable.
Copy link
Contributor

@gibson042 gibson042 Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They seem resizable to me:

$ node -e '
  console.log("Node.js", process.version);
  const buf1 = new ArrayBuffer(10, { maxByteLength: 42 });
  const buf2 = structuredClone(buf1, { transfer: [buf1] });
  console.log({ buf1, resizable: buf1.resizable });
  console.log({ buf2, resizable: buf2.resizable });
  buf2.resize(5);
  console.log({ buf2, resizable: buf2.resizable });
'
Node.js v20.14.0
{ buf1: ArrayBuffer { (detached), byteLength: 0 }, resizable: true }
{
  buf2: ArrayBuffer {
    [Uint8Contents]: <00 00 00 00 00 00 00 00 00 00>,
    byteLength: 10
  },
  resizable: true
}
{
  buf2: ArrayBuffer { [Uint8Contents]: <00 00 00 00 00>, byteLength: 5 },
  resizable: true
}

Comment on lines +194 to +233
(arrayBuffer, newLength = arrayBuffer.byteLength) => {
// There is no `transferToFixedLength` on Node <= 20, but there
// is web-standard `structuredClone` on Node >= 17, on all modern
// browsers, and on many other JS platforms.
// In those cases, we first use `structuredClone` to get a fresh
// buffer with exclusive access to the underlying data, while
// detaching it from the original `arrayBuffer`.

newLength = +newLength;
bigIntAsUintN(newLength, 0n);

const newBuffer = /** @type {ArrayBuffer} */ (
structuredClone(arrayBuffer, {
transfer: [arrayBuffer],
})
);
if (newLength >= newBuffer.byteLength) {
return newBuffer;
}
// If the requested length is shorter than the length of `buffer`,
// we use `slice` to shorted the returned result, but at the cost
// of an extra copy.
//
// `slice` accepts negative arguments but `transferToFixedLength`
// does not...
// get at the underlying ToIndex operation through `BigInt.asUintN`
// (avoiding the redundant allocation of e.g. `ArrayBuffer(newLength)`)
// and ToNumber through unary `+` (rather than `Number(newLength)`,
// which fails to reject BigInts).
//
// On platforms like Node 20
// - without`tranferToFixedLength` or `transfer`
// - with `structuredClone`
// - with `resize`
//
// It might seem like we could avoid the extra copy by
// `newBuffer.resize(newLength)`. But `structuredClone`
// makes ArrayBuffers that are not resizable.
return arrayBufferSlice(newBuffer, 0, newLength);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(arrayBuffer, newLength = arrayBuffer.byteLength) => {
// There is no `transferToFixedLength` on Node <= 20, but there
// is web-standard `structuredClone` on Node >= 17, on all modern
// browsers, and on many other JS platforms.
// In those cases, we first use `structuredClone` to get a fresh
// buffer with exclusive access to the underlying data, while
// detaching it from the original `arrayBuffer`.
newLength = +newLength;
bigIntAsUintN(newLength, 0n);
const newBuffer = /** @type {ArrayBuffer} */ (
structuredClone(arrayBuffer, {
transfer: [arrayBuffer],
})
);
if (newLength >= newBuffer.byteLength) {
return newBuffer;
}
// If the requested length is shorter than the length of `buffer`,
// we use `slice` to shorted the returned result, but at the cost
// of an extra copy.
//
// `slice` accepts negative arguments but `transferToFixedLength`
// does not...
// get at the underlying ToIndex operation through `BigInt.asUintN`
// (avoiding the redundant allocation of e.g. `ArrayBuffer(newLength)`)
// and ToNumber through unary `+` (rather than `Number(newLength)`,
// which fails to reject BigInts).
//
// On platforms like Node 20
// - without`tranferToFixedLength` or `transfer`
// - with `structuredClone`
// - with `resize`
//
// It might seem like we could avoid the extra copy by
// `newBuffer.resize(newLength)`. But `structuredClone`
// makes ArrayBuffers that are not resizable.
return arrayBufferSlice(newBuffer, 0, newLength);
}
(arrayBuffer, newLength = arrayBufferByteLength(arrayBuffer)) => {
// There is no `transferToFixedLength` on Node <= 20, but there
// is web-standard `structuredClone` on Node >= 17, on all modern
// browsers, and on many other JS platforms.
// In those cases, we first use `structuredClone` to get a fresh
// buffer with exclusive access to the underlying data, while
// detaching it from the original `arrayBuffer`.
// Before looking at actual arguments, verify that the input is an
// ArrayBuffer.
arrayBufferByteLength(arrayBuffer);
// Calculate ToIndex(newLengthAsNumber) using `BigInt.asUintN`,
// first getting newLengthAsNumber as ToNumber(newLength) using unary `+`
// (rather than `Number(newLength)`, which fails to reject BigInts).
newLength = +newLength;
bigIntAsUintN(newLength, 0n);
const newBuffer = /** @type {ArrayBuffer} */ (
structuredClone(arrayBuffer, {
transfer: [arrayBuffer],
})
);
// We might already have what we need.
// NOTE: The check for resizability is necessary to correctly emulate
// ArrayBuffer.prototype.transferToFixedLength, but a more narrow
// function specialized to e.g. transferToImmutable could skip it.
if (newLength === arrayBufferByteLength(newBuffer) && !arrayBufferResizable(newBuffer)) {
return newBuffer;
}
// We might be able to copy some or all of the contents.
if (newLength <= arrayBufferByteLength(newBuffer)) {
const copied = arrayBufferSlice(newBuffer, 0, newLength);
const copiedLength = arrayBufferByteLength(copied);
if (copiedLength !== newLength) {
throw RangeError(`internal: length ${copiedLength} should have been ${newLength}`);
}
return copied;
}
// We need a bigger boat.
// NOTE: Uint8Array must be extracted from globalThis.
const view = new Uint8Array(newLength);
typedArraySet(view, new Uint8Array(newBuffer));
return typedArrayBuffer(view);

Comment on lines +239 to +260
(arrayBuffer, newLength = arrayBuffer.byteLength) => {
// There is no `transferToFixedLength` on Node <= 20,
// and no `structuredClone` on Node <= 17 and possibly on some
// non-browser JavaScript platforms.
// In those cases,
// and assuming the absence of `transfer`, we cannot detach
// the original, but we must still produce a new fresh buffer with
// exclusive mutability of its underlying state. We use `slice`
// both to make this exclusive copy and size it appropriately.

// `slice` accepts negative arguments but `transferToFixedLength`
// does not...
// get at the underlying ToIndex operation through `BigInt.asUintN`
// (avoiding the redundant allocation of e.g. `ArrayBuffer(newLength)`)
// and ToNumber through unary `+` (rather than `Number(newLength)`,
// which fails to reject BigInts).
newLength = +newLength;
bigIntAsUintN(newLength, 0n);

const newBuffer = arrayBufferSlice(arrayBuffer, 0, newLength);
return newBuffer;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(arrayBuffer, newLength = arrayBuffer.byteLength) => {
// There is no `transferToFixedLength` on Node <= 20,
// and no `structuredClone` on Node <= 17 and possibly on some
// non-browser JavaScript platforms.
// In those cases,
// and assuming the absence of `transfer`, we cannot detach
// the original, but we must still produce a new fresh buffer with
// exclusive mutability of its underlying state. We use `slice`
// both to make this exclusive copy and size it appropriately.
// `slice` accepts negative arguments but `transferToFixedLength`
// does not...
// get at the underlying ToIndex operation through `BigInt.asUintN`
// (avoiding the redundant allocation of e.g. `ArrayBuffer(newLength)`)
// and ToNumber through unary `+` (rather than `Number(newLength)`,
// which fails to reject BigInts).
newLength = +newLength;
bigIntAsUintN(newLength, 0n);
const newBuffer = arrayBufferSlice(arrayBuffer, 0, newLength);
return newBuffer;
};
(arrayBuffer, newLength = arrayBuffer.byteLength) => {
// There is no `transferToFixedLength` on Node <= 20,
// and no `structuredClone` on Node <= 17 and possibly on some
// non-browser JavaScript platforms.
// In those cases,
// and assuming the absence of `transfer`, we cannot detach
// the original, but we must still produce a new fresh buffer with
// exclusive mutability of its underlying state.
// Before looking at actual arguments, verify that the input is an
// ArrayBuffer.
const srcLength = arrayBufferByteLength(arrayBuffer);
// Calculate ToIndex(newLengthAsNumber) using `BigInt.asUintN`,
// first getting newLengthAsNumber as ToNumber(newLength) using unary `+`
// (rather than `Number(newLength)`, which fails to reject BigInts).
newLength = +newLength;
bigIntAsUintN(newLength, 0n);
// We might be able to copy some or all of the contents.
if (newLength <= srcLength) {
const copied = arrayBufferSlice(arrayBuffer, 0, newLength);
const copiedLength = arrayBufferByteLength(copied);
if (copiedLength !== newLength) {
throw RangeError(`internal: length ${copiedLength} should have been ${newLength}`);
}
return copied;
}
// We need a bigger boat.
// NOTE: Uint8Array must be extracted from globalThis.
const view = new Uint8Array(newLength);
typedArraySet(view, new Uint8Array(arrayBuffer));
return typedArrayBuffer(view);
};

Comment on lines +42 to +44
// This also enforces that `buffer` is a genuine `ArrayBuffer`.
// This constructor is deleted from the prototype below.
this.#buffer = arrayBufferSlice(buffer, 0);
Copy link
Contributor

@gibson042 gibson042 Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be slicing an ArrayBuffer that already came from detachment; full responsibility should fall on either transferToImmutable or ImmutableArrayBufferInternal but not both.

@mhofman
Copy link
Contributor

mhofman commented Jun 12, 2024

which is admittedly itself not necessary in the common case of a length-preserving transfer of non-resizable input

  • support for expansion to a bigger size, e.g. new ArrayBuffer(6).transferToImmutable(8)

Before we go on through another round of implementation changes, we should settle the proposed semantics.

IMO, if someone want to change the length, they can do transferToFixedLength(newLength).transferToImmutable().
No need to overload the methods. That only raises complexity of the API and the shim for no good reason.

@gibson042
Copy link
Contributor

IMO, if someone want to change the length, they can do transferToFixedLength(newLength).transferToImmutable(). No need to overload the methods. That only raises complexity of the API and the shim for no good reason.

transfer and transferToFixedLength both support a newLength parameter which can take any value that coerces to a valid ArrayBuffer length (i.e., an integer in [0, 2**53 - 1]). It would be surprising if transferToImmutable were to lack a parameter with identical behavior, and basically unacceptable for it to have a parameter supporting contraction but not expansion (as is currently the case in this PR).

@erights erights force-pushed the markm-byte-array-2 branch from 5eb079d to 2043b76 Compare July 11, 2024 20:22
@erights erights force-pushed the markm-byte-array-2 branch from 2043b76 to 4dc9583 Compare July 13, 2024 22:59
erights added a commit that referenced this pull request Aug 13, 2024
…2399)

Closes: #XXXX
Refs:  #1538 #1331 #2309 #2311 

## Description

Introduces the `@endo/immutable-arraybuffer` package, the ponyfill
exports of `@endo/immutable-arraybuffer`, and the shim obtained by
importing `@endo/immutable-arraybuffer/shim.js`.

Alternative to #2309 as suggested by @phoddie at
#2309 (comment)

We plan to fix #1331 in a stack of PRs starting with this one
- This PR implements a ponyfill and shim for an upcoming *Immutable
ArrayBuffer* proposal, along the lines suggested by @phoddie at
#2309 (comment) + the
suggestions on an earlier state of #2311 . This is a pure JavaScript
ponyfill/shim, leaving it to #2311 to bring it into Hardened JavaScript.
- #2311 imports the #2399 shim, treating the new objects it introduces
as if they are new primordials, to be permitted and hardened.
- #1538 uses the Hardened JavaScript Immutable ArrayBuffers to define a
new `Passable` type, `ByteArray`, corresponding to the
[OCapN](https://ocapn.org/) `ByteArray`.
- Some future PR extending the various marshal formats to encode and
decode the `ByteArray` objects.

See the README.md in this PR for more.

### Security Considerations

Better support for immutability generally helps security. The
imperfections of the shim are a leaky abstraction in all the ways
explained in the Caveats section of the README.md. For example, objects
that are purely artifacts of the emulation, like the
`immutableArrayBufferPrototype`, are easily discoverable, revealing the
emulation's mechanisms.

As a pure JavaScript polyfill/shim, this `@endo/immutable-arraybuffer`
package does not harden the objects it exposes. Thus, by itself it does
not provide much security -- like the initial state of JavaScript does
not by itself provide much security. Rather, both provide securability,
depending on Hardened JavaScript to harden early as needed to provide
the security. See #2311

Once hardened early, the abstraction will still be leaky as above, but
the immutability of the buffer contents are robustly enforced.

### Scaling Considerations

This ponyfill/shim is a zero-copy implementation, meaning that it does
no more buffer copying than expected of a native implementation.

### Compatibility and Documentation Considerations

This ponyfill/shim implements zero-copy by relying on the platform to
provide one of two primitives: `structuredClone` or
`ArrayBuffer.prototype.transfer`. Neither exist on Node <= 16. Without
either, this ponyfill/shim will fail to initialize.

This PR sets the stage for writing an Immutable ArrayBuffer proposal,
proposing it to tc39, and including it in our own documentation.

### Testing Considerations

Ideally, we should identify the subset of test262 `ArrayBuffer` tests
that should be applicable to immutable ArrayBuffers, and duplicate them
for that purpose.

### Upgrade Considerations

Nothing breaking.
@erights
Copy link
Contributor Author

erights commented Aug 16, 2024

Closing in favor of https://github.com/endojs/endo/pull/2400/files

@erights erights closed this Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support passable immutable ArrayBuffer as copy-data
3 participants