From d4e2a4ea0b64e2a11d51e6927e9a9acb16818bca Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Fri, 16 Jun 2023 18:27:45 -0400
Subject: [PATCH 1/9] lib: merge cjs and esm package json reader caches

---
 lib/internal/modules/cjs/loader.js          |  54 +++-------
 lib/internal/modules/esm/package_config.js  | 106 ++------------------
 lib/internal/modules/esm/resolve.js         |  10 +-
 lib/internal/modules/package_json_reader.js |  90 +++++++++++++++--
 src/node_file.cc                            |  46 ++-------
 test/fixtures/errors/force_colors.snapshot  |   8 +-
 test/parallel/test-module-binding.js        |  28 +-----
 7 files changed, 125 insertions(+), 217 deletions(-)

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 508123c6e306ff..d32d7ebf92c438 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -82,7 +82,6 @@ const {
   pendingDeprecate,
   emitExperimentalWarning,
   kEmptyObject,
-  filterOwnProperties,
   setOwnProperty,
   getLazy,
 } = require('internal/util');
@@ -355,36 +354,12 @@ function initializeCJS() {
 //   -> a.<ext>
 //   -> a/index.<ext>
 
-const packageJsonCache = new SafeMap();
-
+/**
+ * @param {string} requestPath
+ * @return {PackageConfig}
+ */
 function readPackage(requestPath) {
-  const jsonPath = path.resolve(requestPath, 'package.json');
-
-  const existing = packageJsonCache.get(jsonPath);
-  if (existing !== undefined) return existing;
-
-  const result = packageJsonReader.read(jsonPath);
-  const json = result.containsKeys === false ? '{}' : result.string;
-  if (json === undefined) {
-    packageJsonCache.set(jsonPath, false);
-    return false;
-  }
-
-  try {
-    const filtered = filterOwnProperties(JSONParse(json), [
-      'name',
-      'main',
-      'exports',
-      'imports',
-      'type',
-    ]);
-    packageJsonCache.set(jsonPath, filtered);
-    return filtered;
-  } catch (e) {
-    e.path = jsonPath;
-    e.message = 'Error parsing ' + jsonPath + ': ' + e.message;
-    throw e;
-  }
+  return packageJsonReader.read(path.resolve(requestPath, 'package.json'));
 }
 
 let _readPackage = readPackage;
@@ -414,7 +389,7 @@ function readPackageScope(checkPath) {
     if (StringPrototypeEndsWith(checkPath, sep + 'node_modules'))
       return false;
     const pjson = _readPackage(checkPath + sep);
-    if (pjson) return {
+    if (pjson.exists) return {
       data: pjson,
       path: checkPath,
     };
@@ -423,7 +398,7 @@ function readPackageScope(checkPath) {
 }
 
 function tryPackage(requestPath, exts, isMain, originalPath) {
-  const pkg = _readPackage(requestPath)?.main;
+  const pkg = _readPackage(requestPath).main;
 
   if (!pkg) {
     return tryExtensions(path.resolve(requestPath, 'index'), exts, isMain);
@@ -528,9 +503,10 @@ function trySelfParentPath(parent) {
 function trySelf(parentPath, request) {
   if (!parentPath) return false;
 
-  const { data: pkg, path: pkgPath } = readPackageScope(parentPath) || {};
-  if (!pkg || pkg.exports === undefined) return false;
-  if (typeof pkg.name !== 'string') return false;
+  const { data: pkg, path: pkgPath } = readPackageScope(parentPath);
+  if (!pkg || pkg.exports == null || pkg.name === undefined) {
+    return false;
+  }
 
   let expansion;
   if (request === pkg.name) {
@@ -565,7 +541,7 @@ function resolveExports(nmPath, request) {
     return;
   const pkgPath = path.resolve(nmPath, name);
   const pkg = _readPackage(pkgPath);
-  if (pkg?.exports != null) {
+  if (pkg.exists && pkg.exports != null) {
     try {
       const { packageExportsResolve } = require('internal/modules/esm/resolve');
       return finalizeEsmResolution(packageExportsResolve(
@@ -1028,7 +1004,7 @@ Module._resolveFilename = function(request, parent, isMain, options) {
 
   if (request[0] === '#' && (parent?.filename || parent?.id === '<repl>')) {
     const parentPath = parent?.filename ?? process.cwd() + path.sep;
-    const pkg = readPackageScope(parentPath) || {};
+    const pkg = readPackageScope(parentPath) || { __proto__: null };
     if (pkg.data?.imports != null) {
       try {
         const { packageImportsResolve } = require('internal/modules/esm/resolve');
@@ -1274,9 +1250,9 @@ Module._extensions['.js'] = function(module, filename) {
     content = fs.readFileSync(filename, 'utf8');
   }
   if (StringPrototypeEndsWith(filename, '.js')) {
-    const pkg = readPackageScope(filename);
+    const pkg = readPackageScope(filename) || { __proto__: null };
     // Function require shouldn't be used in ES modules.
-    if (pkg?.data?.type === 'module') {
+    if (pkg.data?.type === 'module') {
       const parent = moduleParentCache.get(module);
       const parentPath = parent?.filename;
       const packageJsonPath = path.resolve(pkg.path, 'package.json');
diff --git a/lib/internal/modules/esm/package_config.js b/lib/internal/modules/esm/package_config.js
index dc3c37f6042333..4ca701d4810f74 100644
--- a/lib/internal/modules/esm/package_config.js
+++ b/lib/internal/modules/esm/package_config.js
@@ -1,102 +1,10 @@
 'use strict';
 
 const {
-  JSONParse,
-  ObjectPrototypeHasOwnProperty,
-  SafeMap,
   StringPrototypeEndsWith,
 } = primordials;
 const { URL, fileURLToPath } = require('internal/url');
-const {
-  ERR_INVALID_PACKAGE_CONFIG,
-} = require('internal/errors').codes;
-
-const { filterOwnProperties } = require('internal/util');
-
-
-/**
- * @typedef {string | string[] | Record<string, unknown>} Exports
- * @typedef {'module' | 'commonjs'} PackageType
- * @typedef {{
- *   pjsonPath: string,
- *   exports?: ExportConfig,
- *   name?: string,
- *   main?: string,
- *   type?: PackageType,
- * }} PackageConfig
- */
-
-/** @type {Map<string, PackageConfig>} */
-const packageJSONCache = new SafeMap();
-
-
-/**
- * @param {string} path
- * @param {string} specifier
- * @param {string | URL | undefined} base
- * @returns {PackageConfig}
- */
-function getPackageConfig(path, specifier, base) {
-  const existing = packageJSONCache.get(path);
-  if (existing !== undefined) {
-    return existing;
-  }
-  const packageJsonReader = require('internal/modules/package_json_reader');
-  const source = packageJsonReader.read(path).string;
-  if (source === undefined) {
-    const packageConfig = {
-      pjsonPath: path,
-      exists: false,
-      main: undefined,
-      name: undefined,
-      type: 'none',
-      exports: undefined,
-      imports: undefined,
-    };
-    packageJSONCache.set(path, packageConfig);
-    return packageConfig;
-  }
-
-  let packageJSON;
-  try {
-    packageJSON = JSONParse(source);
-  } catch (error) {
-    throw new ERR_INVALID_PACKAGE_CONFIG(
-      path,
-      (base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier),
-      error.message,
-    );
-  }
-
-  let { imports, main, name, type } = filterOwnProperties(packageJSON, ['imports', 'main', 'name', 'type']);
-  const exports = ObjectPrototypeHasOwnProperty(packageJSON, 'exports') ? packageJSON.exports : undefined;
-  if (typeof imports !== 'object' || imports === null) {
-    imports = undefined;
-  }
-  if (typeof main !== 'string') {
-    main = undefined;
-  }
-  if (typeof name !== 'string') {
-    name = undefined;
-  }
-  // Ignore unknown types for forwards compatibility
-  if (type !== 'module' && type !== 'commonjs') {
-    type = 'none';
-  }
-
-  const packageConfig = {
-    pjsonPath: path,
-    exists: true,
-    main,
-    name,
-    type,
-    exports,
-    imports,
-  };
-  packageJSONCache.set(path, packageConfig);
-  return packageConfig;
-}
-
+const packageJsonReader = require('internal/modules/package_json_reader');
 
 /**
  * @param {URL | string} resolved
@@ -109,7 +17,11 @@ function getPackageScopeConfig(resolved) {
     if (StringPrototypeEndsWith(packageJSONPath, 'node_modules/package.json')) {
       break;
     }
-    const packageConfig = getPackageConfig(fileURLToPath(packageJSONUrl), resolved);
+    const packageConfig = packageJsonReader.read(fileURLToPath(packageJSONUrl), {
+      __proto__: null,
+      specifier: resolved,
+      isESM: true,
+    });
     if (packageConfig.exists) {
       return packageConfig;
     }
@@ -124,7 +36,8 @@ function getPackageScopeConfig(resolved) {
     }
   }
   const packageJSONPath = fileURLToPath(packageJSONUrl);
-  const packageConfig = {
+  return {
+    __proto__: null,
     pjsonPath: packageJSONPath,
     exists: false,
     main: undefined,
@@ -133,12 +46,9 @@ function getPackageScopeConfig(resolved) {
     exports: undefined,
     imports: undefined,
   };
-  packageJSONCache.set(packageJSONPath, packageConfig);
-  return packageConfig;
 }
 
 
 module.exports = {
-  getPackageConfig,
   getPackageScopeConfig,
 };
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
index 927b118f8ede2b..d38f75523704f1 100644
--- a/lib/internal/modules/esm/resolve.js
+++ b/lib/internal/modules/esm/resolve.js
@@ -53,8 +53,9 @@ const {
 } = require('internal/errors').codes;
 
 const { Module: CJSModule } = require('internal/modules/cjs/loader');
-const { getPackageConfig, getPackageScopeConfig } = require('internal/modules/esm/package_config');
+const { getPackageScopeConfig } = require('internal/modules/esm/package_config');
 const { getConditionsSet } = require('internal/modules/esm/utils');
+const packageJsonReader = require('internal/modules/package_json_reader');
 const { internalModuleStat } = internalBinding('fs');
 
 /**
@@ -734,8 +735,7 @@ function packageResolve(specifier, base, conditions) {
   const packageConfig = getPackageScopeConfig(base);
   if (packageConfig.exists) {
     const packageJSONUrl = pathToFileURL(packageConfig.pjsonPath);
-    if (packageConfig.name === packageName &&
-        packageConfig.exports !== undefined && packageConfig.exports !== null) {
+    if (packageConfig.exports != null && packageConfig.name === packageName) {
       return packageExportsResolve(
         packageJSONUrl, packageSubpath, packageConfig, base, conditions);
     }
@@ -759,8 +759,8 @@ function packageResolve(specifier, base, conditions) {
     }
 
     // Package match.
-    const packageConfig = getPackageConfig(packageJSONPath, specifier, base);
-    if (packageConfig.exports !== undefined && packageConfig.exports !== null) {
+    const packageConfig = packageJsonReader.read(packageJSONPath, { __proto__: null, specifier, base, isESM: true });
+    if (packageConfig.exports != null) {
       return packageExportsResolve(
         packageJSONUrl, packageSubpath, packageConfig, base, conditions);
     }
diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js
index bb175d0df54c04..25d2309653810f 100644
--- a/lib/internal/modules/package_json_reader.js
+++ b/lib/internal/modules/package_json_reader.js
@@ -1,30 +1,106 @@
 'use strict';
 
-const { SafeMap } = primordials;
+const {
+  JSONParse,
+  ObjectPrototypeHasOwnProperty,
+  SafeMap,
+} = primordials;
+const {
+  ERR_INVALID_PACKAGE_CONFIG,
+} = require('internal/errors').codes;
 const { internalModuleReadJSON } = internalBinding('fs');
-const { pathToFileURL } = require('url');
 const { toNamespacedPath } = require('path');
+const { kEmptyObject } = require('internal/util');
+const { fileURLToPath, pathToFileURL } = require('internal/url');
 
 const cache = new SafeMap();
 
 let manifest;
 
 /**
- *
+ * @typedef {{
+ *   exists: boolean,
+ *   pjsonPath: string,
+ *   exports?: string | string[] | Record<string, unknown>,
+ *   imports?: string | string[] | Record<string, unknown>,
+ *   name?: string,
+ *   main?: string,
+ *   type: 'commonjs' | 'module' | 'none',
+ * }} PackageConfig
+ */
+
+/**
  * @param {string} jsonPath
+ * @param {{
+ *   base?: string,
+ *   specifier: string,
+ *   isESM: boolean,
+ * }} options
+ * @returns {PackageConfig}
  */
-function read(jsonPath) {
+function read(jsonPath, { base, specifier, isESM } = kEmptyObject) {
   if (cache.has(jsonPath)) {
     return cache.get(jsonPath);
   }
 
-  const { 0: string, 1: containsKeys } = internalModuleReadJSON(
+  const string = internalModuleReadJSON(
     toNamespacedPath(jsonPath),
   );
-  const result = { string, containsKeys };
-  const { getOptionValue } = require('internal/options');
+  const result = {
+    __proto__: null,
+    exists: false,
+    pjsonPath: jsonPath,
+    main: undefined,
+    name: undefined,
+    type: 'none', // Ignore unknown types for forwards compatibility
+    exports: undefined,
+    imports: undefined,
+  };
+
   if (string !== undefined) {
+    let parsed;
+    try {
+      parsed = JSONParse(string);
+    } catch (error) {
+      if (isESM) {
+        throw new ERR_INVALID_PACKAGE_CONFIG(
+          jsonPath,
+          (base ? `"${specifier}" from ` : '') + fileURLToPath(base || specifier),
+          error.message,
+        );
+      } else {
+        error.message = 'Error parsing ' + jsonPath + ': ' + error.message;
+        error.path = jsonPath;
+        throw error;
+      }
+    }
+
+    result.exists = true;
+
+    // ObjectPrototypeHasOwnProperty is used to avoid prototype pollution.
+    if (ObjectPrototypeHasOwnProperty(parsed, 'name') && typeof parsed.name === 'string') {
+      result.name = parsed.name;
+    }
+
+    if (ObjectPrototypeHasOwnProperty(parsed, 'main') && typeof parsed.main === 'string') {
+      result.main = parsed.main;
+    }
+
+    if (ObjectPrototypeHasOwnProperty(parsed, 'exports')) {
+      result.exports = parsed.exports;
+    }
+
+    if (ObjectPrototypeHasOwnProperty(parsed, 'imports')) {
+      result.imports = parsed.imports;
+    }
+
+    // Ignore unknown types for forwards compatibility
+    if (ObjectPrototypeHasOwnProperty(parsed, 'type') && (parsed.type === 'commonjs' || parsed.type === 'module')) {
+      result.type = parsed.type;
+    }
+
     if (manifest === undefined) {
+      const { getOptionValue } = require('internal/options');
       manifest = getOptionValue('--experimental-policy') ?
         require('internal/process/policy').manifest :
         null;
diff --git a/src/node_file.cc b/src/node_file.cc
index 5a92432019dbb1..74093e893c09eb 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -54,7 +54,6 @@ namespace fs {
 
 using v8::Array;
 using v8::BigInt;
-using v8::Boolean;
 using v8::Context;
 using v8::EscapableHandleScope;
 using v8::Function;
@@ -1032,7 +1031,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
       env, permission::PermissionScope::kFileSystemRead, path.ToStringView());
 
   if (strlen(*path) != path.length()) {
-    args.GetReturnValue().Set(Array::New(isolate));
+    args.GetReturnValue().Set(Undefined(isolate));
     return;  // Contains a nul byte.
   }
   uv_fs_t open_req;
@@ -1040,7 +1039,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
   uv_fs_req_cleanup(&open_req);
 
   if (fd < 0) {
-    args.GetReturnValue().Set(Array::New(isolate));
+    args.GetReturnValue().Set(Undefined(isolate));
     return;
   }
 
@@ -1067,7 +1066,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
     uv_fs_req_cleanup(&read_req);
 
     if (numchars < 0) {
-      args.GetReturnValue().Set(Array::New(isolate));
+      args.GetReturnValue().Set(Undefined(isolate));
       return;
     }
     offset += numchars;
@@ -1077,45 +1076,12 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
   if (offset >= 3 && 0 == memcmp(chars.data(), "\xEF\xBB\xBF", 3)) {
     start = 3;  // Skip UTF-8 BOM.
   }
-
   const size_t size = offset - start;
-  char* p = &chars[start];
-  char* pe = &chars[size];
-  char* pos[2];
-  char** ppos = &pos[0];
-
-  while (p < pe) {
-    char c = *p++;
-    if (c == '\\' && p < pe && *p == '"') p++;
-    if (c != '"') continue;
-    *ppos++ = p;
-    if (ppos < &pos[2]) continue;
-    ppos = &pos[0];
-
-    char* s = &pos[0][0];
-    char* se = &pos[1][-1];  // Exclude quote.
-    size_t n = se - s;
-
-    if (n == 4) {
-      if (0 == memcmp(s, "main", 4)) break;
-      if (0 == memcmp(s, "name", 4)) break;
-      if (0 == memcmp(s, "type", 4)) break;
-    } else if (n == 7) {
-      if (0 == memcmp(s, "exports", 7)) break;
-      if (0 == memcmp(s, "imports", 7)) break;
-    }
-  }
 
-
-  Local<Value> return_value[] = {
-    String::NewFromUtf8(isolate,
-                        &chars[start],
-                        v8::NewStringType::kNormal,
-                        size).ToLocalChecked(),
-    Boolean::New(isolate, p < pe ? true : false)
-  };
   args.GetReturnValue().Set(
-    Array::New(isolate, return_value, arraysize(return_value)));
+      String::NewFromUtf8(
+          isolate, &chars[start], v8::NewStringType::kNormal, size)
+          .ToLocalChecked());
 }
 
 // Used to speed up module loading.  Returns 0 if the path refers to
diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot
index b52735370ea0f3..daef7905a6be19 100644
--- a/test/fixtures/errors/force_colors.snapshot
+++ b/test/fixtures/errors/force_colors.snapshot
@@ -4,10 +4,10 @@ throw new Error('Should include grayed stack trace')
 
 Error: Should include grayed stack trace
     at Object.<anonymous> (/test*force_colors.js:1:7)
-    at Module._compile (node:internal*modules*cjs*loader:1257:14)
-    at Module._extensions..js (node:internal*modules*cjs*loader:1311:10)
-    at Module.load (node:internal*modules*cjs*loader:1115:32)
-    at Module._load (node:internal*modules*cjs*loader:962:12)
+    at Module._compile (node:internal*modules*cjs*loader:1233:14)
+    at Module._extensions..js (node:internal*modules*cjs*loader:1287:10)
+    at Module.load (node:internal*modules*cjs*loader:1091:32)
+    at Module._load (node:internal*modules*cjs*loader:938:12)
     at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:83:12)
     at node:internal*main*run_main_module:23:47
 
diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js
index a0fa0a038990f0..03a6e06025fff7 100644
--- a/test/parallel/test-module-binding.js
+++ b/test/parallel/test-module-binding.js
@@ -4,34 +4,14 @@ require('../common');
 const fixtures = require('../common/fixtures');
 const { internalBinding } = require('internal/test/binding');
 const { internalModuleReadJSON } = internalBinding('fs');
-const { readFileSync } = require('fs');
 const { strictEqual } = require('assert');
+
 {
-  const [string, containsKeys] = internalModuleReadJSON('nosuchfile');
-  strictEqual(string, undefined);
-  strictEqual(containsKeys, undefined);
+  strictEqual(internalModuleReadJSON('nosuchfile'), undefined);
 }
 {
-  const [string, containsKeys] =
-    internalModuleReadJSON(fixtures.path('empty.txt'));
-  strictEqual(string, '');
-  strictEqual(containsKeys, false);
+  strictEqual(internalModuleReadJSON(fixtures.path('empty.txt')), '');
 }
 {
-  const [string, containsKeys] =
-    internalModuleReadJSON(fixtures.path('empty.txt'));
-  strictEqual(string, '');
-  strictEqual(containsKeys, false);
-}
-{
-  const [string, containsKeys] =
-    internalModuleReadJSON(fixtures.path('empty-with-bom.txt'));
-  strictEqual(string, '');
-  strictEqual(containsKeys, false);
-}
-{
-  const filename = fixtures.path('require-bin/package.json');
-  const [string, containsKeys] = internalModuleReadJSON(filename);
-  strictEqual(string, readFileSync(filename, 'utf8'));
-  strictEqual(containsKeys, true);
+  strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt')), '');
 }

From b133e1c6bb11f298aca92c24cdd594ae3b66cfed Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Thu, 22 Jun 2023 20:04:54 -0400
Subject: [PATCH 2/9] test: add more tests

---
 test/parallel/test-module-binding.js | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js
index 03a6e06025fff7..d7f76d6ef5b153 100644
--- a/test/parallel/test-module-binding.js
+++ b/test/parallel/test-module-binding.js
@@ -3,8 +3,10 @@
 require('../common');
 const fixtures = require('../common/fixtures');
 const { internalBinding } = require('internal/test/binding');
+const { filterOwnProperties } = require('internal/util');
 const { internalModuleReadJSON } = internalBinding('fs');
-const { strictEqual } = require('assert');
+const { readFileSync } = require('fs');
+const { strictEqual, deepStrictEqual } = require('assert');
 
 {
   strictEqual(internalModuleReadJSON('nosuchfile'), undefined);
@@ -15,3 +17,13 @@ const { strictEqual } = require('assert');
 {
   strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt')), '');
 }
+{
+  const filename = fixtures.path('require-bin/package.json');
+  const returnValue = JSON.parse(internalModuleReadJSON(filename));
+  const file = JSON.parse(readFileSync(filename, 'utf-8'));
+  const expectedValue = filterOwnProperties(file, ['name', 'main', 'exports', 'imports', 'type']);
+  deepStrictEqual({
+    __proto__: null,
+    ...returnValue,
+  }, expectedValue);
+}

From c769ca53066ea8894b8527b992a85eb265827bce Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Fri, 23 Jun 2023 13:09:04 -0400
Subject: [PATCH 3/9] lib: add comment and todo by request

---
 lib/internal/modules/package_json_reader.js | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js
index 25d2309653810f..a0a15c00135f46 100644
--- a/lib/internal/modules/package_json_reader.js
+++ b/lib/internal/modules/package_json_reader.js
@@ -69,6 +69,8 @@ function read(jsonPath, { base, specifier, isESM } = kEmptyObject) {
           error.message,
         );
       } else {
+        // For backward compat, we modify the error returned by JSON.parse rather than creating a new one.
+        // TODO(aduh95): make it throw ERR_INVALID_PACKAGE_CONFIG in a semver-major with original error as cause
         error.message = 'Error parsing ' + jsonPath + ': ' + error.message;
         error.path = jsonPath;
         throw error;

From 5ea384028b72e90f694f452203001cec631c74a6 Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Fri, 23 Jun 2023 13:27:48 -0400
Subject: [PATCH 4/9] test: disable module loading error on aix

---
 test/parallel/test-module-loading-error.js | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/test/parallel/test-module-loading-error.js b/test/parallel/test-module-loading-error.js
index 8cbe8b2c675663..f6ff03672fb257 100644
--- a/test/parallel/test-module-loading-error.js
+++ b/test/parallel/test-module-loading-error.js
@@ -84,10 +84,15 @@ assert.throws(
     message: 'The argument \'id\' must be a non-empty string. Received \'\''
   });
 
-assert.throws(
-  () => { require('../fixtures/packages/is-dir'); },
-  {
-    code: 'MODULE_NOT_FOUND',
-    message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/
-  }
-);
+// Folder read operation succeeds in AIX.
+// For libuv change, see https://github.com/libuv/libuv/pull/2025.
+// https://github.com/nodejs/node/pull/48477#issuecomment-1604586650
+if (process.platform !== 'aix') {
+  assert.throws(
+    () => { require('../fixtures/packages/is-dir'); },
+    {
+      code: 'MODULE_NOT_FOUND',
+      message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/
+    }
+  );
+}

From b4720bd77d8b930cf49fbc692e948a661b10c38c Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Sat, 24 Jun 2023 14:02:00 -0400
Subject: [PATCH 5/9] lib: revert changes for AIX

---
 lib/internal/modules/package_json_reader.js | 15 ++++++-
 src/node_file.cc                            | 43 ++++++++++++++++++---
 test/parallel/test-module-binding.js        |  8 ++--
 test/parallel/test-module-loading-error.js  | 20 ++++------
 4 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js
index a0a15c00135f46..d84ec62d2fa090 100644
--- a/lib/internal/modules/package_json_reader.js
+++ b/lib/internal/modules/package_json_reader.js
@@ -14,6 +14,7 @@ const { kEmptyObject } = require('internal/util');
 const { fileURLToPath, pathToFileURL } = require('internal/url');
 
 const cache = new SafeMap();
+const isAIX = process.platform === 'aix';
 
 let manifest;
 
@@ -43,7 +44,10 @@ function read(jsonPath, { base, specifier, isESM } = kEmptyObject) {
     return cache.get(jsonPath);
   }
 
-  const string = internalModuleReadJSON(
+  const {
+    0: string,
+    1: containsKeys,
+  } = internalModuleReadJSON(
     toNamespacedPath(jsonPath),
   );
   const result = {
@@ -57,7 +61,14 @@ function read(jsonPath, { base, specifier, isESM } = kEmptyObject) {
     imports: undefined,
   };
 
-  if (string !== undefined) {
+  // Folder read operation succeeds in AIX.
+  // For libuv change, see https://github.com/libuv/libuv/pull/2025.
+  // https://github.com/nodejs/node/pull/48477#issuecomment-1604586650
+  // TODO(anonrig): Follow-up on this change and remove it since it is a
+  // semver-major change.
+  const isResultValid = isAIX ? containsKeys : string !== undefined;
+
+  if (isResultValid) {
     let parsed;
     try {
       parsed = JSONParse(string);
diff --git a/src/node_file.cc b/src/node_file.cc
index 74093e893c09eb..66a7a8f55dd4f1 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -54,6 +54,7 @@ namespace fs {
 
 using v8::Array;
 using v8::BigInt;
+using v8::Boolean;
 using v8::Context;
 using v8::EscapableHandleScope;
 using v8::Function;
@@ -1031,7 +1032,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
       env, permission::PermissionScope::kFileSystemRead, path.ToStringView());
 
   if (strlen(*path) != path.length()) {
-    args.GetReturnValue().Set(Undefined(isolate));
+    args.GetReturnValue().Set(Array::New(isolate));
     return;  // Contains a nul byte.
   }
   uv_fs_t open_req;
@@ -1039,7 +1040,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
   uv_fs_req_cleanup(&open_req);
 
   if (fd < 0) {
-    args.GetReturnValue().Set(Undefined(isolate));
+    args.GetReturnValue().Set(Array::New(isolate));
     return;
   }
 
@@ -1066,7 +1067,7 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
     uv_fs_req_cleanup(&read_req);
 
     if (numchars < 0) {
-      args.GetReturnValue().Set(Undefined(isolate));
+      args.GetReturnValue().Set(Array::New(isolate));
       return;
     }
     offset += numchars;
@@ -1078,10 +1079,42 @@ static void InternalModuleReadJSON(const FunctionCallbackInfo<Value>& args) {
   }
   const size_t size = offset - start;
 
-  args.GetReturnValue().Set(
+  // TODO(anonrig): Follow-up on removing the following changes for AIX.
+  char* p = &chars[start];
+  char* pe = &chars[size];
+  char* pos[2];
+  char** ppos = &pos[0];
+
+  while (p < pe) {
+    char c = *p++;
+    if (c == '\\' && p < pe && *p == '"') p++;
+    if (c != '"') continue;
+    *ppos++ = p;
+    if (ppos < &pos[2]) continue;
+    ppos = &pos[0];
+
+    char* s = &pos[0][0];
+    char* se = &pos[1][-1];  // Exclude quote.
+    size_t n = se - s;
+
+    if (n == 4) {
+      if (0 == memcmp(s, "main", 4)) break;
+      if (0 == memcmp(s, "name", 4)) break;
+      if (0 == memcmp(s, "type", 4)) break;
+    } else if (n == 7) {
+      if (0 == memcmp(s, "exports", 7)) break;
+      if (0 == memcmp(s, "imports", 7)) break;
+    }
+  }
+
+  Local<Value> return_value[] = {
       String::NewFromUtf8(
           isolate, &chars[start], v8::NewStringType::kNormal, size)
-          .ToLocalChecked());
+          .ToLocalChecked(),
+      Boolean::New(isolate, p < pe ? true : false)};
+
+  args.GetReturnValue().Set(
+      Array::New(isolate, return_value, arraysize(return_value)));
 }
 
 // Used to speed up module loading.  Returns 0 if the path refers to
diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js
index d7f76d6ef5b153..47170785434099 100644
--- a/test/parallel/test-module-binding.js
+++ b/test/parallel/test-module-binding.js
@@ -9,17 +9,17 @@ const { readFileSync } = require('fs');
 const { strictEqual, deepStrictEqual } = require('assert');
 
 {
-  strictEqual(internalModuleReadJSON('nosuchfile'), undefined);
+  strictEqual(internalModuleReadJSON('nosuchfile')[0], undefined);
 }
 {
-  strictEqual(internalModuleReadJSON(fixtures.path('empty.txt')), '');
+  strictEqual(internalModuleReadJSON(fixtures.path('empty.txt'))[0], '');
 }
 {
-  strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt')), '');
+  strictEqual(internalModuleReadJSON(fixtures.path('empty-with-bom.txt'))[0], '');
 }
 {
   const filename = fixtures.path('require-bin/package.json');
-  const returnValue = JSON.parse(internalModuleReadJSON(filename));
+  const returnValue = JSON.parse(internalModuleReadJSON(filename)[0]);
   const file = JSON.parse(readFileSync(filename, 'utf-8'));
   const expectedValue = filterOwnProperties(file, ['name', 'main', 'exports', 'imports', 'type']);
   deepStrictEqual({
diff --git a/test/parallel/test-module-loading-error.js b/test/parallel/test-module-loading-error.js
index f6ff03672fb257..82d781c29f9f46 100644
--- a/test/parallel/test-module-loading-error.js
+++ b/test/parallel/test-module-loading-error.js
@@ -84,15 +84,11 @@ assert.throws(
     message: 'The argument \'id\' must be a non-empty string. Received \'\''
   });
 
-// Folder read operation succeeds in AIX.
-// For libuv change, see https://github.com/libuv/libuv/pull/2025.
-// https://github.com/nodejs/node/pull/48477#issuecomment-1604586650
-if (process.platform !== 'aix') {
-  assert.throws(
-    () => { require('../fixtures/packages/is-dir'); },
-    {
-      code: 'MODULE_NOT_FOUND',
-      message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/
-    }
-  );
-}
+
+assert.throws(
+  () => { require('../fixtures/packages/is-dir'); },
+  {
+    code: 'MODULE_NOT_FOUND',
+    message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/
+  }
+);

From 4d5ba8c7d144ae2aa05f9f3ce153f4c90aa4b48c Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Sat, 24 Jun 2023 15:17:06 -0400
Subject: [PATCH 6/9] lib: change detection for aix and cjs

---
 lib/internal/modules/package_json_reader.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js
index d84ec62d2fa090..10567aa2ab97e2 100644
--- a/lib/internal/modules/package_json_reader.js
+++ b/lib/internal/modules/package_json_reader.js
@@ -66,7 +66,7 @@ function read(jsonPath, { base, specifier, isESM } = kEmptyObject) {
   // https://github.com/nodejs/node/pull/48477#issuecomment-1604586650
   // TODO(anonrig): Follow-up on this change and remove it since it is a
   // semver-major change.
-  const isResultValid = isAIX ? containsKeys : string !== undefined;
+  const isResultValid = isAIX && !isESM ? containsKeys : string !== undefined;
 
   if (isResultValid) {
     let parsed;

From 54dcf06006e1106d3293a5208e1a092ac4bfa071 Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Wed, 28 Jun 2023 12:00:22 -0400
Subject: [PATCH 7/9] lib: fix worker bootstrap error

---
 lib/internal/modules/package_json_reader.js | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js
index 10567aa2ab97e2..fd8fa3d22ab421 100644
--- a/lib/internal/modules/package_json_reader.js
+++ b/lib/internal/modules/package_json_reader.js
@@ -11,7 +11,10 @@ const {
 const { internalModuleReadJSON } = internalBinding('fs');
 const { toNamespacedPath } = require('path');
 const { kEmptyObject } = require('internal/util');
-const { fileURLToPath, pathToFileURL } = require('internal/url');
+
+// It is important to require from 'url' and not from 'internal/url'
+// in order to add url module to bootstrapped modules for workers.
+const { fileURLToPath, pathToFileURL } = require('url');
 
 const cache = new SafeMap();
 const isAIX = process.platform === 'aix';

From 0ab48b65b980c1b296a0c009b02ddcc909b04093 Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Wed, 28 Jun 2023 14:58:07 -0400
Subject: [PATCH 8/9] lib: remove url from worker bootstrapped modules

---
 lib/internal/modules/package_json_reader.js |  4 +---
 test/parallel/test-bootstrap-modules.js     | 11 ++++++-----
 test/parallel/test-module-loading-error.js  |  1 -
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js
index fd8fa3d22ab421..c6377faae6f5a8 100644
--- a/lib/internal/modules/package_json_reader.js
+++ b/lib/internal/modules/package_json_reader.js
@@ -12,9 +12,7 @@ const { internalModuleReadJSON } = internalBinding('fs');
 const { toNamespacedPath } = require('path');
 const { kEmptyObject } = require('internal/util');
 
-// It is important to require from 'url' and not from 'internal/url'
-// in order to add url module to bootstrapped modules for workers.
-const { fileURLToPath, pathToFileURL } = require('url');
+const { fileURLToPath, pathToFileURL } = require('internal/url');
 
 const cache = new SafeMap();
 const isAIX = process.platform === 'aix';
diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js
index 0a671eb95eb6d4..78db466a95b38e 100644
--- a/test/parallel/test-bootstrap-modules.js
+++ b/test/parallel/test-bootstrap-modules.js
@@ -75,7 +75,6 @@ const expectedModules = new Set([
   'NativeModule internal/blob',
   'NativeModule internal/fs/utils',
   'NativeModule fs',
-  'NativeModule internal/idna',
   'Internal Binding options',
   'NativeModule internal/options',
   'NativeModule internal/source_map/source_map_cache',
@@ -95,7 +94,12 @@ const expectedModules = new Set([
   'NativeModule internal/process/pre_execution',
 ]);
 
-if (!common.isMainThread) {
+if (common.isMainThread) {
+  [
+    'NativeModule internal/idna',
+    'NativeModule url',
+  ].forEach(expectedModules.add.bind(expectedModules));
+} else {
   [
     'NativeModule diagnostics_channel',
     'NativeModule internal/abort_controller',
@@ -134,9 +138,6 @@ if (common.isWindows) {
 
 if (common.hasIntl) {
   expectedModules.add('Internal Binding icu');
-  expectedModules.add('NativeModule url');
-} else {
-  expectedModules.add('NativeModule url');
 }
 
 if (process.features.inspector) {
diff --git a/test/parallel/test-module-loading-error.js b/test/parallel/test-module-loading-error.js
index 82d781c29f9f46..8cbe8b2c675663 100644
--- a/test/parallel/test-module-loading-error.js
+++ b/test/parallel/test-module-loading-error.js
@@ -84,7 +84,6 @@ assert.throws(
     message: 'The argument \'id\' must be a non-empty string. Received \'\''
   });
 
-
 assert.throws(
   () => { require('../fixtures/packages/is-dir'); },
   {

From a8fbf805c9519b4239722a7e201ddcbd8c7b2a7b Mon Sep 17 00:00:00 2001
From: Yagiz Nizipli <yagiz@nizipli.com>
Date: Wed, 28 Jun 2023 16:10:58 -0400
Subject: [PATCH 9/9] lib: move SafeSet declarations to internal/url

---
 lib/internal/url.js | 38 ++++++++++++++++++++++++++++++++++++++
 lib/url.js          | 31 +++----------------------------
 2 files changed, 41 insertions(+), 28 deletions(-)

diff --git a/lib/internal/url.js b/lib/internal/url.js
index 1813b86962f4cd..302d3fa08753a5 100644
--- a/lib/internal/url.js
+++ b/lib/internal/url.js
@@ -18,6 +18,7 @@ const {
   ReflectOwnKeys,
   RegExpPrototypeSymbolReplace,
   SafeMap,
+  SafeSet,
   StringPrototypeCharAt,
   StringPrototypeCharCodeAt,
   StringPrototypeCodePointAt,
@@ -88,6 +89,40 @@ const FORWARD_SLASH = /\//g;
 
 const contextForInspect = Symbol('context');
 
+// `unsafeProtocol`, `hostlessProtocol` and `slashedProtocol` is
+// deliberately moved to `internal/url` rather than `url`.
+// Workers does not bootstrap URL module. Therefore, `SafeSet`
+// is not initialized on bootstrap. This case breaks the
+// test-require-delete-array-iterator test.
+
+// Protocols that can allow "unsafe" and "unwise" chars.
+const unsafeProtocol = new SafeSet([
+  'javascript',
+  'javascript:',
+]);
+// Protocols that never have a hostname.
+const hostlessProtocol = new SafeSet([
+  'javascript',
+  'javascript:',
+]);
+// Protocols that always contain a // bit.
+const slashedProtocol = new SafeSet([
+  'http',
+  'http:',
+  'https',
+  'https:',
+  'ftp',
+  'ftp:',
+  'gopher',
+  'gopher:',
+  'file',
+  'file:',
+  'ws',
+  'ws:',
+  'wss',
+  'wss:',
+]);
+
 const updateActions = {
   kProtocol: 0,
   kHost: 1,
@@ -1464,4 +1499,7 @@ module.exports = {
 
   urlUpdateActions: updateActions,
   getURLOrigin,
+  unsafeProtocol,
+  hostlessProtocol,
+  slashedProtocol,
 };
diff --git a/lib/url.js b/lib/url.js
index 96a34babebb029..2cb51ff362a295 100644
--- a/lib/url.js
+++ b/lib/url.js
@@ -25,7 +25,6 @@ const {
   Boolean,
   Int8Array,
   ObjectKeys,
-  SafeSet,
   StringPrototypeCharCodeAt,
   decodeURIComponent,
 } = primordials;
@@ -56,6 +55,9 @@ const {
   fileURLToPath,
   pathToFileURL,
   urlToHttpOptions,
+  unsafeProtocol,
+  hostlessProtocol,
+  slashedProtocol,
 } = require('internal/url');
 
 const bindingUrl = internalBinding('url');
@@ -91,33 +93,6 @@ const hostPattern = /^\/\/[^@/]+@[^@/]+/;
 const simplePathPattern = /^(\/\/?(?!\/)[^?\s]*)(\?[^\s]*)?$/;
 
 const hostnameMaxLen = 255;
-// Protocols that can allow "unsafe" and "unwise" chars.
-const unsafeProtocol = new SafeSet([
-  'javascript',
-  'javascript:',
-]);
-// Protocols that never have a hostname.
-const hostlessProtocol = new SafeSet([
-  'javascript',
-  'javascript:',
-]);
-// Protocols that always contain a // bit.
-const slashedProtocol = new SafeSet([
-  'http',
-  'http:',
-  'https',
-  'https:',
-  'ftp',
-  'ftp:',
-  'gopher',
-  'gopher:',
-  'file',
-  'file:',
-  'ws',
-  'ws:',
-  'wss',
-  'wss:',
-]);
 const {
   CHAR_SPACE,
   CHAR_TAB,