lib: rework logic of stripping BOM+Shebang from commonjs

Fixes #27767 PR-URL: #27768 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · May 26, 2019 · e2c0c0c · e2c0c0c
1 parent 73c16b1
commit e2c0c0c
Show file tree

Hide file tree

Showing 13 changed files with 92 additions and 68 deletions.
diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
@@ -338,7 +338,7 @@ function initializePolicy() {
 }
 
 function initializeCJSLoader() {
-  require('internal/modules/cjs/loader')._initPaths();
+  require('internal/modules/cjs/loader').Module._initPaths();
 }
 
 function initializeESMLoader() {
@@ -387,7 +387,9 @@ function loadPreloadModules() {
   const preloadModules = getOptionValue('--require');
   if (preloadModules) {
     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
@@ -72,6 +72,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',
@@ -137,5 +148,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
@@ -40,7 +40,7 @@ const {
   makeRequireFunction,
   normalizeReferrerURL,
   stripBOM,
-  stripShebang
+  stripShebangOrBOM,
 } = require('internal/modules/cjs/helpers');
 const { getOptionValue } = require('internal/options');
 const preserveSymlinks = getOptionValue('--preserve-symlinks');
@@ -59,7 +59,7 @@ const {
 const { validateString } = require('internal/validators');
 const pendingDeprecation = getOptionValue('--pending-deprecation');
 
-module.exports = Module;
+module.exports = { wrapSafe, Module };
 
 let asyncESM;
 let ModuleJob;
@@ -690,22 +690,10 @@ Module.prototype.require = function(id) {
 var 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) {
-  if (manifest) {
-    const moduleURL = pathToFileURL(filename);
-    manifest.assertIntegrity(moduleURL, content);
-  }
-
-  content = stripShebang(content);
-
-  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,
@@ -714,35 +702,54 @@ Module.prototype._compile = function(content, filename) {
         return loader.import(specifier, normalizeReferrerURL(filename));
       } : undefined,
     });
-  } else {
-    compiledWrapper = compileFunction(
-      content,
-      filename,
-      0,
-      0,
-      undefined,
-      false,
-      undefined,
-      [],
-      [
-        'exports',
-        'require',
-        'module',
-        '__filename',
-        '__dirname',
-      ]
-    );
-    if (experimentalModules) {
-      const { callbackMap } = internalBinding('module_wrap');
-      callbackMap.set(compiledWrapper, {
-        importModuleDynamically: async (specifier) => {
-          const loader = await asyncESM.loaderPromise;
-          return loader.import(specifier, normalizeReferrerURL(filename));
-        }
-      });
-    }
   }
 
+  const compiledWrapper = compileFunction(
+    content,
+    filename,
+    0,
+    0,
+    undefined,
+    false,
+    undefined,
+    [],
+    [
+      'exports',
+      'require',
+      'module',
+      '__filename',
+      '__dirname',
+    ]
+  );
+
+  if (experimentalModules) {
+    const { callbackMap } = internalBinding('module_wrap');
+    callbackMap.set(compiledWrapper, {
+      importModuleDynamically: async (specifier) => {
+        const loader = await asyncESM.loaderPromise;
+        return loader.import(specifier, normalizeReferrerURL(filename));
+      }
+    });
+  }
+
+  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);
+  }
+
+  // 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) {
@@ -782,7 +789,7 @@ Module.prototype._compile = function(content, filename) {
 // Native extension for .js
 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
@@ -11,10 +11,9 @@ const {
 
 const { NativeModule } = require('internal/bootstrap/loaders');
 const {
-  stripShebang,
   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');
@@ -48,7 +47,7 @@ translators.set('module', async function moduleStrategy(url) {
   const source = `${await readFileAsync(new URL(url))}`;
   debug(`Translating StandardModule ${url}`);
   const { ModuleWrap, callbackMap } = internalBinding('module_wrap');
-  const module = new ModuleWrap(stripShebang(source), url);
+  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() {