feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on obj literal short-hand methods #2206

leotm · 2024-04-10T16:42:52Z

closes: #XXXX
refs: #1891

Description

Fix SES compat with Hermes when calling addIntrinsics(tameSymbolConstructor());

SES lockdown Hermes compat tracking issue #1891

Follow-up to

feat(ses): tolerate omitted species #2108

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
Updates NEWS.md for user-facing changes.

mhofman

I'm confused as to what the problem is this is trying to solve?

How is the just created SharedSymbol function born with own non-configurable properties, conflicting with the original Symbol properties?

leotm · 2024-04-10T17:04:13Z

packages/ses/src/tame-symbol-constructor.js

@@ -50,14 +50,20 @@ export const tameSymbolConstructor = () => {
    },
  });

+  const sharedSymbolDescs = getOwnPropertyDescriptors(SharedSymbol);
  const originalDescsEntries = entries(
    getOwnPropertyDescriptors(OriginalSymbol),
  );
  const descs = fromEntries(


consider adding a comment like
// Hermes has many non-configurable Symbol properties. We should support such platforms.

The JS spec says that well known symbols are non-configurable properties of the Symbol global. How is hermes different exactly?

oh right i see your point, the differences listed are

Supported

Symbols (including most well-known Symbols)

In Progress

Symbol.prototype.description (it's not fully spec-conformant yet. Symbol().description should be undefined but it's currently '').

Excluded From Support

Symbol.species and its interactions with JS library functions

Symbol.unscopables (Hermes does not support with)

source: https://github.com/facebook/hermes/blob/main/doc/Features.md#excluded-from-support

and what i've discovered from logging originalDescsEntries and Hermes Debugger in #2206 (comment)

looking further at the source

... /// Represents a JS Symbol (es6). Movable, not copyable. /// TODO T40778724: this is a limited implementation sufficient for /// the debugger not to crash when a Symbol is a property in an Object /// or element in an array. Complete support for creating will come /// later. class JSI_EXPORT Symbol : public Pointer { ...

https://github.com/facebook/hermes/blob/main/API/jsi/jsi/jsi.h#L515-L540

https://github.com/facebook/hermes/blob/main/include/hermes/VM/PrimitiveBox.h#L310-L347

leotm · 2024-04-10T17:32:20Z

I'm confused as to what the problem is this is trying to solve?

How is the just created SharedSymbol function born with own non-configurable properties, conflicting with the original Symbol properties?

it's solving SES compatibility for React Native running on Hermes ^ when calling addIntrinsics(tameSymbolConstructor()); in the shim

otherwise the output on Android emulators is property is not configurable, no stack

Android emu > screenshot (older ses)
Android emu > screenshot 2 ([email protected])

after calling defineProperties(SharedSymbol, descs);

the original originalDescsEntries on Hermes logged out are

 LOG  length {"configurable": true, "enumerable": false, "value": 0, "writable": false}
 LOG  name {"configurable": true, "enumerable": false, "value": "Symbol", "writable": false}
 LOG  for {"configurable": true, "enumerable": false, "value": [Function for], "writable": true}
 LOG  keyFor {"configurable": true, "enumerable": false, "value": [Function keyFor], "writable": true}
 LOG  hasInstance {"configurable": false, "enumerable": false, "value": Symbol(Symbol.hasInstance), "writable": false}
 LOG  iterator {"configurable": false, "enumerable": false, "value": Symbol(Symbol.iterator), "writable": false}
 LOG  isConcatSpreadable {"configurable": false, "enumerable": false, "value": Symbol(Symbol.isConcatSpreadable), "writable": false}
 LOG  toPrimitive {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toPrimitive), "writable": false}
 LOG  toStringTag {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toStringTag), "writable": false}
 LOG  match {"configurable": false, "enumerable": false, "value": Symbol(Symbol.match), "writable": false}
 LOG  matchAll {"configurable": false, "enumerable": false, "value": Symbol(Symbol.matchAll), "writable": false}
 LOG  search {"configurable": false, "enumerable": false, "value": Symbol(Symbol.search), "writable": false}
 LOG  replace {"configurable": false, "enumerable": false, "value": Symbol(Symbol.replace), "writable": false}
 LOG  split {"configurable": false, "enumerable": false, "value": Symbol(Symbol.split), "writable": false}

so the conflict seems to be attempting to define 11 non-configurable properties to configurable

mhofman · 2024-04-10T18:07:19Z

otherwise the output on Android emulators is property is not configurable, no stack

Android emu > screenshot (older ses)

Android emu > screenshot 2 ([email protected])

after calling defineProperties(SharedSymbol, descs);

Flipper > Hermes Debugger > screenshot

Flipper > Hermes Debugger > screenshot 2

the original originalDescsEntries on Hermes logged out are

None of this tells me which non-configurable property exists on SharedSymbol that cannot be redefined. What is the actual trigger?

To be more clear, what are the own property descriptors of SharedSymbol right after construction?

leotm · 2024-04-10T19:07:08Z

otherwise the output on Android emulators is property is not configurable, no stack

Android emu > screenshot (older ses)

Android emu > screenshot 2 ([email protected])

after calling defineProperties(SharedSymbol, descs);

Flipper > Hermes Debugger > screenshot

Flipper > Hermes Debugger > screenshot 2

the original originalDescsEntries on Hermes logged out are

None of this tells me which non-configurable property exists on SharedSymbol that cannot be redefined. What is the actual trigger?

To be more clear, what are the own property descriptors of SharedSymbol right after construction?

getOwnPropertyDescriptors(SharedSymbol) appears to conform to the JS spec

{
   "arguments":{
      "configurable":false,
      "enumerable":false,
      "get":[
         "Function anonymous"
      ],
      "set":[
         "Function anonymous"
      ]
   },
   "caller":{
      "configurable":false,
      "enumerable":false,
      "get":[
         "Function anonymous"
      ],
      "set":[
         "Function anonymous"
      ]
   },
   "length":{
      "configurable":true,
      "enumerable":false,
      "value":1,
      "writable":false
   },
   "name":{
      "configurable":true,
      "enumerable":false,
      "value":"Symbol",
      "writable":false
   },
   "prototype":{
      "configurable":false,
      "enumerable":false,
      "value":{},
      "writable":true
   }
}

logging

// ...
  const descs=  fromEntries(
    arrayMap(originalDescsEntries, ([name, desc])=>  {
      console.log(name, desc);
      console.log(name, sharedSymbolDescs[name]);
// ...

we get

length {"configurable": true, "enumerable": false, "value": 0, "writable": false}
length {"configurable": true, "enumerable": false, "value": 1, "writable": false}

name {"configurable": true, "enumerable": false, "value": "Symbol", "writable": false}
name {"configurable": true, "enumerable": false, "value": "Symbol", "writable": false}

prototype {"configurable": false, "enumerable": false, "value": {}, "writable": true}

for {"configurable": true, "enumerable": false, "value": [Function for], "writable": true}
for undefined

keyFor {"configurable": true, "enumerable": false, "value": [Function keyFor], "writable": true}
keyFor undefined

hasInstance {"configurable": false, "enumerable": false, "value": Symbol(Symbol.hasInstance), "writable": false}
hasInstance undefined

iterator {"configurable": false, "enumerable": false, "value": Symbol(Symbol.iterator), "writable": false}
iterator undefined

isConcatSpreadable {"configurable": false, "enumerable": false, "value": Symbol(Symbol.isConcatSpreadable), "writable": false}
isConcatSpreadable undefined

toPrimitive {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toPrimitive), "writable": false}
toPrimitive undefined

toStringTag {"configurable": false, "enumerable": false, "value": Symbol(Symbol.toStringTag), "writable": false}
toStringTag undefined

match {"configurable": false, "enumerable": false, "value": Symbol(Symbol.match), "writable": false}
match undefined

matchAll {"configurable": false, "enumerable": false, "value": Symbol(Symbol.matchAll), "writable": false}
matchAll undefined

search {"configurable": false, "enumerable": false, "value": Symbol(Symbol.search), "writable": false}
search undefined

replace {"configurable": false, "enumerable": false, "value": Symbol(Symbol.replace), "writable": false}
replace undefined

split {"configurable": false, "enumerable": false, "value": Symbol(Symbol.split), "writable": false}
split undefined

the issue appears to be the prototype

replacing the condition
if (sharedSymbolDescs[name] && sharedSymbolDescs[name].configurable === false) {
with
if (name === 'prototype') {
also works

leotm · 2024-04-10T19:09:34Z

packages/ses/src/tame-symbol-constructor.js

+        sharedSymbolDescs[name] &&
+        sharedSymbolDescs[name].configurable === false


nb: condition name === 'prototype' is also sufficient on Hermes

Alright it looks like the problem is a spec non compliance from Hermes. Short-hand methods are not supposed to have a .prototype property. This is actually a fairly big problem for Symbol taming, as it would break Object(Symbol()) instanceof Symbol

@erights any ideas on how to best handle this non-compliance? It will likely bite us in other places.

I've never run Hermes before. I'm on a Mac. What's the best way for me to reproduce this? Possible to reproduce under a debugger, as I saw in your screenshots?

Short-hand methods are not supposed to have a .prototype property.

Current hypothesis. The correct fix is likely to make SharedSymbol a function function rather than a concise method. This probably would have been somewhat better all along anyway, since the original Symbol does, in most ways, act more like a function function than a concise method.

since the https://github.com/facebook/hermes/releases/tag/v0.12.0 CLI is shy of couple yrs old
i requested to help get an updated version (being discussed with team)

so i tested the commit fix by building and running Hermes, remembering to git checkout the static_h branch after cloning (before building) on Mac (debug build was fine), which lgtm (expected/conformed behaviour)

nb: the CLI version displays 0.12.0, hopefully latest commit will be shown too for clarity soon

i'll get back to you @erights with repro's on RN 71+ and running them with Flipper and Hermes Debugger on Mac
debugging works well for me on RN 71, but not above versions (if the same for you i'll file an upstream issue)

will proceed with changes
perhaps with a comment like this?

The changes I suggested are actually an improvement over what's currently there, and should be kept regardless of Hermes.

updated ^ and tested working on RN 71 (containing Hermes bug) on Android
with SES shim built from current/suggested changes
(dist/ses.cjs generated from yarn clean && yarn build)

I've never run Hermes before. I'm on a Mac. What's the best way for me to reproduce this? Possible to reproduce under a debugger, as I saw in your screenshots?

i'll get back to you @erights with repro's on RN 71+ and running them with Flipper and Hermes Debugger on Mac debugging works well for me on RN 71, but not above versions (if the same for you i'll file an upstream issue)

@erights here's the repro with @mhofman's fix applied in a commit
https://github.com/leotm/RN07117SES

and i've added a ReadMe instructions to run it on Mac, debug with Flipper and repro the issues tracked in

SES lockdown Hermes compat tracking issue #1891

mhofman

We should bypass the Hermes bug, and increase spec fidelity by only doing the following:

  const SharedSymbol = functionBind(Symbol, undefined);

Where in common.js:

export const functionBind = uncurryThis(bind);

The above will maintain the behavior of the original Symbol "constructor", including its native throwing behavior. It works because the receiver is not used by the [[Call]] of the primordial %Symbol%.

The name property will be "fixed" by the later copy of own properties from OriginalSymbol, and the toString() will be censored like for the original. Special casing of own properties should no longer be necessary for Hermes.

endojs/endo#2206

mhofman

Looks good, but since I proposed the change, I'd like @erights to approve.

Also suggestion on the comment, since we don't currently use bind anywhere for this purpose.

packages/ses/src/tame-symbol-constructor.js

kriskowal

This also looks good to me and I defer to @erights for the final stamp.

erights · 2024-04-16T02:19:17Z

(rerunning failed CI jobs)

leotm · 2024-04-16T12:04:38Z

passing ^ looks good to approve then merge

erights

LGTM, thanks!

This reverts commit 22b2c17.

Co-authored-by: Mathieu Hofman <[email protected]>

leotm · 2024-05-07T10:09:10Z

sry @kriskowal looks like i added an extra mandatory approval required by you ^
i don't have merge auth so Zb's enabled auto-merge

mhofman · 2024-05-07T18:46:09Z

I am honestly unsure what the unmet merge requirement is. Will update the branch just in case

- includes endojs/endo#2206 - includes #8786 `curl -O https://npmfs.com/download/ses/1.5.0/dist/ses.cjs`

## **Description** - bump SES to [v1.5.0](https://github.com/endojs/endo/releases/tag/ses%401.5.0) - includes Hermes fix in preparation for Hermes SES shim ## **Related issues** - includes endojs/endo#2206 - includes #8786 ## **Manual testing steps** Run: `curl -O https://npmfs.com/download/ses/1.5.0/dist/ses.cjs` Result: no change/diff ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. Co-authored-by: legobeat <[email protected]>

leotm mentioned this pull request Apr 10, 2024

SES lockdown Hermes compat tracking issue #1891

Open

30 tasks

leotm marked this pull request as ready for review April 10, 2024 17:00

mhofman reviewed Apr 10, 2024

View reviewed changes

leotm commented Apr 10, 2024

View reviewed changes

mhofman requested changes Apr 11, 2024

View reviewed changes

leotm changed the title ~~feat(ses): tolerate non-configurable Symbol props~~ feat(ses): tolerate prototype on object literal short-hand methods Apr 12, 2024

leotm changed the title ~~feat(ses): tolerate prototype on object literal short-hand methods~~ feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on obj literal short-hand methods Apr 15, 2024

leotm requested review from mhofman and erights April 15, 2024 10:47

leotm added a commit to leotm/RN07117SES that referenced this pull request Apr 15, 2024

Update ses (Hermes fix)

7c4c043

endojs/endo#2206

mhofman approved these changes Apr 15, 2024

View reviewed changes

packages/ses/src/tame-symbol-constructor.js Outdated Show resolved Hide resolved

kriskowal reviewed Apr 15, 2024

View reviewed changes

leotm requested a review from kriskowal April 16, 2024 12:03

erights approved these changes Apr 16, 2024

View reviewed changes

leotm and others added 6 commits April 30, 2024 11:39

feat(ses): tolerate non-configurable Symbol props

91fbaa9

Revert "feat(ses): tolerate non-configurable Symbol props"

8f260a6

This reverts commit 22b2c17.

fixup! feat(ses): add functionBind definition in commons.js

5d4e052

fixup! feat(ses): redefine SharedSymbol using functionBind

6c4827a

fixup! feat(ses): add comment

1079bdc

fixup! feat(ses): update comment

f74eee9

Co-authored-by: Mathieu Hofman <[email protected]>

naugtur force-pushed the tolerate-non-configurable-symbol-props branch from 587119c to f74eee9 Compare April 30, 2024 09:39

naugtur enabled auto-merge (rebase) April 30, 2024 09:40

naugtur disabled auto-merge April 30, 2024 09:40

naugtur enabled auto-merge (squash) April 30, 2024 09:42

mhofman disabled auto-merge May 7, 2024 18:45

Merge branch 'master' into tolerate-non-configurable-symbol-props

fd9d473

mhofman enabled auto-merge (squash) May 7, 2024 18:47

mhofman merged commit 59bb9ba into endojs:master May 7, 2024
17 checks passed

leotm added a commit to MetaMask/metamask-mobile that referenced this pull request May 15, 2024

chore: bump SES

2896113

- includes endojs/endo#2206 - includes #8786 `curl -O https://npmfs.com/download/ses/1.5.0/dist/ses.cjs`

leotm mentioned this pull request May 15, 2024

chore: bump SES MetaMask/metamask-mobile#9637

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on obj literal short-hand methods #2206

feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on obj literal short-hand methods #2206

leotm commented Apr 10, 2024 •

edited

Loading

mhofman left a comment

leotm Apr 10, 2024

mhofman Apr 10, 2024

leotm Apr 10, 2024

leotm Apr 10, 2024

leotm commented Apr 10, 2024

mhofman commented Apr 10, 2024 •

edited

Loading

leotm commented Apr 10, 2024

leotm Apr 10, 2024

mhofman Apr 10, 2024 •

edited

Loading

mhofman Apr 10, 2024

erights Apr 10, 2024

erights Apr 10, 2024

leotm Apr 12, 2024 •

edited

Loading

leotm Apr 12, 2024 •

edited

Loading

mhofman Apr 12, 2024

leotm Apr 15, 2024

leotm Apr 15, 2024

mhofman left a comment •

edited

Loading

mhofman left a comment

kriskowal left a comment

erights commented Apr 16, 2024

leotm commented Apr 16, 2024

erights left a comment

leotm commented May 7, 2024

mhofman commented May 7, 2024

		sharedSymbolDescs[name] &&
		sharedSymbolDescs[name].configurable === false

feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on obj literal short-hand methods #2206

feat(ses): redefine SharedSymbol to bypass Hermes prototype bug on obj literal short-hand methods #2206

Conversation

leotm commented Apr 10, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leotm commented Apr 10, 2024

mhofman commented Apr 10, 2024 • edited Loading

leotm commented Apr 10, 2024

Choose a reason for hiding this comment

mhofman Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leotm Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

leotm Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment • edited Loading

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

erights commented Apr 16, 2024

leotm commented Apr 16, 2024

erights left a comment

Choose a reason for hiding this comment

leotm commented May 7, 2024

mhofman commented May 7, 2024

leotm commented Apr 10, 2024 •

edited

Loading

mhofman commented Apr 10, 2024 •

edited

Loading

mhofman Apr 10, 2024 •

edited

Loading

leotm Apr 12, 2024 •

edited

Loading

leotm Apr 12, 2024 •

edited

Loading

mhofman left a comment •

edited

Loading