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.

Sign up for GitHub

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

Jump to bottom

fix(ses): completePrototypes on Hermes #2563

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+38 −12
Open

fix(ses): completePrototypes on Hermes #2563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d4bd35
fix(ses): expect fn instance prototype on Hermes in permit
leotm Oct 28, 2024
048efd3
fix(ses): set fn instance prototype to undefined on Hermes
leotm Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 24 additions & 9 deletions packages/ses/src/intrinsics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,25 +98,40 @@ export const makeIntrinsicsCollector = () => {
if (typeof permit !== 'object') {
throw TypeError(`Expected permit object at whitelist.${name}`);
}
const namePrototype = permit.prototype;
if (!namePrototype) {
const permitPrototype = permit.prototype;

if (
typeof intrinsic === 'function' &&
intrinsic.prototype !== undefined &&
permitPrototype === 'undefined' // permits.js
Copy link
Contributor

Choose a reason for hiding this comment

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

it's literally the string undefined?

Copy link
Contributor

@erights erights Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. When a permit is a string which is a typeof name, it means "this field must be a value whose typeof is that string. The only value v for which typeof v === 'undefined' is undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, IIUC, given #2624 , none of this may still be necessary, which is why I ask.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or the possibility of just forcing these functions to have no prototype property even in Hermes.

) {
// Set non-standard `.prototype` properties to `undefined` on Hermes.
// These include intrinsics that are additional properties of the global object,
// proposed by SES defined as function instances
// - arrow functions: lockdown, harden
// - concise methods: %InitialGetStackString%
intrinsic.prototype = undefined;
}

const intrinsicPrototype = intrinsic.prototype;

if (!permitPrototype) {
throw TypeError(`${name}.prototype property not whitelisted`);
}
if (
typeof namePrototype !== 'string' ||
!objectHasOwnProperty(permitted, namePrototype)
typeof permitPrototype !== 'string' ||
!objectHasOwnProperty(permitted, permitPrototype)
) {
throw TypeError(`Unrecognized ${name}.prototype whitelist entry`);
}
const intrinsicPrototype = intrinsic.prototype;
if (objectHasOwnProperty(intrinsics, namePrototype)) {
if (intrinsics[namePrototype] !== intrinsicPrototype) {
throw TypeError(`Conflicting bindings of ${namePrototype}`);
if (objectHasOwnProperty(intrinsics, permitPrototype)) {
if (intrinsics[permitPrototype] !== intrinsicPrototype) {
throw TypeError(`Conflicting bindings of ${permitPrototype}`);
}
// eslint-disable-next-line no-continue
continue;
}
intrinsics[namePrototype] = intrinsicPrototype;
intrinsics[permitPrototype] = intrinsicPrototype;
}
};
freeze(completePrototypes);
Expand Down
17 changes: 14 additions & 3 deletions packages/ses/src/permits.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,9 @@ export const FunctionInstance = {
// Do not specify "prototype" here, since only Function instances that can
// be used as a constructor have a prototype property. For constructors,
// since prototype properties are instance-specific, we define it there.
// Older versions of Hermes contain a non-standard prototype, noted below in 'hermesFn'.
// This is fixed fully in Static Hermes: https://github.com/facebook/hermes/tree/static_h
// See: https://speakerdeck.com/tmikov2023/optimizing-with-static-hermes-chain-react-2024
};

// AsyncFunction Instances
Expand All @@ -281,6 +284,14 @@ export const AsyncFunctionInstance = {

// Aliases
const fn = FunctionInstance;
// Bypass Hermes bugs, fixed in:
// - https://github.com/facebook/hermes/commit/c42491de94aff479e5e83c073eff96a6261da080 (hermes: v0.13.0)
// - https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599 (sh_stable, static_h)
// Expect Additional Properties of the Global Object (Annex B) proposed by SES,
// that are function instances defined as arrow functions ('lockdown' and 'harden')
// and concise methods ('%InitialGetStackString%'), to have their non-standard
// prototype properties set to `undefined` in intrinsics.js when completing prototypes.
const hermesFn = { ...FunctionInstance, prototype: 'undefined' };
const asyncFn = AsyncFunctionInstance;

const getter = {
Expand Down Expand Up @@ -1643,8 +1654,8 @@ export const permitted = {
'@@toStringTag': 'string',
},

lockdown: fn,
harden: { ...fn, isFake: 'boolean' },
lockdown: hermesFn,
harden: { ...hermesFn, isFake: 'boolean' },

'%InitialGetStackString%': fn,
'%InitialGetStackString%': hermesFn,
Comment on lines +1657 to +1660
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exactly these three?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's these three intrinsics ^ that throw at

endo/packages/ses/src/intrinsics.js

Lines 101 to 104 in 7a991aa

const namePrototype = permit.prototype;
if (!namePrototype) {
throw TypeError(`${name}.prototype property not whitelisted`);
}

when calling lockdown() on Hermes VM (surfaced in React Native)

Copy link
Contributor Author

@leotm leotm Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

on Hermes v0.12.0 it's all three ^
on v0.13.0 for RN0.75.x only harden and %InitialGetStackString%

otherwise on Static Hermes (unreleased) none of these are an issue
which atm can only be tested by building and running either sh_stable or static_h

after #2334 lands, adding v0.13.0 to CI is up next (it's not on npm, so will need to wget the .tar.gz)
followed by Static Hermes after (no distro yet so in CI will need to build and run from scratch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the initial approach was minimal and to ignore these three, but it doesn't remove them

the proposed change expects undefined and sets undefined which seems better

or calling .bind() on these three instead which also works (repro)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. Why doesn't #1221 make this a non-issue, setting the .prototype to undefined itself?

Copy link
Contributor Author

@leotm leotm Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

side note: looks like all of the hermes releases have been published to GitHub now?! (but still not npmjs, if planned)

but it remains there's no need for SES to support these older versions
since metamask-mobile is now on React Native 0.72.15
whereas Hermes v0.11.0 for RN0.68.x has been unsupported for years

Copy link
Contributor Author

@leotm leotm Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But AFAICT lockdown is defined as an arrow function. So why is it more problematic than a zillion other arrow functions? Or, perhaps, is it the only intrinsic defined by us as an arrow function?

this part's been puzzling me too...
why is completing the lockdown prototype a problem on Hermes v0.12.0 (and older)...
but not on Hermes v0.13.0+ 🤔
(and the a zillion other arrow functions)

If all this is true, then it makes sense to permit prototype: undefined on these three. So I'll approve on that basis. But before merging, please let us know if this is indeed all true.

sure ^ i'll keep digging to find the answer to these last couple questions (and #1221)

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'm confused. Why doesn't #1221 make this a non-issue, setting the .prototype to undefined itself?

Is it because the test in intrinsics.js is inconsistently stricter than the #1221 test in permit-intrinsics.js and runs first?

exactly the current order and strictness on master is

  • intrinsics.js > completePrototypes
    • isObject(intrinsic)
    • objectHasOwnProperty(intrinsic, 'prototype')
    • typeof permit === 'object'
    • !permit.prototype
      • throw TypeError(`${name}.prototype property not whitelisted`);
  • permit-intrinsics.js > whitelistIntrinsics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separately, it looks like a bug that

the test in intrinsics.js is inconsistently stricter than the #1221 test in permit-intrinsics.js and runs first

Could you file an issue on that and assign it to me? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So why is it more problematic than a zillion other arrow functions?

for example checking: permits.js > Appendix B > Annex B

endo/packages/ses/src/permits.js

Lines 1613 to 1650 in 4ca4028

// Appendix B
// Annex B: Additional Properties of the Global Object
escape: fn,
unescape: fn,
// Proposed
'%UniqueCompartment%': {
'[[Proto]]': '%FunctionPrototype%',
prototype: '%CompartmentPrototype%',
toString: fn,
},
'%InertCompartment%': {
'[[Proto]]': '%FunctionPrototype%',
prototype: '%CompartmentPrototype%',
toString: fn,
},
'%CompartmentPrototype%': {
constructor: '%InertCompartment%',
evaluate: fn,
globalThis: getter,
name: getter,
import: asyncFn,
load: asyncFn,
importNow: fn,
module: fn,
'@@toStringTag': 'string',
},
lockdown: fn,
harden: { ...fn, isFake: 'boolean' },
'%InitialGetStackString%': fn,
};

these aren't problematic on Hermes
since they're not proposed by SES
so already implemented correctly in Hermes natively:

function escape() { [native code] }
function unescape() { [native code] }

however our three functions proposed by SES are problematic
since we are adding them to Hermes as buggy arrow functions and concise methods:

function (a0) { [bytecode] } // lockdown
function harden(a0) { [bytecode] } // harden
function getStackString(a0) { [bytecode] } // intrinsic: %InitialGetStackString%

};
Loading