From a639b18a9d4f070ec9046b9af68344c1d503249e Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 28 Dec 2020 12:20:05 +0100 Subject: [PATCH 1/4] process: use frozen array for process.allowedNodeEnvironmentFlags Maintain no-op calls of `Set` prototype methods to `process.allowedNodeEnvironmentFlags` for backward compatibility. --- doc/api/process.md | 14 +-- lib/internal/process/per_thread.js | 99 +++++++++---------- .../test-process-env-allowed-flags.js | 37 +++++-- 3 files changed, 85 insertions(+), 65 deletions(-) diff --git a/doc/api/process.md b/doc/api/process.md index 773686354efd8a..4cb18fb81797b9 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -577,15 +577,15 @@ This feature is not available in [`Worker`][] threads. added: v10.10.0 --> -* {Set} +* {string[]} The `process.allowedNodeEnvironmentFlags` property is a special, -read-only `Set` of flags allowable within the [`NODE_OPTIONS`][] +read-only `Array` of flags allowable within the [`NODE_OPTIONS`][] environment variable. -`process.allowedNodeEnvironmentFlags` extends `Set`, but overrides -`Set.prototype.has` to recognize several different possible flag -representations. `process.allowedNodeEnvironmentFlags.has()` will +`process.allowedNodeEnvironmentFlags` is a frozen `Array`, but overrides +`Array.prototype.includes` to recognize several different possible flag +representations. `process.allowedNodeEnvironmentFlags.includes()` will return `true` in the following cases: * Flags may omit leading single (`-`) or double (`--`) dashes; e.g., @@ -613,10 +613,6 @@ process.allowedNodeEnvironmentFlags.forEach((flag) => { }); ``` -The methods `add()`, `clear()`, and `delete()` of -`process.allowedNodeEnvironmentFlags` do nothing, and will fail -silently. - If Node.js was compiled *without* [`NODE_OPTIONS`][] support (shown in [`process.config`][]), `process.allowedNodeEnvironmentFlags` will contain what *would have* been allowable. diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index 04da1a862ca7c0..81d7505e65777b 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -6,17 +6,18 @@ const { ArrayPrototypeEvery, + ArrayPrototypeIncludes, ArrayPrototypeMap, ArrayPrototypePush, ArrayPrototypeSplice, BigUint64Array, Float64Array, + FunctionPrototype, NumberMAX_SAFE_INTEGER, - ObjectDefineProperty, ObjectFreeze, ReflectApply, + ReflectGet, RegExpPrototypeTest, - SafeSet, StringPrototypeEndsWith, StringPrototypeReplace, StringPrototypeSlice, @@ -293,55 +294,53 @@ function buildAllowedFlags() { // Save these for comparison against flags provided to // process.allowedNodeEnvironmentFlags.has() which lack leading dashes. - const nodeFlags = new SafeSet(ArrayPrototypeMap(allowedNodeEnvironmentFlags, - trimLeadingDashes)); - - class NodeEnvironmentFlagsSet extends SafeSet { - constructor(...args) { - super(...args); - - // The super constructor consumes `add`, but - // disallow any future adds. - ObjectDefineProperty(this, 'add', { - value: () => this - }); - } - - delete() { - // No-op, `Set` API compatible - return false; - } - - clear() { - // No-op - } - - has(key) { - // This will return `true` based on various possible - // permutations of a flag, including present/missing leading - // dash(es) and/or underscores-for-dashes. - // Strips any values after `=`, inclusive. - // TODO(addaleax): It might be more flexible to run the option parser - // on a dummy option set and see whether it rejects the argument or - // not. - if (typeof key === 'string') { - key = StringPrototypeReplace(key, replaceUnderscoresRegex, '-'); - if (RegExpPrototypeTest(leadingDashesRegex, key)) { - key = StringPrototypeReplace(key, trailingValuesRegex, ''); - return super.has(key); - } - return nodeFlags.has(key); + const nodeFlags = + ArrayPrototypeMap(allowedNodeEnvironmentFlags, trimLeadingDashes); + + return new Proxy(ObjectFreeze(allowedNodeEnvironmentFlags), { + get(target, key, receiver) { + switch (key) { + case 'add': + // No-op, `Set` API compatible + return () => receiver; + + case 'delete': + // No-op, `Set` API compatible + return () => false; + + case 'clear': + // No-op, `Set` API compatible + return FunctionPrototype; + + case 'has': + case 'includes': + return (key) => { + // This will return `true` based on various possible + // permutations of a flag, including present/missing leading + // dash(es) and/or underscores-for-dashes. + // Strips any values after `=`, inclusive. + // TODO(addaleax): It might be more flexible to run the option + // parser on a dummy option set and see whether it rejects the + // argument or not. + if (typeof key === 'string') { + key = StringPrototypeReplace(key, replaceUnderscoresRegex, '-'); + if (RegExpPrototypeTest(leadingDashesRegex, key)) { + key = StringPrototypeReplace(key, trailingValuesRegex, ''); + return ArrayPrototypeIncludes(target, key); + } + return ArrayPrototypeIncludes(nodeFlags, key); + } + return false; + }; + + case 'size': + return target.length; + + default: + return ReflectGet(target, key); } - return false; - } - } - - ObjectFreeze(NodeEnvironmentFlagsSet.prototype.constructor); - ObjectFreeze(NodeEnvironmentFlagsSet.prototype); - - return ObjectFreeze(new NodeEnvironmentFlagsSet( - allowedNodeEnvironmentFlags - )); + }, + }); } // Lazy load internal/trace_events_async_hooks only if the async_hooks diff --git a/test/parallel/test-process-env-allowed-flags.js b/test/parallel/test-process-env-allowed-flags.js index 21beb5357a5f65..14df469ce1074b 100644 --- a/test/parallel/test-process-env-allowed-flags.js +++ b/test/parallel/test-process-env-allowed-flags.js @@ -1,6 +1,6 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); // Assert legit flags are allowed, and bogus flags are disallowed @@ -63,14 +63,39 @@ const assert = require('assert'); process.allowedNodeEnvironmentFlags.add('foo'); assert.strictEqual(process.allowedNodeEnvironmentFlags.has('foo'), false); - process.allowedNodeEnvironmentFlags.forEach((flag) => { - assert.strictEqual(flag === 'foo', false); - }); - process.allowedNodeEnvironmentFlags.clear(); - assert.strictEqual(process.allowedNodeEnvironmentFlags.size > 0, true); + const thisArg = {}; + process.allowedNodeEnvironmentFlags.forEach( + common.mustCallAtLeast(function(flag, _, set) { + assert.notStrictEqual(flag, 'foo'); + assert.strictEqual(this, thisArg); + assert.strictEqual(set, process.allowedNodeEnvironmentFlags); + }), + thisArg + ); + + for (const flag of process.allowedNodeEnvironmentFlags.keys()) { + assert.notStrictEqual(flag, 'foo'); + } + for (const flag of process.allowedNodeEnvironmentFlags.values()) { + assert.notStrictEqual(flag, 'foo'); + } + for (const flag of process.allowedNodeEnvironmentFlags) { + assert.notStrictEqual(flag, 'foo'); + } + for (const [flag] of process.allowedNodeEnvironmentFlags.entries()) { + assert.notStrictEqual(flag, 'foo'); + } const size = process.allowedNodeEnvironmentFlags.size; + + process.allowedNodeEnvironmentFlags.clear(); + assert.strictEqual(process.allowedNodeEnvironmentFlags.size, size); + process.allowedNodeEnvironmentFlags.delete('-r'); assert.strictEqual(process.allowedNodeEnvironmentFlags.size, size); + + assert.throws(() => process.allowedNodeEnvironmentFlags.splice(0), + /Cannot delete property/); + assert.strictEqual(process.allowedNodeEnvironmentFlags.size, size); } From c8cfc415631674ca7f31de2cf2b697cb0cf8f392 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 1 Feb 2021 18:27:48 +0100 Subject: [PATCH 2/4] fixup! process: use frozen array for process.allowedNodeEnvironmentFlags --- test/parallel/test-process-env-allowed-flags.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-process-env-allowed-flags.js b/test/parallel/test-process-env-allowed-flags.js index 14df469ce1074b..f4b457c04bfd12 100644 --- a/test/parallel/test-process-env-allowed-flags.js +++ b/test/parallel/test-process-env-allowed-flags.js @@ -62,14 +62,23 @@ const assert = require('assert'); true); process.allowedNodeEnvironmentFlags.add('foo'); + assert.throws( + () => Set.prototype.add.call(process.allowedNodeEnvironmentFlags, 'foo'), + /Set\.prototype\.add called on incompatible receiver/); assert.strictEqual(process.allowedNodeEnvironmentFlags.has('foo'), false); + assert.throws( + () => process.allowedNodeEnvironmentFlags.push('foo'), + /object is not extensible/); + assert.strictEqual(process.allowedNodeEnvironmentFlags.includes('foo'), + false); + const thisArg = {}; process.allowedNodeEnvironmentFlags.forEach( - common.mustCallAtLeast(function(flag, _, set) { + common.mustCallAtLeast(function(flag, _, arr) { assert.notStrictEqual(flag, 'foo'); assert.strictEqual(this, thisArg); - assert.strictEqual(set, process.allowedNodeEnvironmentFlags); + assert.strictEqual(arr, process.allowedNodeEnvironmentFlags); }), thisArg ); @@ -89,6 +98,8 @@ const assert = require('assert'); const size = process.allowedNodeEnvironmentFlags.size; + assert.strictEqual(process.allowedNodeEnvironmentFlags.length, size); + process.allowedNodeEnvironmentFlags.clear(); assert.strictEqual(process.allowedNodeEnvironmentFlags.size, size); From 77be7cf4dcb313c4c366c2eeedbe2ade5c9fbb45 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 1 Feb 2021 18:39:15 +0100 Subject: [PATCH 3/4] fixup! process: use frozen array for process.allowedNodeEnvironmentFlags --- test/parallel/test-process-env-allowed-flags.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-process-env-allowed-flags.js b/test/parallel/test-process-env-allowed-flags.js index f4b457c04bfd12..5675400b08c22e 100644 --- a/test/parallel/test-process-env-allowed-flags.js +++ b/test/parallel/test-process-env-allowed-flags.js @@ -70,8 +70,15 @@ const assert = require('assert'); assert.throws( () => process.allowedNodeEnvironmentFlags.push('foo'), /object is not extensible/); - assert.strictEqual(process.allowedNodeEnvironmentFlags.includes('foo'), - false); + assert.throws( + () => Array.prototype.push.call(process.allowedNodeEnvironmentFlags, 'foo'), + /object is not extensible/); + assert.strictEqual( + process.allowedNodeEnvironmentFlags.includes('foo'), + false); + assert.strictEqual( + Array.prototype.includes.call(process.allowedNodeEnvironmentFlags, 'foo'), + false); const thisArg = {}; process.allowedNodeEnvironmentFlags.forEach( From 28e58b433cfaa2ba26fdc211c963ab0e9ae5b852 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 1 Feb 2021 19:19:13 +0100 Subject: [PATCH 4/4] fixup! process: use frozen array for process.allowedNodeEnvironmentFlags --- doc/api/process.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/api/process.md b/doc/api/process.md index 4cb18fb81797b9..4b9c86c8ce7921 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -575,6 +575,10 @@ This feature is not available in [`Worker`][] threads. ## `process.allowedNodeEnvironmentFlags` * {string[]}