From 54896a6961060f8e19e0dcf0576e3fd3d62fd33d Mon Sep 17 00:00:00 2001 From: Ujjwal Sharma Date: Wed, 7 Nov 2018 22:17:09 +0530 Subject: [PATCH] module: revert module._compile to original state if module is patched PR-URL: https://github.com/nodejs/node/pull/21573 Fixes: https://github.com/nodejs/node/issues/17396 Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater Reviewed-By: Gus Caplan Reviewed-By: Joyee Cheung Reviewed-By: Anna Henningsen --- lib/assert.js | 11 --- lib/internal/modules/cjs/helpers.js | 11 +++ lib/internal/modules/cjs/loader.js | 100 +++++++++++++++++++++++----- src/env-inl.h | 3 + src/env.cc | 11 +++ src/env.h | 10 +++ src/module_wrap.cc | 2 + src/module_wrap.h | 1 + src/node_contextify.cc | 55 ++++++++++++--- src/node_contextify.h | 2 + test/fixtures/cjs-module-wrap.js | 9 +++ test/parallel/test-module-wrap.js | 9 +++ test/parallel/test-v8-coverage.js | 26 ++++---- 13 files changed, 201 insertions(+), 49 deletions(-) create mode 100644 test/fixtures/cjs-module-wrap.js create mode 100644 test/parallel/test-module-wrap.js diff --git a/lib/assert.js b/lib/assert.js index 013ae6e674faec..94e9406393bf67 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -39,7 +39,6 @@ let isDeepEqual; let isDeepStrictEqual; let parseExpressionAt; let findNodeAround; -let columnOffset = 0; let decoder; function lazyLoadComparison() { @@ -256,16 +255,6 @@ function getErrMessage(message, fn) { const line = call.getLineNumber() - 1; let column = call.getColumnNumber() - 1; - // Line number one reports the wrong column due to being wrapped in a - // function. Remove that offset to get the actual call. - if (line === 0) { - if (columnOffset === 0) { - const { wrapper } = require('internal/modules/cjs/loader'); - columnOffset = wrapper[0].length; - } - column -= columnOffset; - } - const identifier = `${filename}${line}${column}`; if (errorCache.has(identifier)) { diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 1a2a91c509b011..2e5290b4520a27 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -1,6 +1,9 @@ 'use strict'; const { validateString } = require('internal/validators'); +const path = require('path'); +const { pathToFileURL } = require('internal/url'); +const { URL } = require('url'); const { CHAR_LINE_FEED, @@ -145,10 +148,18 @@ function addBuiltinLibsToObject(object) { }); } +function normalizeReferrerURL(referrer) { + if (typeof referrer === 'string' && path.isAbsolute(referrer)) { + return pathToFileURL(referrer).href; + } + return new URL(referrer).href; +} + module.exports = exports = { addBuiltinLibsToObject, builtinLibs, makeRequireFunction, + normalizeReferrerURL, requireDepth: 0, stripBOM, stripShebang diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index bc7ecf9f16c7f8..fd592be8ea4c7e 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -29,7 +29,6 @@ const assert = require('internal/assert'); const fs = require('fs'); const internalFS = require('internal/fs/utils'); const path = require('path'); -const { URL } = require('url'); const { internalModuleReadJSON, internalModuleStat @@ -37,6 +36,7 @@ const { const { safeGetenv } = internalBinding('credentials'); const { makeRequireFunction, + normalizeReferrerURL, requireDepth, stripBOM, stripShebang @@ -48,6 +48,7 @@ const experimentalModules = getOptionValue('--experimental-modules'); const manifest = getOptionValue('--experimental-policy') ? require('internal/process/policy').manifest : null; +const { compileFunction } = internalBinding('contextify'); const { ERR_INVALID_ARG_VALUE, @@ -129,15 +130,52 @@ Module._extensions = Object.create(null); var modulePaths = []; Module.globalPaths = []; -Module.wrap = function(script) { +let patched = false; + +// eslint-disable-next-line func-style +let wrap = function(script) { return Module.wrapper[0] + script + Module.wrapper[1]; }; -Module.wrapper = [ +const wrapper = [ '(function (exports, require, module, __filename, __dirname) { ', '\n});' ]; +let wrapperProxy = new Proxy(wrapper, { + set(target, property, value, receiver) { + patched = true; + return Reflect.set(target, property, value, receiver); + }, + + defineProperty(target, property, descriptor) { + patched = true; + return Object.defineProperty(target, property, descriptor); + } +}); + +Object.defineProperty(Module, 'wrap', { + get() { + return wrap; + }, + + set(value) { + patched = true; + wrap = value; + } +}); + +Object.defineProperty(Module, 'wrapper', { + get() { + return wrapperProxy; + }, + + set(value) { + patched = true; + wrapperProxy = value; + } +}); + const debug = util.debuglog('module'); Module._debug = util.deprecate(debug, 'Module._debug is deprecated.', @@ -672,13 +710,6 @@ Module.prototype.require = function(id) { // (needed for setting breakpoint when called with --inspect-brk) var resolvedArgv; -function normalizeReferrerURL(referrer) { - if (typeof referrer === 'string' && path.isAbsolute(referrer)) { - return pathToFileURL(referrer).href; - } - return new URL(referrer).href; -} - // Run the file contents in the correct scope or sandbox. Expose // the correct helper variables (require, module, exports) to @@ -692,13 +723,48 @@ Module.prototype._compile = function(content, filename) { content = stripShebang(content); - const compiledWrapper = vm.compileFunction(content, [ - 'exports', - 'require', - 'module', - '__filename', - '__dirname', - ], { filename }); + let compiledWrapper; + if (patched) { + const wrapper = Module.wrap(content); + compiledWrapper = vm.runInThisContext(wrapper, { + filename, + lineOffset: 0, + displayErrors: true, + importModuleDynamically: experimentalModules ? async (specifier) => { + if (asyncESM === undefined) lazyLoadESM(); + const loader = await asyncESM.loaderPromise; + 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) => { + if (asyncESM === undefined) lazyLoadESM(); + const loader = await asyncESM.loaderPromise; + return loader.import(specifier, normalizeReferrerURL(filename)); + } + }); + } + } var inspectorWrapper = null; if (process._breakFirstLine && process._eval == null) { diff --git a/src/env-inl.h b/src/env-inl.h index b115f9686b7e4d..47331a61954726 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -461,6 +461,9 @@ inline uint32_t Environment::get_next_module_id() { inline uint32_t Environment::get_next_script_id() { return script_id_counter_++; } +inline uint32_t Environment::get_next_function_id() { + return function_id_counter_++; +} Environment::ShouldNotAbortOnUncaughtScope::ShouldNotAbortOnUncaughtScope( Environment* env) diff --git a/src/env.cc b/src/env.cc index 7610f2b7622942..42fe6bdc4b0b0e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -252,6 +252,11 @@ Environment::Environment(IsolateData* isolate_data, isolate()->SetPromiseRejectCallback(task_queue::PromiseRejectCallback); } +CompileFnEntry::CompileFnEntry(Environment* env, uint32_t id) + : env(env), id(id) { + env->compile_fn_entries.insert(this); +} + Environment::~Environment() { isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback( BuildEmbedderGraph, this); @@ -260,6 +265,12 @@ Environment::~Environment() { // CleanupHandles() should have removed all of them. CHECK(file_handle_read_wrap_freelist_.empty()); + // dispose the Persistent references to the compileFunction + // wrappers used in the dynamic import callback + for (auto& entry : compile_fn_entries) { + delete entry; + } + HandleScope handle_scope(isolate()); #if HAVE_INSPECTOR diff --git a/src/env.h b/src/env.h index 773d12d4a1e15d..a8b3b42d7944ed 100644 --- a/src/env.h +++ b/src/env.h @@ -448,6 +448,12 @@ struct ContextInfo { bool is_default = false; }; +struct CompileFnEntry { + Environment* env; + uint32_t id; + CompileFnEntry(Environment* env, uint32_t id); +}; + // Listing the AsyncWrap provider types first enables us to cast directly // from a provider type to a debug category. #define DEBUG_CATEGORY_NAMES(V) \ @@ -718,9 +724,12 @@ class Environment { std::unordered_map id_to_module_map; std::unordered_map id_to_script_map; + std::unordered_set compile_fn_entries; + std::unordered_map> id_to_function_map; inline uint32_t get_next_module_id(); inline uint32_t get_next_script_id(); + inline uint32_t get_next_function_id(); std::unordered_map package_json_cache; @@ -1010,6 +1019,7 @@ class Environment { uint32_t module_id_counter_ = 0; uint32_t script_id_counter_ = 0; + uint32_t function_id_counter_ = 0; AliasedBuffer should_abort_on_uncaught_toggle_; int should_not_abort_scope_counter_ = 0; diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 4414e874ffa697..f642440bff5a88 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -752,6 +752,8 @@ static MaybeLocal ImportModuleDynamically( } else if (type == ScriptType::kModule) { ModuleWrap* wrap = ModuleWrap::GetFromID(env, id); object = wrap->object(); + } else if (type == ScriptType::kFunction) { + object = env->id_to_function_map.find(id)->second.Get(iso); } else { UNREACHABLE(); } diff --git a/src/module_wrap.h b/src/module_wrap.h index 372e02bc5d1b83..dc34685fedadbc 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -20,6 +20,7 @@ enum PackageMainCheck : bool { enum ScriptType : int { kScript, kModule, + kFunction, }; enum HostDefinedOptions : int { diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 343342408b1a75..61f60576d2f2a4 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -285,6 +285,15 @@ void ContextifyContext::WeakCallback( delete context; } +void ContextifyContext::WeakCallbackCompileFn( + const WeakCallbackInfo& data) { + CompileFnEntry* entry = data.GetParameter(); + if (entry->env->compile_fn_entries.erase(entry) != 0) { + entry->env->id_to_function_map.erase(entry->id); + delete entry; + } +} + // static ContextifyContext* ContextifyContext::ContextFromContextifiedSandbox( Environment* env, @@ -1027,7 +1036,30 @@ void ContextifyContext::CompileFunction( data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength()); } - ScriptOrigin origin(filename, line_offset, column_offset, True(isolate)); + // Get the function id + uint32_t id = env->get_next_function_id(); + + // Set host_defined_options + Local host_defined_options = + PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); + host_defined_options->Set( + isolate, + loader::HostDefinedOptions::kType, + Number::New(isolate, loader::ScriptType::kFunction)); + host_defined_options->Set( + isolate, loader::HostDefinedOptions::kID, Number::New(isolate, id)); + + ScriptOrigin origin(filename, + line_offset, // line offset + column_offset, // column offset + True(isolate), // is cross origin + Local(), // script id + Local(), // source map URL + False(isolate), // is opaque (?) + False(isolate), // is WASM + False(isolate), // is ES Module + host_defined_options); + ScriptCompiler::Source source(code, origin, cached_data); ScriptCompiler::CompileOptions options; if (source.GetCachedData() == nullptr) { @@ -1061,38 +1093,45 @@ void ContextifyContext::CompileFunction( } } - MaybeLocal maybe_fun = ScriptCompiler::CompileFunctionInContext( + MaybeLocal maybe_fn = ScriptCompiler::CompileFunctionInContext( parsing_context, &source, params.size(), params.data(), context_extensions.size(), context_extensions.data(), options); - Local fun; - if (maybe_fun.IsEmpty() || !maybe_fun.ToLocal(&fun)) { + if (maybe_fn.IsEmpty()) { DecorateErrorStack(env, try_catch); try_catch.ReThrow(); return; } + Local fn = maybe_fn.ToLocalChecked(); + env->id_to_function_map.emplace(std::piecewise_construct, + std::make_tuple(id), + std::make_tuple(isolate, fn)); + CompileFnEntry* gc_entry = new CompileFnEntry(env, id); + env->id_to_function_map[id].SetWeak(gc_entry, + WeakCallbackCompileFn, + v8::WeakCallbackType::kParameter); if (produce_cached_data) { const std::unique_ptr cached_data( - ScriptCompiler::CreateCodeCacheForFunction(fun)); + ScriptCompiler::CreateCodeCacheForFunction(fn)); bool cached_data_produced = cached_data != nullptr; if (cached_data_produced) { MaybeLocal buf = Buffer::Copy( env, reinterpret_cast(cached_data->data), cached_data->length); - if (fun->Set( + if (fn->Set( parsing_context, env->cached_data_string(), buf.ToLocalChecked()).IsNothing()) return; } - if (fun->Set( + if (fn->Set( parsing_context, env->cached_data_produced_string(), Boolean::New(isolate, cached_data_produced)).IsNothing()) return; } - args.GetReturnValue().Set(fun); + args.GetReturnValue().Set(fn); } diff --git a/src/node_contextify.h b/src/node_contextify.h index 631671cbf1a161..4186e5190f8ef9 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -61,6 +61,8 @@ class ContextifyContext { const v8::FunctionCallbackInfo& args); static void WeakCallback( const v8::WeakCallbackInfo& data); + static void WeakCallbackCompileFn( + const v8::WeakCallbackInfo& data); static void PropertyGetterCallback( v8::Local property, const v8::PropertyCallbackInfo& args); diff --git a/test/fixtures/cjs-module-wrap.js b/test/fixtures/cjs-module-wrap.js new file mode 100644 index 00000000000000..2e11cc1a3b3231 --- /dev/null +++ b/test/fixtures/cjs-module-wrap.js @@ -0,0 +1,9 @@ +const assert = require('assert'); +const m = require('module'); + +global.mwc = 0; +m.wrapper[0] += 'global.mwc = (global.mwc || 0 ) + 1;'; + +require('./not-main-module.js'); +assert.strictEqual(mwc, 1); +delete global.mwc; diff --git a/test/parallel/test-module-wrap.js b/test/parallel/test-module-wrap.js new file mode 100644 index 00000000000000..639300cb6af66a --- /dev/null +++ b/test/parallel/test-module-wrap.js @@ -0,0 +1,9 @@ +'use strict'; +require('../common'); +const fixtures = require('../common/fixtures'); +const { execFileSync } = require('child_process'); + +const cjsModuleWrapTest = fixtures.path('cjs-module-wrap.js'); +const node = process.argv[0]; + +execFileSync(node, [cjsModuleWrapTest], { stdio: 'pipe' }); diff --git a/test/parallel/test-v8-coverage.js b/test/parallel/test-v8-coverage.js index 3db8b81d5504b4..5be68bf4b7947f 100644 --- a/test/parallel/test-v8-coverage.js +++ b/test/parallel/test-v8-coverage.js @@ -27,9 +27,9 @@ function nextdir() { const fixtureCoverage = getFixtureCoverage('basic.js', coverageDirectory); assert.ok(fixtureCoverage); // first branch executed. - assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1); + assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1); // second branch did not execute. - assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0); + assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0); } // Outputs coverage when process.exit(1) exits process. @@ -43,9 +43,9 @@ function nextdir() { const fixtureCoverage = getFixtureCoverage('exit-1.js', coverageDirectory); assert.ok(fixtureCoverage, 'coverage not found for file'); // first branch executed. - assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1); + assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1); // second branch did not execute. - assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0); + assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0); } // Outputs coverage when process.kill(process.pid, "SIGINT"); exits process. @@ -61,9 +61,9 @@ function nextdir() { const fixtureCoverage = getFixtureCoverage('sigint.js', coverageDirectory); assert.ok(fixtureCoverage); // first branch executed. - assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1); + assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1); // second branch did not execute. - assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0); + assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0); } // outputs coverage from subprocess. @@ -78,9 +78,9 @@ function nextdir() { coverageDirectory); assert.ok(fixtureCoverage); // first branch executed. - assert.strictEqual(fixtureCoverage.functions[2].ranges[0].count, 1); + assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1); // second branch did not execute. - assert.strictEqual(fixtureCoverage.functions[2].ranges[1].count, 0); + assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0); } // outputs coverage from worker. @@ -95,9 +95,9 @@ function nextdir() { coverageDirectory); assert.ok(fixtureCoverage); // first branch executed. - assert.strictEqual(fixtureCoverage.functions[2].ranges[0].count, 1); + assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1); // second branch did not execute. - assert.strictEqual(fixtureCoverage.functions[2].ranges[1].count, 0); + assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0); } // Does not output coverage if NODE_V8_COVERAGE is empty. @@ -125,7 +125,7 @@ function nextdir() { coverageDirectory); assert.ok(fixtureCoverage); // first branch executed. - assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1); + assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1); } // Outputs coverage when the coverage directory is not absolute. @@ -144,9 +144,9 @@ function nextdir() { absoluteCoverageDirectory); assert.ok(fixtureCoverage); // first branch executed. - assert.strictEqual(fixtureCoverage.functions[1].ranges[0].count, 1); + assert.strictEqual(fixtureCoverage.functions[0].ranges[0].count, 1); // second branch did not execute. - assert.strictEqual(fixtureCoverage.functions[1].ranges[1].count, 0); + assert.strictEqual(fixtureCoverage.functions[0].ranges[1].count, 0); } // Extracts the coverage object for a given fixture name.