src: make EnvDelete behave like the delete operator

fhinkel · cjihrig · commit 317ae96c3323 · 2016-08-10T12:47:31.000-04:00
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: #7960 PR-URL: #7975 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
diff --git a/src/node.cc b/src/node.cc
@@ -2774,23 +2774,18 @@ static void EnvQuery(Local<String> property,
 
 static void EnvDeleter(Local<String> property,
                        const PropertyCallbackInfo<Boolean>& 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<WCHAR*>(*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
@@ -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);
