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

vm: vm.compileFunction does not support negative lineOffset and columnOffset #49848

Closed
wmtdru8xip opened this issue Sep 25, 2023 · 2 comments · Fixed by #49855
Closed

vm: vm.compileFunction does not support negative lineOffset and columnOffset #49848

wmtdru8xip opened this issue Sep 25, 2023 · 2 comments · Fixed by #49855
Labels
confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. vm Issues and PRs related to the vm subsystem.

Comments

@wmtdru8xip
Copy link

Version:

v20.6.1; older versions are also affected

Subsystem

vm

Current issue:

The vm.compileFunction method in Node.js currently doesn't support negative lineOffset and columnOffset values.

  1. An error is thrown when the value is negative, as demonstrated below:
let { compileFunction } = require('node:vm');

try {
	compileFunction('', [], { lineOffset: -1 });
} catch (exception) {
	console.log(exception.stack);
}
/*
RangeError [ERR_OUT_OF_RANGE]: The value of 'options.lineOffset' is out of range. It must be >= 0 && <= 4294967295. Received -1
    at internalCompileFunction (node:internal/vm:50:3)
    at Module.compileFunction (node:vm:302:10)
    at ...
*/
  1. An overflow occurs when the value is passed to the V8 engine if it is larger than (2^31)-1, causing it to become -2147483637 with the presence of overflow protection measures:
compileFunction('console.trace()', [], {
	lineOffset: 3456789012,
})();
/*
Trace
    at <anonymous>:-2147483648:9
    at ...
*/

Additional information:

The vm.compileFunction method in Node.js currently doesn't support negative lineOffset and columnOffset values. This is inconsistent with other methods such as vm.runInContext, vm.runInNewContext, and new vm.Script(), which do support negative values for these parameters.

The underlying V8 engine also supports negative values for these parameters as indicated in the ScriptOrigin class:

class V8_EXPORT ScriptOrigin {
 public:
  V8_INLINE ScriptOrigin(Isolate* isolate, Local<Value> resource_name,
                         int resource_line_offset = 0,
                         int resource_column_offset = 0,
                         bool resource_is_shared_cross_origin = false,
                         int script_id = -1,

A common use case for the lineOffset option in vm.compileFunction is to shift lines in error tracebacks to support adding wrappers. Negative values enable the correct line number to be displayed in stack traces when wrapper code is added, while positive values would require part of the code to be removed to get a correct line number, which is not applicable in most scenarios. Hence, negative options are a primary use case that should be supported. An example use case is shown below:

function compileAsyncFunction (code, params, options) {
	if (!params) {
		params = [];
	}
	if (!options) {
		if (!params.join) {
			options = params;
			params = [];
		} else {
			options = {};
		}
	}
	options = {
		...options, lineOffset: (options.lineOffset ?? 0) - 1
	};
	return vm.compileFunction("(async (" + params.join(", ") + ") => {\n" + code + "\n}).apply(null, arguments)", params, options);
}

compileAsyncFunction(scriptFileContentReadFromDisk)();

Possible solution:

Upon investigation, the cause of this issue can be traced back to this line in lib/internal/vm.js:

validateUint32(lineOffset, 'options.lineOffset');

A tested and effective solution is to change the validateUint32 function to validateInt32 for the lineOffset and columnOffset parameters.

@bnoordhuis
Copy link
Member

(1) is a validation bug and probably also responsible for (2), see diff and PR welcome. Other parts of node do seem to be validating it correctly as an int32, just not lib/internal/vm.js.

diff --git a/lib/internal/vm.js b/lib/internal/vm.js
index ba5e232466..111bab8621 100644
--- a/lib/internal/vm.js
+++ b/lib/internal/vm.js
@@ -13,10 +13,10 @@ const {
   validateBoolean,
   validateBuffer,
   validateFunction,
+  validateInt32,
   validateObject,
   validateString,
   validateStringArray,
-  validateUint32,
 } = require('internal/validators');
 const {
   ERR_INVALID_ARG_TYPE,
@@ -46,8 +46,8 @@ function internalCompileFunction(code, params, options) {
   } = options;
 
   validateString(filename, 'options.filename');
-  validateUint32(columnOffset, 'options.columnOffset');
-  validateUint32(lineOffset, 'options.lineOffset');
+  validateInt32(columnOffset, 'options.columnOffset');
+  validateInt32(lineOffset, 'options.lineOffset');
   if (cachedData !== undefined)
     validateBuffer(cachedData, 'options.cachedData');
   validateBoolean(produceCachedData, 'options.produceCachedData');

@bnoordhuis bnoordhuis added confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem. labels Sep 25, 2023
@joyeecheung joyeecheung added the good first issue Issues that are suitable for first-time contributors. label Sep 25, 2023
@MrJithil
Copy link
Member

Created PR.

nodejs-github-bot pushed a commit that referenced this issue Oct 6, 2023
PR-URL: #49855
Fixes: #49848
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
PR-URL: nodejs#49855
Fixes: nodejs#49848
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this issue Nov 11, 2023
PR-URL: #49855
Fixes: #49848
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
joyeecheung pushed a commit to joyeecheung/node that referenced this issue Nov 25, 2023
PR-URL: nodejs#49855
Fixes: nodejs#49848
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
joyeecheung pushed a commit to joyeecheung/node that referenced this issue Dec 1, 2023
PR-URL: nodejs#49855
Fixes: nodejs#49848
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
richardlau pushed a commit that referenced this issue Mar 15, 2024
PR-URL: #49855
Backport-PR-URL: #51004
Fixes: #49848
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
PR-URL: nodejs#49855
Fixes: nodejs#49848
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants