endo icon indicating copy to clipboard operation
endo copied to clipboard
verified publisher icon endojs

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

Open leotm opened this issue 1 year ago • 5 comments

closes: #XXXX refs: https://github.com/endojs/endo/issues/1891

Description

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

  • https://github.com/endojs/endo/issues/1891

Follow-up to

  • https://github.com/endojs/endo/pull/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.

leotm avatar Apr 10 '24 16:04 leotm

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

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

leotm avatar Apr 10 '24 17:04 leotm

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

after calling defineProperties(SharedSymbol, descs);

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?

mhofman avatar Apr 10 '24 18:04 mhofman

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

after calling defineProperties(SharedSymbol, descs);

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 avatar Apr 10 '24 19:04 leotm

(rerunning failed CI jobs)

erights avatar Apr 16 '24 02:04 erights

passing ^ looks good to approve then merge

leotm avatar Apr 16 '24 12:04 leotm

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

leotm avatar May 07 '24 10:05 leotm

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

mhofman avatar May 07 '24 18:05 mhofman