Skip to content

Mirror entire environment under node #18820

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Mirror entire environment under node #18820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ See docs/process.md for more on how version tagging works.
3.1.33 (in development)
-----------------------
- Update SDL2_ttf port to 2.20.2 (#18804)
- When running under node the full set of environment variables (i.e.
process.env) are now reflected to running emscripten process (via e.g.
getenv). If you don't want this behaviour `-sDETERMINISTIC` can be used
to disable this can give fake envinonment instead.

3.1.32 - 02/17/23
-----------------
Expand Down
14 changes: 10 additions & 4 deletions src/library_wasi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ var WasiLibrary = {
$getEnvStrings: function() {
if (!getEnvStrings.strings) {
// Default values.
#if !DETERMINISTIC
// Browser language detection #8751
var lang = ((typeof navigator == 'object' && navigator.languages && navigator.languages[0]) || 'C').replace('-', '_') + '.UTF-8';
#else
#if DETERMINISTIC
// Deterministic language detection, ignore the browser's language.
var lang = 'C.UTF-8';
#else
// Browser language detection #8751
var lang = ((typeof navigator == 'object' && navigator.languages && navigator.languages[0]) || 'C').replace('-', '_') + '.UTF-8';
#endif
var env = {
'USER': 'web_user',
Expand All @@ -58,6 +58,12 @@ var WasiLibrary = {
'LANG': lang,
'_': getExecutableName()
};
#if ENVIRONMENT_MAY_BE_NODE && !DETERMINISTIC
if (ENVIRONMENT_IS_NODE) {
// Under node we mirror then entire outer environment
env = process.env;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is breaking change, I think.
For example, there could be OS shell variable which would point to files which do not exist on VFS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible yes. Should we prune the environment? Or perhaps only do this when NODERAWFS is used (i.e. when all paths would be valid)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, given that folks can still override with using the ENV global in JS, do you think it would be a problem in practice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also a little worried about breaking changes here.

Comparing this to the filesystem, by default we are sandboxed, and only use minimal things from the environment as makes sense (or no things, in DETERMINISTIC mode). The user can opt into NODEFS or NODERAWFS to get direct/unsandboxed access to the outside. Maybe something similar makes sense for the environment?

If that direction sounds relevant, I'm not sure what's best but options could include a new option, or adding a new "node direct" option (for both this and files), or perhaps bundling with NODE[RAW]FS somehow.

Copy link
Collaborator Author

@sbc100 sbc100 Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about in the default case.. should certain env vars be allowed to pass though by default? e.g. LANG and LC_* were specifically requested in #18816. Is there any reason to not pass those ones through?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think specific ones might be ok on a case by case basis.

}
#endif
// Apply the user-provided values, if any.
for (var x in ENV) {
// x is a key in ENV; if ENV[x] is undefined, that means it was
Expand Down
20 changes: 16 additions & 4 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7282,21 +7282,33 @@ def test_override_js_execution_environment(self):
seen = self.run_js('test.js', engine=engine, assert_returncode=NON_ZERO)
self.assertContained('Module.ENVIRONMENT has been deprecated. To force the environment, use the ENVIRONMENT compile-time option (for example, -sENVIRONMENT=web or -sENVIRONMENT=node', seen)

@requires_node
@with_env_modify({'FOO': 'bar'})
def test_node_environ(self):
create_file('src.c', r'''
#include <stdlib.h>
#include <stdio.h>
int main() {
printf("|%s|\n", getenv("FOO"));
}
''')
self.do_runf('src.c', '|bar|')

def test_override_c_environ(self):
create_file('pre.js', r'''
var Module = {
preRun: [function() { ENV.hello = 'world'; ENV.LANG = undefined; }]
};
''')
create_file('src.cpp', r'''
create_file('src.c', r'''
#include <stdlib.h>
#include <stdio.h>
int main() {
printf("|%s|\n", getenv("hello"));
printf("LANG is %s\n", getenv("LANG") ? "set" : "not set");
}
''')
self.run_process([EMXX, 'src.cpp', '--pre-js', 'pre.js'])
self.run_process([EMCC, 'src.c', '--pre-js', 'pre.js'])
output = self.run_js('a.out.js')
self.assertContained('|world|', output)
self.assertContained('LANG is not set', output)
Expand All @@ -7306,10 +7318,10 @@ def test_override_c_environ(self):
preRun: [function(module) { module.ENV.hello = 'world' }]
};
''')
self.run_process([EMXX, 'src.cpp', '--pre-js', 'pre.js', '-sEXPORTED_RUNTIME_METHODS=ENV'])
self.run_process([EMCC, 'src.c', '--pre-js', 'pre.js', '-sEXPORTED_RUNTIME_METHODS=ENV'])
self.assertContained('|world|', self.run_js('a.out.js'))

self.run_process([EMXX, 'src.cpp', '--pre-js', 'pre.js', '-sEXPORTED_RUNTIME_METHODS=ENV', '-sMODULARIZE'])
self.run_process([EMCC, 'src.c', '--pre-js', 'pre.js', '-sEXPORTED_RUNTIME_METHODS=ENV', '-sMODULARIZE'])
output = self.run_process(config.NODE_JS + ['-e', 'require("./a.out.js")();'], stdout=PIPE, stderr=PIPE)
self.assertContained('|world|', output.stdout)

Expand Down