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(xsnap): Base64 C bindings' #3817

Merged
merged 4 commits into from
Sep 13, 2021
Merged

feat(xsnap): Base64 C bindings' #3817

merged 4 commits into from
Sep 13, 2021

Conversation

kriskowal
Copy link
Member

  • feat(xsnap): Update xsnap-native for base64
  • feat(xsnap): Add Base64 C bindings

refs: #2682

Description

This change adds a Base64 namespace with encode and decode functions.
A pure JavaScript Base 64 codec performs very poorly on XS.
Decoding 400KB is a common load for a Base 64 encoded Agoric zip archive source bundle,
and takes about an hour to decode in xsnap.
This change surfaces XSnap bindings for Base 64 based on Moddables existing module.

agoric-labs/xsnap-pub#4

Security Considerations

XSnap does not harden the Base64 namespace object.
It should be hardened before introduction to a compartment as an endowment.

Documentation Considerations

This should be transparent to users.
Subsequent work in @endo/base64 will harness this API if it is present on the
global object and work around one known defect by having a fast path for empty
input.

Testing Considerations

This change adds the new Base64 and existing TextEncoder and TextDecoder to the
XSnap REPL so reviewers can fiddle with it using yarn repl in the xsnap
package.

This includes a failing test, where an empty command buffer, Base64 encoded,
then UTF-8 encoded, effects a detached buffer error.
We can work-around this in @endo/base64 for now.

@dckc
Copy link
Member

dckc commented Sep 11, 2021

security considerations += expands the scope of the upcoming memory safety audit (#2466).

@jessysaurusrex In that scoping meeting, I said we use only the xs interpreter code and not the modules. But we started pulling in modules with #3797 . We plan to use sha512 (#3512 ) as well.

@dckc dckc added xsnap the XS execution tool performance Performance related issues labels Sep 11, 2021
@kriskowal kriskowal requested a review from dckc September 11, 2021 19:42
@@ -50,6 +51,61 @@ test('simple TextEncoder / TextDecoder are available', async t => {
t.deepEqual(opts.messages, ['"Hello! 😊"', '"A\\u0000A"']);
});

test('Base64.encode', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

for the other FFI bindings, we test that they are only available in the start compartment.

When adding Base64, we should probably refactor those tests to just handle a list of things.

packages/xsnap/test/test-xs-js.js Show resolved Hide resolved
packages/xsnap/test/test-xs-js.js Show resolved Hide resolved
@dckc dckc changed the title feat(xsnap): Basee64 C bindings' feat(xsnap): Base64 C bindings' Sep 11, 2021
@kriskowal kriskowal force-pushed the 2682-kris-base64 branch 5 times, most recently from b057b0d to 5d00dd3 Compare September 12, 2021 00:07
@dckc
Copy link
Member

dckc commented Sep 12, 2021

nit: for the 2 submodule updates, I suggest build rather than feat, following cosmos-sdk conventional-commit-types where build is defined as "Changes that affect ... external dependencies".

Only "Add Base64 C bindings" is a feat that I would expect to see in the xsnap release notes.

@kriskowal kriskowal force-pushed the 2682-kris-base64 branch 2 times, most recently from fb97ee1 to 5d242b6 Compare September 13, 2021 18:43
@kriskowal kriskowal force-pushed the 2682-kris-base64 branch 2 times, most recently from 9a4e51f to 492c3e5 Compare September 13, 2021 21:08
@kriskowal kriskowal merged commit 1b93efe into master Sep 13, 2021
@kriskowal kriskowal deleted the 2682-kris-base64 branch September 13, 2021 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related issues xsnap the XS execution tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants