fix: prevent mangling errno codes for nodefs #14722

cyyynthia · 2021-07-22T00:15:02Z

As described in #14705, errno codes may be looked up dynamically when using nodefs, but closure was mangling the properties making some fs operations behave unexpectedly resulting in undefined behavior.

This PR adds ERRNO_CODES and its properties as node externs, to solve this issue and only affect code size when property names may be needed (witnin a web browser, we will never need plain code names). Also includes a test case to prevent future regressions.

Closes #14705

closes emscripten-core#14705

welcome · 2021-07-22T00:15:04Z

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

sbc100

I'm not sure I understand the issue exactly. Can you point me to where in the nodefs code things go wrong without this change?

sbc100 · 2021-07-22T05:03:47Z

tests/test_core.py

@@ -5202,6 +5202,10 @@ def test_fs_nodefs_home(self):
    self.emcc_args += ['-lnodefs.js']
    self.do_runf(test_file('fs/test_nodefs_home.c'), 'success', js_engines=[config.NODE_JS])

+  def test_fs_nodefs_stat(self):


Given this test is specifically related to closure perhaps it should be named as such? Maybe test_fs_nodefs_errno_closure?

That's a better name for it; I've made the change in abd8d45

Would updating ERRNO_CODES in src/library.js to be defined with strings for keys fix the issue also (or have the same effect)?

I just gave it a try, the test fails and the keys are transformed by Closure

I think its our library system the removes the quotes here, rather than closure. Sadly I can't think of a better way to achieve this, but I worry that this will increase code side a fair bit for programs that need ERRNO_CODE.

I wonder if we can convert a lot of the uses of ERRNO_CODES to use the {{{ cDefine('ENOTTY') }}} pattern and therefore avoid including ERRNO_CODES unless really needed?

Well, the problem is that the code is dynamically returned by Node itself as a property of the error, not minifying the codes is the only way I can think of to allow that dynamic lookup :/

For the code size, I'm unsure how big the real world impact could be, but I doubt the impact would be anything more than a couple bytes, considering I've seen during my Closure-ified code exploration that all references to the ERRNO_CODES enum were inlined; if it wasn't for that dynamic lookup imposed by nodefs.js the object would already get stipped away.

sbc100 · 2021-07-22T05:07:46Z

I'm not sure I understand the issue exactly. Can you point me to where in the nodefs code things go wrong without this change?

Would updating ERRNO_CODES in src/library.js to be defined with strings for keys fix the issue also (or have the same effect)?

cyyynthia · 2021-07-22T08:58:59Z

I'm not sure I understand the issue exactly. Can you point me to where in the nodefs code things go wrong without this change?

When doing some operations (like stat on a non existing file), node throws an error with the errno attached to it as a string. This string is used to lookup the error code in the map (in the convertNodeError method), which fails when the map has been reprocessed by Closure

cyyynthia · 2021-07-22T09:13:32Z

Would updating ERRNO_CODES in src/library.js to be defined with strings for keys fix the issue also (or have the same effect)?

I just gave it a try, the test fails and the keys are transformed by Closure

kripken · 2021-07-22T22:47:06Z

Good find, yes, dynamic name lookups on ERRNO_CODES may break with closure.

After #14730 we will at least only use ERRNO_CODES where we need an actual dynamic lookup. And then I think the proper solution is to quote that object's keys, that is,

ERRNO_CODES = {
  "EAGAIN": ..

Doing it that way instead of as a closure extern is slightly simpler, and better as it will still be able to minify the name of ERRNO_CODES itself.

kripken · 2021-07-22T22:51:53Z

Oh, sorry, I just saw the earlier comments on quoting now.

We should probably fix the quoting in our JS library. But this should also work, to move the object values into a string:

diff --git a/src/library.js b/src/library.js
index dc9566970..36fb829b5 100644
--- a/src/library.js
+++ b/src/library.js
@@ -1550,8 +1550,8 @@ LibraryManager.library = {
   // errno.h
   // ==========================================================================
 
-  $ERRNO_CODES: {
-    EPERM: {{{ cDefine('EPERM') }}},
+  $ERRNO_CODES__postset: `ERRNO_CODES = {
+    "EPERM": {{{ cDefine('EPERM') }}},
     ENOENT: {{{ cDefine('ENOENT') }}},
     ESRCH: {{{ cDefine('ESRCH') }}},
     EINTR: {{{ cDefine('EINTR') }}},
@@ -1672,7 +1672,8 @@ LibraryManager.library = {
     ENOTRECOVERABLE: {{{ cDefine('ENOTRECOVERABLE') }}},
     EOWNERDEAD: {{{ cDefine('EOWNERDEAD') }}},
     ESTRPIPE: {{{ cDefine('ESTRPIPE') }}},
-  },
+  };`,
+  $ERRNO_CODES: {},
   $ERRNO_MESSAGES: {
     0: 'Success',
     {{{ cDefine('EPERM') }}}: 'Not super-user',

sbc100 · 2021-07-23T16:00:45Z

tests/test_core.py

@@ -5202,6 +5202,10 @@ def test_fs_nodefs_home(self):
    self.emcc_args += ['-lnodefs.js']
    self.do_runf(test_file('fs/test_nodefs_home.c'), 'success', js_engines=[config.NODE_JS])

+  def test_fs_nodefs_errno_closure(self):
+    self.emcc_args += ['-lnodefs.js', '--closure=1']
+    self.do_runf(test_file('fs/test_nodefs_errno_closure.c'), 'success', js_engines=[config.NODE_JS])


I wonder if we can get test coverage for this simply by adding self.maybe_closure() to some/all of the exists nodefs tests? (i.e. avoid adding a new test).

BTW, even we do add this new test that way to get closure coverage in the core tests in this file is to add self.maybe_closure() rather than '--closure=1' directly.

Yes it looks like for example you can add self.maybe_closure() to the existing test_fs_nodefs_home test

I'm unsure if test_fs_nodefs_home or any existing test would catch this specific case, as this issue happens only when an error is thrown by Node and it seems all these tests expect no error to happen (and therefore we never need to do that problematic dynamic lookup).

Hmm.. if none of the existing tests are testing error conditions that sounds pretty bad.. maybe find the existing test that uses exercises stat and add some error condition testing to it?

Those existing tests do look quite spotty!

How about you just add a new assert to ./tests/fs/test_nodefs_rw.c ? Just try to open a non-existent file and check the error code?

I've replaced my test case by 2 additional asserts in 99b9306, seems to cover the case nicely

sbc100 · 2021-07-23T18:31:34Z

Thanks!

kripken · 2021-07-26T20:37:16Z

The SIMD errors have been fixed on main meanwhile, and merging in latest should fix them. I think the same is true for the other errors as well, as the new quotes should only be used in NODEFS, NODERAWFS, and PROXYFS.

kripken · 2021-07-26T22:15:46Z

Perfect, thanks @cyyynthia !

fix: prevent mangling errno codes for nodefs

f118bb7

closes emscripten-core#14705

cyyynthia added 2 commits July 22, 2021 02:26

fix: forgot var keyword

feb9294

fix: supress duplicate errors

76b2696

sbc100 reviewed Jul 22, 2021

View reviewed changes

chore: rename test to a better name

abd8d45

fix: prevent mangling with quoted object keys

b401c12

sbc100 reviewed Jul 23, 2021

View reviewed changes

cyyynthia added 3 commits July 23, 2021 18:36

fix: use maybe_closure rather than hardcoded --closure=1

199cdb7

fix: wrong edit (coffee was required)

a84b7e2

fix: reuse existing tests

99b9306

sbc100 approved these changes Jul 23, 2021

View reviewed changes

Merge branch 'emscripten-core:main' into fix/14705

f026b59

kripken merged commit 88c9073 into emscripten-core:main Jul 26, 2021

kripken mentioned this pull request Aug 2, 2021

Possible regression in 2.0.26. now seeing error 'TypeError: Cannot read property 'ENOENT' of undefined' #14783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent mangling errno codes for nodefs #14722

fix: prevent mangling errno codes for nodefs #14722

cyyynthia commented Jul 22, 2021

welcome bot commented Jul 22, 2021

sbc100 left a comment

sbc100 Jul 22, 2021

cyyynthia Jul 22, 2021

sbc100 Jul 22, 2021

cyyynthia Jul 22, 2021 •

edited

Loading

sbc100 commented Jul 22, 2021

cyyynthia commented Jul 22, 2021 •

edited

Loading

cyyynthia commented Jul 22, 2021

kripken commented Jul 22, 2021 •

edited

Loading

kripken commented Jul 22, 2021

sbc100 Jul 23, 2021

sbc100 Jul 23, 2021

sbc100 Jul 23, 2021

cyyynthia Jul 23, 2021

sbc100 Jul 23, 2021

sbc100 Jul 23, 2021

cyyynthia Jul 23, 2021

sbc100 commented Jul 23, 2021

kripken commented Jul 26, 2021

kripken commented Jul 26, 2021

fix: prevent mangling errno codes for nodefs #14722

fix: prevent mangling errno codes for nodefs #14722

Conversation

cyyynthia commented Jul 22, 2021

welcome bot commented Jul 22, 2021

sbc100 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

cyyynthia Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

sbc100 commented Jul 22, 2021

cyyynthia commented Jul 22, 2021 • edited Loading

cyyynthia commented Jul 22, 2021

kripken commented Jul 22, 2021 • edited Loading

kripken commented Jul 22, 2021

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

sbc100 commented Jul 23, 2021

kripken commented Jul 26, 2021

kripken commented Jul 26, 2021

cyyynthia Jul 22, 2021 •

edited

Loading

cyyynthia commented Jul 22, 2021 •

edited

Loading

kripken commented Jul 22, 2021 •

edited

Loading