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(pass-style,marshal): ByteArray, a binary Passable #2414

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erights
Copy link
Contributor

@erights erights commented Aug 16, 2024

Staged on #2414

Closes: #1331
Refs: #1538 ocapn/ocapn#5 (comment)

Description

Use the @endo/immutable-arraybuffer ponyfill directly, rather than the shim @endo/immutable-arraybuffer/shim.js, to provide objects that act like the immutable ArrayBuffers that would result from the transferToImmutable proposal.

Recognize these objects, when also frozen, as the new pass-style byteArray. Add a branch for each of the dispatches on pass-style that must now understand byte-arrays as well. As of this PR, many of those additional branches are stubbed out with "not yet implemented" errors as placeholders.

Security Considerations

Does correctly enforce immutability. But by depending only on the ponyfill rather than the shim, this PR inherits the eval twin problem of the ponyfill. Each instantiation of passStyleOf will only recognize the byte-arrays from the instantiation of @endo/pass-style that made that byte-array. For environment that, for example, use the passStyleOf endowed to them by liveslots, liveslots will also need to endow them with the corresponding byte-array maker.

In theory, this is not much worse than the need to co-endow passStyleOf and Proxy, so that passStyleOf can reject apparent copy structures that are also proxies.

Scaling Considerations

The underlying ponyfill implementation helps us handle bulk binary data in a largely no-copy or minimal-copy manner.

Documentation Considerations

Will add a new pass-style, so everywhere we enumerate all the pass-styles, we'll need to add byte-array.

Testing Considerations

  • TODO tests. For each stubbed out case, there should be a test.failing test.

Compatibility Considerations

Old code that dispatched only on the pass-styles it knew about will not correctly handle passables that contain byte-arrays.

Upgrade Considerations

  • TODO BREAKING.md
  • TODO NEWS.md

@erights erights self-assigned this Aug 16, 2024
@erights erights changed the base branch from master to markm-repair-arraybuffer-transfer August 19, 2024 22:48
@erights erights marked this pull request as ready for review August 19, 2024 23:13
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from 4dd9bf8 to d19e509 Compare August 22, 2024 01:08
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from d19e509 to 585042f Compare August 26, 2024 20:48
@erights erights marked this pull request as draft August 26, 2024 20:52
@erights
Copy link
Contributor Author

erights commented Aug 26, 2024

Converting to draft until more of the stubbed out cases are implemented, and until more of both are tested.

@erights erights force-pushed the markm-repair-arraybuffer-transfer branch 3 times, most recently from 8dce401 to 06e15fe Compare September 2, 2024 21:30
@erights erights force-pushed the markm-binary-direct branch 2 times, most recently from 53ec91b to ce0fd14 Compare September 4, 2024 00:11
erights added a commit that referenced this pull request Sep 4, 2024
Staged on #2419 

Closes: #XXXX
Refs: #2414 #2418 #2419 

## Description

#2414  by itself does not work on Node 18 and Node 20 because
- those platforms do not have `Array.prototype.transfer`, so #2414 must
use `structuredClone` instead
- `structuredClone` does exist on Node >= 18, so it should be on
supported platforms (though see #2418 ). However, `structuredClone`
itself is dangerous and so must not be added to the shared intrinsics.
As a result, in #2414 , when `@endo/pass-style` is initialized in a
created compartment, it fails to find either `Array.prototype.transfer`
and `structuredClone

To solve this, @kriskowal suggested that we also shim
`Array.prototype.transfer` if needed during `lockdown`, along with other
repairs. We are avoiding similarly shimming
`Array.prototype.transferToImmutable` because it is not yet standard.
But `Array.prototype.transfer` is standard, and so `lockdown` can
globally shim it before hardening the shared intrinsics.

This PR implements @kriskowal 's suggestion.

### Security Considerations

none
### Scaling Considerations

by itself, none
### Documentation Considerations

nothing signicant.
### Testing Considerations

See #2418 . Aside from that, none
### Compatibility and Upgrade Considerations

On platforms with neither `Array.prototype.transfer` nor a global
`structuredClone`, the ses-shim will *currently* not install an
emulation of `Array.prototype.transfer`. However, once we verify that
endo is not intended to support platforms without both, we may change
lockdown to throw, failing to lock down.
Base automatically changed from markm-repair-arraybuffer-transfer to master September 4, 2024 21:20
// then according to their lengths.
// Thus, if the data of ByteArray X is a prefix of
// the data of ByteArray Y, then X is smaller than Y.
return comparator(left.byteLength, right.byteLength);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should switch to shortlex. See https://en.wikipedia.org/wiki/Shortlex_order

That way, encodePassable could encode as prefix followed by the unescaped content.

// Thus, if the data of ByteArray X is a prefix of
// the data of ByteArray Y, then X is smaller than Y.
// @ts-expect-error narrowed
return compareRank(left.byteLength, right.byteLength);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should delegate the entire compare to rankOrder, which, btw, will switch to shortlex order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant