fix(ses): completePrototypes on Hermes #2563
Conversation
leotm
force-pushed
the
ses-hermes-completePrototypes
branch
from
490dbed
to
ba9f9fd
Compare
|
draft pending testing changes from endo sync in VM and React Native |
|
we considered implementing endo/packages/ses/src/intrinsics.js Lines 103 to 110 in a13a879 instead in // packages/ses/src/tame-function-tostring.js
// ...
if (
!/^[A-Z]/.test(func.name) &&
func.prototype !== undefined
) {
func.prototype = undefined;
}
// ...but at this point it's too late, so went with the current approach |
|
https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563 Run npx playwright install --with-deps
npx playwright install --with-deps
shell: /usr/bin/bash -e {0}
Installing dependencies...
Switching to root user to install dependencies...
Hit:1 http://azure.archive.ubuntu.com/ubuntu noble InRelease
Get:[2](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:2) http://azure.archive.ubuntu.com/ubuntu noble-updates InRelease [126 kB]
Get:[3](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:3) http://azure.archive.ubuntu.com/ubuntu noble-backports InRelease [126 kB]
Get:[4](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:5) http://azure.archive.ubuntu.com/ubuntu noble-security InRelease [126 kB]
Hit:[5](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:6) https://packages.microsoft.com/repos/azure-cli noble InRelease
Get:[6](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:7) https://packages.microsoft.com/ubuntu/24.04/prod noble InRelease [3600 B]
Get:7 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 Packages [59[7](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:8) kB]
Get:8 http://azure.archive.ubuntu.com/ubuntu noble-updates/main Translation-en [146 kB]
Get:9 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 c-n-f Metadata [10.3 kB]
Get:10 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 Packages [706 kB]
Get:11 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe Translation-en [209 kB]
Get:12 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 c-n-f Metadata [19.[8](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:9) kB]
Get:13 http://azure.archive.ubuntu.com/ubuntu noble-updates/restricted amd64 Packages [388 kB]
Get:14 http://azure.archive.ubuntu.com/ubuntu noble-updates/restricted Translation-en [74.8 kB]
Get:15 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse amd64 Packages [14.8 kB]
Get:16 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse Translation-en [3820 B]
Get:17 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse amd64 c-n-f Metadata [552 B]
Get:18 http://azure.archive.ubuntu.com/ubuntu noble-backports/universe amd64 Packages [10.6 kB]
Get:1[9](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:10) http://azure.archive.ubuntu.com/ubuntu noble-backports/universe Translation-en [10.8 kB]
Get:20 http://azure.archive.ubuntu.com/ubuntu noble-backports/universe amd64 c-n-f Metadata [1[10](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:11)4 B]
Get:21 http://azure.archive.ubuntu.com/ubuntu noble-security/main amd64 Packages [410 kB]
Get:22 http://azure.archive.ubuntu.com/ubuntu noble-security/main Translation-en [90.4 kB]
Get:23 http://azure.archive.ubuntu.com/ubuntu noble-security/main amd64 c-n-f Metadata [5788 B]
Get:24 http://azure.archive.ubuntu.com/ubuntu noble-security/universe amd64 Packages [553 kB]
Get:25 http://azure.archive.ubuntu.com/ubuntu noble-security/universe Translation-en [147 kB]
Get:26 http://azure.archive.ubuntu.com/ubuntu noble-security/universe amd64 c-n-f Metadata [13.5 kB]
Get:27 https://packages.microsoft.com/ubuntu/24.04/prod noble/main armhf Packages [5057 B]
Get:28 https://packages.microsoft.com/ubuntu/24.04/prod noble/main amd64 Packages [12.2 kB]
Get:29 https://packages.microsoft.com/ubuntu/24.04/prod noble/main arm64 Packages [8451 B]
Fetched 3820 kB in 1s (7439 kB/s)
Reading package lists...
Reading package lists...
Building dependency tree...
Reading state information...
Package libicu70 is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source
Package libasound2 is a virtual package provided by:
liboss4-salsa-asound2 4.2-build2020-1ubuntu3
libasound2t64 1.2.[11](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:12)-1build2 (= 1.2.11-1build2)
E: Package 'libasound2' has no installation candidate
E: Package 'libicu70' has no installation candidate
E: Unable to locate package libffi7
E: Unable to locate package libx264-163
Failed to install browsers
Error: Installation process exited with code: 100
Error: Process completed with exit code 1.flakey CI, re-run edit: fail persisting after re-run and happening elsewhere #2583 (comment)
|
leotm
force-pushed
the
ses-hermes-completePrototypes
branch
from
a13a879
to
2b0b560
Compare
|
Another thought: concise functions in Hermes may incorrectly have a $ eshost -se '[()=>{}, (()=>{}).bind()].map(fn => Object.hasOwnProperty.call(fn, "prototype"))'
#### engine262, GraalJS, JavaScriptCore, Moddable XS, QuickJS, SpiderMonkey, V8
false,false
#### Hermes
true,falseSo any receiver-insensitive function that needs be presented as non-newable (which I think describes all functions that we want to appear native) can be wrapped with |
@leotm I believe this is because GitHub runner images switched from This should fix it: |
good point ^ same with normal functions # Hermes release version: 0.12.0, HBC bytecode version: 89
>> [function(){}, (function(){}).bind()].map(fn => Object.hasOwnProperty.call(fn, "prototype"))
[ true, false, [length]: 2 ]so binding the intrinsic works too after restoring the permits |
packages/ses/src/intrinsics.js
Outdated
| if (!namePrototype) { | ||
| const permitPrototype = permit.prototype; | ||
|
|
||
| // Bypass Hermes bug, fixed in: https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599 |
There was a problem hiding this comment.
Please explain the Hermes bug you're bypassing below, in addition to the commit link.
There was a problem hiding this comment.
TODO: add explanation pending investigation how lockdown / harden / %InitialGetStackString% are defined in SES
- if (object literal) concise method, commit link is correct
- likely
harden - likely
%InitialGetStackString%
- likely
- if concise (arrow) function, facebook/hermes@c42491d
- likely
lockdown
- likely
There was a problem hiding this comment.
just clarifying the above (how these three are defined in SES)
- lockdown: arrow fn
- harden: arrow fn
- getStackString: (object literal) concise (shorthand) method
endo/packages/ses/src/lockdown-shim.js
Lines 15 to 18 in 4ca4028
endo/packages/ses/src/lockdown.js
Line 408 in 4ca4028
endo/packages/ses/src/error/tame-error-constructor.js
Lines 21 to 23 in 4ca4028
so added commit earlier updating documentation in permits.js and intrinsics.js 575cd60
packages/ses/src/permits.js
Outdated
| @@ -281,6 +282,7 @@ export const AsyncFunctionInstance = { | |||
|
|
|||
| // Aliases | |||
| const fn = FunctionInstance; | |||
| const hermesFn = { ...FunctionInstance, prototype: 'undefined' }; // Bypass Hermes bug, fixed in: https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599 | |||
There was a problem hiding this comment.
Likewise. Is there an issue for this Hermes bug?
There was a problem hiding this comment.
for concise (arrow) functions
- no issue was raised
- facebook/hermes@c42491d fixed in Hermes v0.13.0
but yes for (object literal) concise (shorthand) methods
- Short-hand methods don't have prototype / construct facebook/hermes#1371
- facebook/hermes@00f18c8 fixed in Static Hermes (unreleased)
| lockdown: hermesFn, | ||
| harden: { ...hermesFn, isFake: 'boolean' }, | ||
|
|
||
| '%InitialGetStackString%': fn, | ||
| '%InitialGetStackString%': hermesFn, |
There was a problem hiding this comment.
it's these three intrinsics ^ that throw at
endo/packages/ses/src/intrinsics.js
Lines 101 to 104 in 7a991aa
when calling lockdown() on Hermes VM (surfaced in React Native)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I'm confused. Why doesn't #1221 make this a non-issue, setting the .prototype to undefined itself?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
But AFAICT
lockdownis 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: undefinedon 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)
There was a problem hiding this comment.
I'm confused. Why doesn't #1221 make this a non-issue, setting the
.prototypetoundefineditself?
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.prototypethrow TypeError(`${name}.prototype property not whitelisted`);
- permit-intrinsics.js > whitelistIntrinsics
prop in objtypeof obj === 'function' && prop === 'prototype'obj.prototype = undefined(not reached)- fix: tolerate empty func.prototype #1221
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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%
packages/ses/src/permits.js
Outdated
| @@ -270,6 +270,7 @@ 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. | |||
| // The exception to this is Hermes, fixed since in Static Hermes. | |||
There was a problem hiding this comment.
I don't know what "Static Hermes" is. add reference to comment?
There was a problem hiding this comment.
@leotm Are we testing against Hermes in CI somewhere? Can you point me to it?
both proposed 2 changes (permits.js, intrinsics.js) can be verified working against Hermes CI in
https://github.com/endojs/endo/tree/ses-hermes-whitelistIntrinsics-repro
running yarn build:hermes && yarn test:hermes where completePrototypes is now fixed on Hermes
otherwise final changes to #2334 nearly ready for official Hermes CI feedback
leotm
force-pushed
the
ses-hermes-completePrototypes
branch
from
575cd60
to
048efd3
Compare
all questions answered and addressed ^ in comments and commits |
Closes: #2598 Refs: #2563 #2334 #1221 ## Description #1221 was supposed to make ses tolerate undeletable `func.prototype` properties that should be absent, so long as they could be set to `undefined` instead, making them harmless. This tolerance came with a warning to flag the remaining non-conformance. However #2598 explains why #1221 sometimes fails to do this. #1221 did come with a test, but it fell into the case where #1221 works, which is a non-toplevel function. #2563 (and #2334 ?) fell into the trap explained by #2598 and untested by #1221, which is an undeletable `func.prototype` on a top-level instrinsic. As a result, #2563 currently contains a workaround for #2598 which this PR would make unnecessary. This PR fixes the problem by factoring out the `func.prototype`-tolerant property deletion into a separate `cauterizeProperty` function which it calls from both places. This PR also adds the test that was missing from #1221 , having first checked that the test detects #2598 when run without the rest of this PR. If this PR gets merged before #2563, then #2563's workaround for #2598 can first be removed before it is merged. - [ ] TODO should pass a genuine reporter in to all calls to `cauterizeProperty`. @kriskowal , please advise how intrinsics.js should arrange to do so. ### Security Considerations Allowing a `func.prototype` property that really shouldn't be there seems safe, so long as it is safely set to `undefined` first, which this PR does, and then checks that it has done so. ### Scaling Considerations none ### Documentation Considerations generally, this would be one less thing to worry about, and thus one less thing that needs to be documented for most users. ### Testing Considerations Adds the test that was missing from #1221 that let #2598 go unnoticed until #2563 ### Compatibility Considerations Should be none. ### Upgrade Considerations Should be none.
| if ( | ||
| typeof intrinsic === 'function' && | ||
| intrinsic.prototype !== undefined && | ||
| permitPrototype === 'undefined' // permits.js |
There was a problem hiding this comment.
it's literally the string undefined?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But, IIUC, given #2624 , none of this may still be necessary, which is why I ask.
There was a problem hiding this comment.
Or the possibility of just forcing these functions to have no prototype property even in Hermes.
Description
Calling lockdown on Hermes, in the VM or React Native runtime, we reach
endo/packages/ses/src/intrinsics.js
Lines 101 to 104 in 7b8c677
checking the
lockdownintrinsic descriptor{"length":{"value":1,"writable":false,"enumerable":false,"configurable":true},"name":{"value":"","writable":false,"enumerable":false,"configurable":true},"caller":{"enumerable":false,"configurable":false},"arguments":{"enumerable":false,"configurable":false},"prototype":{"value":{},"writable":true,"enumerable":false,"configurable":false}}we see it's because the
prototypeis non-standardendo/packages/ses/src/permits.js
Line 1644 in 7b8c677
endo/packages/ses/src/permits.js
Lines 282 to 283 in 7b8c677
endo/packages/ses/src/permits.js
Lines 265 to 273 in 7b8c677
so we should be terminating the current loop iteration instead of throwing the error, ideally at
endo/packages/ses/src/intrinsics.js
Lines 93 to 96 in 7b8c677
the initial approach fixed our 3 problematic intrinsics on Hermes
by terminating the current loop iteration earlier
before throwing the error as intended
i.e. skip completing prototypes that shouldn't be there
Repro branch including proposed fix and detailed Hermes VM print output (no Babel)
yarn clean && yarn build:hermes && test:hermesTODO: check v0.13.0 for RN0.75.x ✅ partially fixed:
lockdownintrinsic onlywe can easily test this in #2334 by downloading the .tar.gz locally then updating both binary paths
nb:
endo/packages/ses/src/permits.js
Lines 1644 to 1647 in 7b8c677
TODO: build Static Hermes with cmake and ninja then test (i.e. is it fully fixed upstream?)
yes ✅ object literals short hand method
the current proposed change accounts for Hermes' non-standard fn instance prototype (fixed in Static Hermes), by setting it to
undefinedwhen completing the prototypesanother approach: #2563 (comment)
Security Considerations
Scaling Considerations
Documentation Considerations
lockdownis fixed on v0.13.0 for RN0.75.x, so this part of the condition can be sunset when we want to drop support for v0.12.0Testing Considerations
Compatibility Considerations
Considered alternative solution
which accounts for
{ ... "prototype":{"value":{},"writable":true,"enumerable":false,"configurable":false} }but this breaks building the ses bundle when calling
whitelistIntrinsicsaftercompletePrototypeswhich appears to be a regression, so no bueno
(See also Documentation Considerations RE backward compatibility)
Upgrade Considerations
(See also Documentation Considerations RE backward compatibility)