process: EnvGetter to use libuv #14641

refack · 2017-08-05T12:44:28Z

Fixes: #14593
Refs: Maximus5/ConEmu#1209

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

process

refack · 2017-08-05T12:49:19Z

Context: MSDN recommends checking for ERROR_ENVVAR_NOT_FOUND when return value is zero.

If the function fails, the return value is zero. If the specified environment variable was
not found in the environment block, GetLastError returns ERROR_ENVVAR_NOT_FOUND.

refack · 2017-08-05T13:04:54Z

Not ready yet, sorry

payload · 2017-09-06T11:51:26Z

src/node.cc

-  // not found.
-  if ((result > 0 || GetLastError() == ERROR_SUCCESS) &&
-      result < arraysize(buffer)) {
+  // happen. If GetLastError() == ERROR_SUCCESS the variable was not found.


comment says "== ERROR_SUCCESS" and means "ERROR_ENVVAR_NOT_FOUND"

hoodie · 2017-09-14T14:52:31Z

Is there any chance of this landing in the next 8.x release or even a 8.5.1?

cjihrig · 2017-09-14T14:57:06Z

@refack FYI, there is a uv_os_getenv() that you may be able to use.

HipsterZipster · 2017-10-27T02:31:34Z

Facing this issue now. Did this get released yet? If so, which version?

proProbe · 2017-11-18T08:48:12Z

I am also facing this right now. Any updates? :)

refack · 2017-11-18T18:25:04Z

I'll pick this up again.

tniessen

Mostly LGTM. Does this still fix the referenced issue?

tniessen · 2017-11-18T19:05:49Z

src/node.cc

-    const uint16_t* two_byte_buffer = reinterpret_cast<const uint16_t*>(buffer);
-    Local<String> rc = String::NewFromTwoByte(isolate, two_byte_buffer);
-    return info.GetReturnValue().Set(rc);
+  char buffer[32767];


Could you keep the comment about how this value was chosen?

tniessen · 2017-11-18T19:11:51Z

src/node.cc

-    if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 ||
-        GetLastError() == ERROR_SUCCESS) {
+    GetEnvironmentVariableW(key_ptr, nullptr, 0);
+    if (GetLastError() != ERROR_ENVVAR_NOT_FOUND) {


Is it safe to ignore the return value of GetEnvironmentVariableW?

That was the working assumption at the time. But I just missed this part of the patch, I want to use uv_os_getenv here as well.

uv_os_getenv() will return UV_ENOENT in this case instead of ERROR_ENVVAR_NOT_FOUND.

cjihrig · 2017-11-21T14:19:48Z

src/node.cc

-    Local<String> rc = String::NewFromTwoByte(isolate, two_byte_buffer);
-    return info.GetReturnValue().Set(rc);
+  char buffer[32767];
+  int ret = uv_os_getenv(*key, buffer, arraysize(buffer));


Shouldn't the final argument just be sizeof(buffer)?

After reading the manual I figured it should be 🤦‍♂️

size_t buf_size = sizeof(buffer); uv_os_getenv(*key, buffer, &buf_size);

cjihrig · 2017-11-21T14:21:15Z

src/node.cc

-    if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 ||
-        GetLastError() == ERROR_SUCCESS) {
+    GetEnvironmentVariableW(key_ptr, nullptr, 0);
+    if (GetLastError() != ERROR_ENVVAR_NOT_FOUND) {


uv_os_getenv() will return UV_ENOENT in this case instead of ERROR_ENVVAR_NOT_FOUND.

cjihrig · 2017-11-21T15:59:51Z

src/node.cc

+  // On POSIX there is not explicitly defined size limit, but on Windows
+  // environment variables have a maximum size limit of 2**15 - 1.
+  char buffer[32768];
+  size_t buf_size = arraysize(buffer);


Still arraysize() 😄

I thought I had woken up. Apparently not, so I need more coffee.

cjihrig · 2017-11-21T16:00:15Z

src/node.cc

-    WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
-    GetEnvironmentVariableW(key_ptr, nullptr, 0);
-    if (GetLastError() != ERROR_ENVVAR_NOT_FOUND) {
+    // We are only interested in exsistance, so we can keep the buffer small.


exsistance -> existence

cjihrig · 2017-11-21T16:00:27Z

src/node.cc

-    if (GetLastError() != ERROR_ENVVAR_NOT_FOUND) {
+    // We are only interested in exsistance, so we can keep the buffer small.
+    char buffer[256];
+    size_t = sizeof(buffer);


Does this compile?

Well obviously not (which means my way of locally compiling is broken. I was trying to not compile everything 'cause Windows)

refack · 2017-11-21T22:14:49Z

Fails 1 test on windows with:

not ok 300 parallel/test-os
  ---
  duration_ms: 0.147
  severity: fail
  stack: |-
    assert.js:42
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: '/temp' === '/tmp'
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-os.js:52:10)
        at Module._compile (module.js:644:30)
        at Object.Module._extensions..js (module.js:655:10)
        at Module.load (module.js:563:32)
        at tryModuleLoad (module.js:506:12)
        at Function.Module._load (module.js:498:3)
        at Function.Module.runMain (module.js:685:10)
        at startup (bootstrap_node.js:192:16)
        at bootstrap_node.js:627:3
  ...

cjihrig · 2017-11-22T15:18:44Z

src/node.cc

-    if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 ||
-        GetLastError() == ERROR_SUCCESS) {
+    // We are only interested in existence, so we can keep the buffer small.
+    char buffer[256];


If you're only interested in existence, couldn't you go much smaller than 256?

refack · 2017-11-23T15:41:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11670/

So there may be a bug in the test, it's using an undefined behaviour to delete variables (env.VAR = ''):

node/test/parallel/test-os.js

Lines 50 to 55 in 283b949

    
           assert.strictEqual(os.tmpdir(), '/temp'); 
        
           process.env.TEMP = ''; 
        
           assert.strictEqual(os.tmpdir(), '/tmp'); 
        
           process.env.TMP = ''; 
        
           const expected = `${process.env.SystemRoot || process.env.windir}\\temp`; 
        
           assert.strictEqual(os.tmpdir(), expected);

I'm wondering is we should fix the test, or turn env.VAR = '' into an actual env var deletion?

@cjihrig @nodejs/testing PTAL

addaleax · 2017-11-23T15:50:25Z

it's using an undefined behaviour to delete variables (env.VAR = '')

Can you explain why that is undefined behaviour?

addaleax

Hm – what about setenv? It would be nice to have consistency on that side as well, right?

refack · 2017-11-23T16:40:29Z

It's undefined as currently (pre this change), setting an env var to '' gave inconsistent behaviour:

> process.env
{ ... TEMP: 'c:\\temp\\usr', ... }
> process.env.TEMP = ''
''
> process.env
{ ... TEMP: undefined, ... }
> process.env.TEMP
''

while the actual value (via WIN32 api is ''):

AFAIK that's why we have a note to that effect in the docs:

node/doc/api/process.md

Line 897 in 088bba3

Use `delete` to delete a property from `process.env`.

The bug behind this PR is that in some cases the value is stringified to 'undefined'.

refack · 2017-11-23T16:43:18Z

I'll try to write a failing test
Moving to uv_os_getenv is a good idea. I'll check if moving is easy.

cjihrig · 2017-11-24T02:50:35Z

So there may be a bug in the test, it's using an undefined behaviour to delete variables (env.VAR = '')

Is there any reason that delete process.env.VAR and process.env.VAR = '' shouldn't accomplish the same thing in the context of that test? All it's trying to do is manipulate the || operations here.

bnoordhuis

Needs work and should be checked carefully for performance regressions.

bnoordhuis · 2017-11-24T11:37:38Z

src/node.cc

+  // environment variables have a maximum size limit of 2**15 - 1.
+  char buffer[32768];
+  size_t buf_size = sizeof(buffer);
+  int ret = uv_os_getenv(*key, buffer, &buf_size);


You should retry with a bigger buffer when ret == UV_ENOBUFS, otherwise you introduce an arbitrary size restriction on Unices that wasn't there before.

(And also, can you rename it to err for consistency?)

bnoordhuis · 2017-11-24T11:41:08Z

src/node.cc

+    char buffer[256];
+    size_t buf_size = sizeof(buffer);
+    int ret = uv_os_getenv(*key, buffer, &buf_size);
+    if (ret != UV_ENOENT) {


Reads uninitialized memory when ret == UV_ENOBUFS (i.e., buffer too small.)

bnoordhuis · 2017-11-24T11:42:00Z

src/node.cc

      rc = 0;
-      if (key_ptr[0] == L'=') {
+#ifdef _WIN32
+      if (key[0] == L'=') {


Just '=', not L'='.

BridgeAR · 2017-12-05T23:40:05Z

Ping @refack

BridgeAR · 2018-01-12T15:37:39Z

Closing due to long inactivity. Please feel free to reopen.

refack added process Issues and PRs related to the process subsystem. windows Issues and PRs related to the Windows platform. labels Aug 5, 2017

refack requested a review from bnoordhuis August 5, 2017 12:44

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 5, 2017

refack self-assigned this Aug 5, 2017

refack requested a review from tniessen August 5, 2017 12:45

refack added the wip Issues and PRs that are still a work in progress. label Aug 5, 2017

refack closed this Aug 5, 2017

refack mentioned this pull request Aug 9, 2017

Passing empty environment variables to child processes convert to 'undefined' using ConEmu + Node #14593

Closed

payload reviewed Sep 6, 2017

View reviewed changes

process: EnvGetter to use libuv

2968191

refack reopened this Nov 18, 2017

refack force-pushed the more-complaint-GetEnvironmentVariableW branch from 6a02a6a to 2968191 Compare November 18, 2017 18:55

refack changed the title ~~process: more complaint GetEnvironmentVariableW~~ process: EnvGetter to use libuv Nov 18, 2017

tniessen reviewed Nov 18, 2017

View reviewed changes

cjihrig reviewed Nov 21, 2017

View reviewed changes

[sqwsh] correct usage of uv_os_getenv & implement in EnvQuery

a3464e6

cjihrig reviewed Nov 21, 2017

View reviewed changes

[sqwsh] typos & syntax errors

ead9c39

refack force-pushed the more-complaint-GetEnvironmentVariableW branch from c529f55 to ead9c39 Compare November 21, 2017 16:07

[sqwsh] on boy

a23bdb5

cjihrig reviewed Nov 22, 2017

View reviewed changes

refack added libuv Issues and PRs related to the libuv dependency or the uv binding. and removed wip Issues and PRs that are still a work in progress. labels Nov 23, 2017

addaleax reviewed Nov 23, 2017

View reviewed changes

bnoordhuis reviewed Nov 24, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

BridgeAR closed this Jan 12, 2018

refack removed their assignment Oct 12, 2018

joyeecheung mentioned this pull request Apr 13, 2019

Refactor RealEnvStore methods to use uv_os_getenv/setenv/unsetenv #27211

Closed

process: EnvGetter to use libuv #14641

process: EnvGetter to use libuv #14641

Conversation

refack commented Aug 5, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Aug 5, 2017

refack commented Aug 5, 2017

Choose a reason for hiding this comment

hoodie commented Sep 14, 2017

cjihrig commented Sep 14, 2017

HipsterZipster commented Oct 27, 2017

proProbe commented Nov 18, 2017

refack commented Nov 18, 2017

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Nov 21, 2017

Choose a reason for hiding this comment

refack commented Nov 23, 2017

addaleax commented Nov 23, 2017

addaleax left a comment

Choose a reason for hiding this comment

refack commented Nov 23, 2017

refack commented Nov 23, 2017

cjihrig commented Nov 24, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Dec 5, 2017

BridgeAR commented Jan 12, 2018

refack commented Aug 5, 2017 •

edited

Loading