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

internal/process/per_thread.js is incompatible with Hardened JS shim #43496

Closed
michaelfig opened this issue Jun 19, 2022 · 1 comment · Fixed by #43907
Closed

internal/process/per_thread.js is incompatible with Hardened JS shim #43496

michaelfig opened this issue Jun 19, 2022 · 1 comment · Fixed by #43907

Comments

@michaelfig
Copy link

michaelfig commented Jun 19, 2022

Version

16.15.1

Platform

Darwin snow.local 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:37 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T6000 arm64

Subsystem

internal/process

What steps will reproduce the bug?

Unfortunately, I currently need to install third party deps to reproduce:

mkdir repro
cd repro
yarn init --yes
yarn add zx endo-exec
cd node_modules/endo-exec
node endo-exec.cjs ./scripts/zx-head.js /etc/passwd

How often does it reproduce? Is there a required condition?

It reproduces every time.

What is the expected behavior?

Hello, world (from ./scripts/zx-head.js)!
$ head -5 /etc/passwd
##
# User Database
# 
# Note that this file is consulted directly only when the system is running
# in single-user mode.  At other times this information is provided by
$

What do you see instead?

(TypeError#1)
TypeError#1: Cannot assign to read only property 'Symbol(Symbol.iterator)' of object '[object Set]'
  at Object.buildAllowedFlags (node:internal/process/per_thread:375:53)
  at process.get [as allowedNodeEnvironmentFlags] (node:internal/bootstrap/node:279:34)
  at get (<anonymous>)
  at getOwn (node:internal/bootstrap/loaders:182:5)
  at NativeModule.syncExports (node:internal/bootstrap/loaders:294:31)
  at ModuleWrap.<anonymous> (node:internal/bootstrap/loaders:274:22)
  at NativeModule.getESMFacade (node:internal/bootstrap/loaders:279:17)
  at NativeModule.compileForPublicLoader (node:internal/bootstrap/loaders:259:10)
  at loadNativeModule (node:internal/modules/cjs/helpers:49:9)
  at ESMLoader.builtinStrategy (node:internal/modules/esm/translators:258:18)
  at ESMLoader.moduleProvider (node:internal/modules/esm/loader:337:14)

Additional information

Here is a a fix that makes Node.js compatible with the prototype freezing done by the Endo SES shim:

diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js
index 709bcb7b13..6ddc88a555 100644
--- a/lib/internal/process/per_thread.js
+++ b/lib/internal/process/per_thread.js
@@ -15,6 +15,7 @@ const {
   Float64Array,
   NumberMAX_SAFE_INTEGER,
   ObjectFreeze,
+  ObjectDefineProperty,
   ReflectApply,
   RegExpPrototypeTest,
   SafeArrayIterator,
@@ -371,9 +372,11 @@ function buildAllowedFlags() {
       return SetPrototypeValues(this[kInternal].set);
     }
   }
-  NodeEnvironmentFlagsSet.prototype.keys =
-  NodeEnvironmentFlagsSet.prototype[SymbolIterator] =
-    NodeEnvironmentFlagsSet.prototype.values;
+
+  const flagSetValues = NodeEnvironmentFlagsSet.prototype.values;
+  // NodeEnvironmentFlagsSet.prototype[SymbolIterator] = flagSetValues;
+  ObjectDefineProperty(NodeEnvironmentFlagsSet.prototype, SymbolIterator, { value: flagSetValues });
+  ObjectDefineProperty(NodeEnvironmentFlagsSet.prototype, 'keys', { value: flagSetValues });
 
   ObjectFreeze(NodeEnvironmentFlagsSet.prototype.constructor);
   ObjectFreeze(NodeEnvironmentFlagsSet.prototype);
@aduh95
Copy link
Contributor

aduh95 commented Jun 20, 2022

Not sure I understand the use case here, an example that doesn't uses third party code would be 💯
Anyway, if you want such a change to be made you should open a PR with the suggested change and a test that fails on main and passes with your PR.

aduh95 pushed a commit that referenced this issue Jul 24, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit that referenced this issue Jul 26, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit that referenced this issue Jul 31, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit that referenced this issue Jul 31, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit that referenced this issue Aug 1, 2022
PR-URL: #43907
Fixes: #43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
PR-URL: nodejs#43907
Fixes: nodejs#43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
PR-URL: nodejs/node#43907
Fixes: nodejs/node#43496
Refs: endojs/endo#576
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants