lib: rework logic of stripping BOM+Shebang from commonjs

Fixes nodejs#27767 PR-URL: nodejs#27768 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
aduh95 · Jan 6, 2020 · eb38365 · eb38365
1 parent b08601b
commit eb38365
Show file tree

Hide file tree

Showing 13 changed files with 100 additions and 85 deletions.
diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
@@ -389,7 +389,7 @@ function initializePolicy() {
 }
 
 function initializeCJSLoader() {
-  require('internal/modules/cjs/loader')._initPaths();
+  require('internal/modules/cjs/loader').Module._initPaths();
 }
 
 function initializeESMLoader() {
@@ -438,7 +438,9 @@ function loadPreloadModules() {
   const preloadModules = getOptionValue('--require');
   if (preloadModules && preloadModules.length > 0) {
     const {
-      _preloadModules
+      Module: {
+        _preloadModules
+      },
     } = require('internal/modules/cjs/loader');
     _preloadModules(preloadModules);
   }

diff --git a/lib/internal/main/check_syntax.js b/lib/internal/main/check_syntax.js
@@ -13,14 +13,15 @@ const {
 
 const { pathToFileURL } = require('url');
 
-const vm = require('vm');
 const {
-  stripShebang, stripBOM
+  stripShebangOrBOM,
 } = require('internal/modules/cjs/helpers');
 
 const {
-  _resolveFilename: resolveCJSModuleName,
-  wrap: wrapCJSModule
+  Module: {
+    _resolveFilename: resolveCJSModuleName,
+  },
+  wrapSafe,
 } = require('internal/modules/cjs/loader');
 
 // TODO(joyeecheung): not every one of these are necessary
@@ -49,9 +50,6 @@ if (process.argv[1] && process.argv[1] !== '-') {
 }
 
 function checkSyntax(source, filename) {
-  // Remove Shebang.
-  source = stripShebang(source);
-
   const { getOptionValue } = require('internal/options');
   const experimentalModules = getOptionValue('--experimental-modules');
   if (experimentalModules) {
@@ -70,10 +68,5 @@ function checkSyntax(source, filename) {
     }
   }
 
-  // Remove BOM.
-  source = stripBOM(source);
-  // Wrap it.
-  source = wrapCJSModule(source);
-  // Compile the script, this will throw if it fails.
-  new vm.Script(source, { displayErrors: true, filename });
+  wrapSafe(filename, stripShebangOrBOM(source));
 }
diff --git a/lib/internal/main/run_main_module.js b/lib/internal/main/run_main_module.js
@@ -6,7 +6,7 @@ const {
 
 prepareMainThreadExecution(true);
 
-const CJSModule = require('internal/modules/cjs/loader');
+const CJSModule = require('internal/modules/cjs/loader').Module;
 
 markBootstrapComplete();
 

diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js
@@ -131,6 +131,17 @@ function stripShebang(content) {
   return content;
 }
 
+// Strip either the shebang or UTF BOM of a file.
+// Note that this only processes one. If both occur in
+// either order, the one that comes second is not
+// significant.
+function stripShebangOrBOM(content) {
+  if (content.charCodeAt(0) === 0xFEFF) {
+    return content.slice(1);
+  }
+  return stripShebang(content);
+}
+
 const builtinLibs = [
   'assert', 'async_hooks', 'buffer', 'child_process', 'cluster', 'crypto',
   'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'http2', 'https', 'net',
@@ -197,5 +208,6 @@ module.exports = {
   makeRequireFunction,
   normalizeReferrerURL,
   stripBOM,
-  stripShebang
+  stripShebang,
+  stripShebangOrBOM,
 };
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -51,8 +51,7 @@ const {
   makeRequireFunction,
   normalizeReferrerURL,
   stripBOM,
-  stripShebang,
-  loadNativeModule
+  stripShebangOrBOM,
 } = require('internal/modules/cjs/helpers');
 const { getOptionValue } = require('internal/options');
 const enableSourceMaps = getOptionValue('--enable-source-maps');
@@ -73,7 +72,7 @@ const { validateString } = require('internal/validators');
 const pendingDeprecation = getOptionValue('--pending-deprecation');
 const experimentalExports = getOptionValue('--experimental-exports');
 
-module.exports = Module;
+module.exports = { wrapSafe, Module };
 
 let asyncESM, ModuleJob, ModuleWrap, kInstantiated;
 
@@ -857,26 +856,10 @@ Module.prototype.require = function(id) {
 let resolvedArgv;
 let hasPausedEntry = false;
 
-// Run the file contents in the correct scope or sandbox. Expose
-// the correct helper variables (require, module, exports) to
-// the file.
-// Returns exception, if any.
-Module.prototype._compile = function(content, filename) {
-  let moduleURL;
-  let redirects;
-  if (manifest) {
-    moduleURL = pathToFileURL(filename);
-    redirects = manifest.getRedirector(moduleURL);
-    manifest.assertIntegrity(moduleURL, content);
-  }
-
-  content = stripShebang(content);
-  maybeCacheSourceMap(filename, content, this);
-
-  let compiledWrapper;
+function wrapSafe(filename, content) {
   if (patched) {
     const wrapper = Module.wrap(content);
-    compiledWrapper = vm.runInThisContext(wrapper, {
+    return vm.runInThisContext(wrapper, {
       filename,
       lineOffset: 0,
       displayErrors: true,
@@ -885,46 +868,61 @@ Module.prototype._compile = function(content, filename) {
         return loader.import(specifier, normalizeReferrerURL(filename));
       } : undefined,
     });
-  } else {
-    let compiled;
-    try {
-      compiled = compileFunction(
-        content,
-        filename,
-        0,
-        0,
-        undefined,
-        false,
-        undefined,
-        [],
-        [
-          'exports',
-          'require',
-          'module',
-          '__filename',
-          '__dirname',
-        ]
-      );
-    } catch (err) {
-      if (experimentalModules) {
-        enrichCJSError(err);
+  }
+
+  let compiledWrapper;
+  try {
+    compiledWrapper = compileFunction(
+      content,
+      filename,
+      0,
+      0,
+      undefined,
+      false,
+      undefined,
+      [],
+      [
+        'exports',
+        'require',
+        'module',
+        '__filename',
+        '__dirname',
+      ]
+    );
+  } catch (err) {
+    enrichCJSError(err);
+    throw err;
+  }
+
+  if (experimentalModules) {
+    const { callbackMap } = internalBinding('module_wrap');
+    callbackMap.set(compiledWrapper, {
+      importModuleDynamically: async (specifier) => {
+        const loader = await asyncESM.loaderPromise;
+        return loader.import(specifier, normalizeReferrerURL(filename));
       }
-      throw err;
-    }
+    });
+  }
 
-    if (experimentalModules) {
-      const { callbackMap } = internalBinding('module_wrap');
-      callbackMap.set(compiled.cacheKey, {
-        importModuleDynamically: async (specifier) => {
-          const loader = await asyncESM.loaderPromise;
-          return loader.import(specifier, normalizeReferrerURL(filename));
-        }
-      });
-    }
-    compiledWrapper = compiled.function;
+  return compiledWrapper;
+}
+
+// Run the file contents in the correct scope or sandbox. Expose
+// the correct helper variables (require, module, exports) to
+// the file.
+// Returns exception, if any.
+Module.prototype._compile = function(content, filename) {
+  if (manifest) {
+    const moduleURL = pathToFileURL(filename);
+    manifest.assertIntegrity(moduleURL, content);
   }
 
-  let inspectorWrapper = null;
+  // Strip after manifest integrity check
+  content = stripShebangOrBOM(content);
+
+  const compiledWrapper = wrapSafe(filename, content);
+
+  var inspectorWrapper = null;
   if (getOptionValue('--inspect-brk') && process._eval == null) {
     if (!resolvedArgv) {
       // We enter the repl if we're not given a filename argument.
@@ -988,7 +986,7 @@ Module._extensions['.js'] = function(module, filename) {
     }
   }
   const content = fs.readFileSync(filename, 'utf8');
-  module._compile(stripBOM(content), filename);
+  module._compile(content, filename);
 };
 
 

diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -12,11 +12,9 @@ const {
 const { Buffer } = require('buffer');
 
 const {
-  stripShebang,
-  stripBOM,
-  loadNativeModule
+  stripBOM
 } = require('internal/modules/cjs/helpers');
-const CJSModule = require('internal/modules/cjs/loader');
+const CJSModule = require('internal/modules/cjs/loader').Module;
 const internalURLModule = require('internal/url');
 const createDynamicModule = require(
   'internal/modules/esm/create_dynamic_module');
@@ -80,8 +78,9 @@ translators.set('module', async function moduleStrategy(url) {
   const source = `${await getSource(url)}`;
   maybeCacheSourceMap(url, source);
   debug(`Translating StandardModule ${url}`);
-  const module = new ModuleWrap(stripShebang(source), url);
-  moduleWrap.callbackMap.set(module, {
+  const { ModuleWrap, callbackMap } = internalBinding('module_wrap');
+  const module = new ModuleWrap(source, url);
+  callbackMap.set(module, {
     initializeImportMeta,
     importModuleDynamically,
   });

diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js
@@ -53,7 +53,7 @@ function evalModule(source, print) {
 }
 
 function evalScript(name, body, breakFirstLine, print) {
-  const CJSModule = require('internal/modules/cjs/loader');
+  const CJSModule = require('internal/modules/cjs/loader').Module;
   const { kVmBreakFirstLineSymbol } = require('internal/util');
 
   const cwd = tryGetCwd();

diff --git a/lib/internal/util/inspector.js b/lib/internal/util/inspector.js
@@ -24,7 +24,7 @@ function sendInspectorCommand(cb, onError) {
 function installConsoleExtensions(commandLineApi) {
   if (commandLineApi.require) { return; }
   const { tryGetCwd } = require('internal/process/execution');
-  const CJSModule = require('internal/modules/cjs/loader');
+  const CJSModule = require('internal/modules/cjs/loader').Module;
   const { makeRequireFunction } = require('internal/modules/cjs/helpers');
   const consoleAPIModule = new CJSModule('<inspector console>');
   const cwd = tryGetCwd();

diff --git a/lib/module.js b/lib/module.js
@@ -1,3 +1,3 @@
 'use strict';
 
-module.exports = require('internal/modules/cjs/loader');
+module.exports = require('internal/modules/cjs/loader').Module;
diff --git a/lib/repl.js b/lib/repl.js
@@ -65,7 +65,7 @@ const path = require('path');
 const fs = require('fs');
 const { Interface } = require('readline');
 const { Console } = require('console');
-const CJSModule = require('internal/modules/cjs/loader');
+const CJSModule = require('internal/modules/cjs/loader').Module;
 const domain = require('domain');
 const debug = require('internal/util/debuglog').debuglog('repl');
 const {

diff --git a/test/fixtures/utf8-bom-shebang.js b/test/fixtures/utf8-bom-shebang.js
@@ -0,0 +1,2 @@
+#!shebang
+module.exports = 42;
diff --git a/test/fixtures/utf8-shebang-bom.js b/test/fixtures/utf8-shebang-bom.js
@@ -0,0 +1,2 @@
+#!shebang
+module.exports = 42;
diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js
@@ -352,6 +352,13 @@ process.on('exit', function() {
 assert.strictEqual(require('../fixtures/utf8-bom.js'), 42);
 assert.strictEqual(require('../fixtures/utf8-bom.json'), 42);
 
+// Loading files with BOM + shebang.
+// See https://github.com/nodejs/node/issues/27767
+assert.throws(() => {
+  require('../fixtures/utf8-bom-shebang.js');
+}, { name: 'SyntaxError' });
+assert.strictEqual(require('../fixtures/utf8-shebang-bom.js'), 42);
+
 // Error on the first line of a module should
 // have the correct line number
 assert.throws(function() {