From 317ae96c3323ad1162a3090d84154af54df3d441 Mon Sep 17 00:00:00 2001 From: Franziska Hinkelmann Date: Thu, 4 Aug 2016 09:36:50 +0200 Subject: [PATCH] src: make EnvDelete behave like the delete operator According to TC39 specification, the delete operator returns false or throws in strict mode, if the property is non-configurable. It returns true in all other cases. Process.env can never have non-configurable properties, thus EnvDelete must always return true. This is independent of strict mode. Fixes: https://github.com/nodejs/node/issues/7960 PR-URL: https://github.com/nodejs/node/pull/7975 Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- src/node.cc | 17 ++++++---------- test/parallel/test-process-env.js | 32 +++++++++++++++++-------------- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/node.cc b/src/node.cc index 64bbfaac48af71..ac9ce4a96c6891 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2774,23 +2774,18 @@ static void EnvQuery(Local property, static void EnvDeleter(Local property, const PropertyCallbackInfo& info) { - bool rc = true; #ifdef __POSIX__ node::Utf8Value key(info.GetIsolate(), property); - rc = getenv(*key) != nullptr; - if (rc) - unsetenv(*key); + unsetenv(*key); #else String::Value key(property); WCHAR* key_ptr = reinterpret_cast(*key); - if (key_ptr[0] == L'=' || !SetEnvironmentVariableW(key_ptr, nullptr)) { - // Deletion failed. Return true if the key wasn't there in the first place, - // false if it is still there. - rc = GetEnvironmentVariableW(key_ptr, nullptr, 0) == 0 && - GetLastError() != ERROR_SUCCESS; - } + SetEnvironmentVariableW(key_ptr, nullptr); #endif - info.GetReturnValue().Set(rc); + + // process.env never has non-configurable properties, so always + // return true like the tc39 delete operator. + info.GetReturnValue().Set(true); } diff --git a/test/parallel/test-process-env.js b/test/parallel/test-process-env.js index 33a1d9674eaa44..52f6de9278f73d 100644 --- a/test/parallel/test-process-env.js +++ b/test/parallel/test-process-env.js @@ -2,11 +2,8 @@ /* eslint-disable max-len */ require('../common'); -// first things first, set the timezone; see tzset(3) -process.env.TZ = 'Europe/Amsterdam'; - -var assert = require('assert'); -var spawn = require('child_process').spawn; +const assert = require('assert'); +const spawn = require('child_process').spawn; /* For the moment we are not going to support setting the timezone via the * environment variables. The problem is that various V8 platform backends @@ -16,6 +13,8 @@ var spawn = require('child_process').spawn; https://github.com/joyent/node/blob/08782931205bc4f6d28102ebc29fd806e8ccdf1f/deps/v8/src/platform-linux.cc#L339-345 https://github.com/joyent/node/blob/08782931205bc4f6d28102ebc29fd806e8ccdf1f/deps/v8/src/platform-win32.cc#L590-596 +// first things first, set the timezone; see tzset(3) +process.env.TZ = 'Europe/Amsterdam'; // time difference between Greenwich and Amsterdam is +2 hours in the summer date = new Date('Fri, 10 Sep 1982 03:15:00 GMT'); @@ -27,28 +26,28 @@ assert.equal(5, date.getHours()); // changes in environment should be visible to child processes if (process.argv[2] == 'you-are-the-child') { // failed assertion results in process exiting with status code 1 - assert.equal(false, 'NODE_PROCESS_ENV_DELETED' in process.env); - assert.equal(42, process.env.NODE_PROCESS_ENV); - assert.equal('asdf', process.env.hasOwnProperty); + assert.strictEqual(false, 'NODE_PROCESS_ENV_DELETED' in process.env); + assert.strictEqual('42', process.env.NODE_PROCESS_ENV); + assert.strictEqual('asdf', process.env.hasOwnProperty); var hasOwnProperty = Object.prototype.hasOwnProperty; const has = hasOwnProperty.call(process.env, 'hasOwnProperty'); - assert.equal(true, has); + assert.strictEqual(true, has); process.exit(0); } else { - assert.equal(Object.prototype.hasOwnProperty, process.env.hasOwnProperty); + assert.strictEqual(Object.prototype.hasOwnProperty, process.env.hasOwnProperty); const has = process.env.hasOwnProperty('hasOwnProperty'); - assert.equal(false, has); + assert.strictEqual(false, has); process.env.hasOwnProperty = 'asdf'; process.env.NODE_PROCESS_ENV = 42; - assert.equal(42, process.env.NODE_PROCESS_ENV); + assert.strictEqual('42', process.env.NODE_PROCESS_ENV); process.env.NODE_PROCESS_ENV_DELETED = 42; - assert.equal(true, 'NODE_PROCESS_ENV_DELETED' in process.env); + assert.strictEqual(true, 'NODE_PROCESS_ENV_DELETED' in process.env); delete process.env.NODE_PROCESS_ENV_DELETED; - assert.equal(false, 'NODE_PROCESS_ENV_DELETED' in process.env); + assert.strictEqual(false, 'NODE_PROCESS_ENV_DELETED' in process.env); var child = spawn(process.argv[0], [process.argv[1], 'you-are-the-child']); child.stdout.on('data', function(data) { console.log(data.toString()); }); @@ -59,3 +58,8 @@ if (process.argv[2] == 'you-are-the-child') { } }); } + +// delete should return true except for non-configurable properties +// https://github.com/nodejs/node/issues/7960 +delete process.env.NON_EXISTING_VARIABLE; +assert.strictEqual(true, delete process.env.NON_EXISTING_VARIABLE);