feat(ses): ArrayBuffer.p.transferToImmutable

erights commented 3 weeks ago

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

Description

Alternative to #2309 as suggested by @phoddie at https://github.com/endojs/endo/pull/2309#issuecomment-2155513240

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

[x] 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 https://github.com/endojs/endo/pull/2311#discussion_r1632607527 . 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 commented 3 weeks ago

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

mhofman commented 3 weeks ago

From https://github.com/endojs/endo/pull/2309#issuecomment-2155714787

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

erights commented 3 weeks ago

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 commented 3 weeks ago

Converted to draft, because all the subtlety around resizing is

currently still wrong
needs tests.

mhofman commented 3 weeks ago

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)

mhofman commented 3 weeks ago

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 commented 2 weeks ago

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).

endojs / endo