All uncaughtException and unhandledRejection listeners are unexpectedly getting removed, including ones attached by other code

CherryDT commented 4 years ago

Description

When libsodium-wrappers is initialized, all uncaughtException and unhandledRejection listeners are getting removed. This means that other parts of the code get disabled if somewhere libsodium happens to be used!

The offending code is here:

https://github.com/jedisct1/libsodium.js/blob/8fa117918df38f52668feaf9cbc498c633671855/wrapper/libsodium-post.js#L2-L3

(In my case, crash logging in my application suddenly stopped working after adding support for the cryptocurrency Tezos, which turned out to be because the library sotez which I use to interact with the Tezos blockchain uses libsodium-wrappers as dependency.)

Reproduction

Try the following code:

process.on('unhandledRejection', (reason, promise) => {
  console.log('AAAAA', reason)
})

process.on('uncaughtException', (err, origin) => {
  console.error('BBBBB', err)
  process.exit(1)
})

;(async () => {
  await require('libsodium-wrappers').ready // <<< THIS MAKES THE HANDLERS BREAK
  Promise.reject('HALP')
  setTimeout(() => {
    throw new Error('OH NOEZ')
  }, 1000)
})()

Expected output: (custom handlers fired just fine)

AAAAA HALP
BBBBB Error: OH NOEZ
    at Timeout._onTimeout (/tmp/_tmp.js:14:11)
    at listOnTimeout (internal/timers.js:549:17)
    at processTimers (internal/timers.js:492:7)

Actual output: (no custom handlers fired)

(node:19737) UnhandledPromiseRejectionWarning: HALP
(node:19737) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:19737) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
/tmp/_tmp.js:14
    throw new Error('OH NOEZ')
    ^

Error: OH NOEZ
    at Timeout._onTimeout (/tmp/_tmp.js:14:11)
    at listOnTimeout (internal/timers.js:549:17)
    at processTimers (internal/timers.js:492:7)

Now, remove the line await require('libsodium-wrappers').ready and try again. This time, the expected output will appear.

Environment

Version of this library used: 0.7.8
node.js version: 12.16.3

Comments and proposed solution

I am aware of issue #177 but it was closed a year ago and the problem still exists, so I want to raise awareness of this problem, as I believe it requires addressing. I don't understand why it was closed without action because at least there should be a big warning on top of the readme that using this library will break unrelated code in your project, and additionally a warning in a preinstall script so that when a dependency installs this library (like sotez did for me), there is at least indication in the console while the install is running that a source of bugs has been introduced into your code that you need to be aware of.

But, actually, I think it would be better to just not remove unrelated listeners at all but instead prevent the listeners from getting added in the first place. There is a number of ways to do that and I don't understand your project well enough to know what works best in all targeted environments, but my ideas would be as follows (from "cleanest" to "dirtiest"):

You could modify sodium.js in during your build process and remove the calls that attach those listeners in the first place.
You could wrap the code and pass it a modified version of process. It seems only process.on, process.versions and process.argv get accessed, so it should work to add this line at the end of libsodium_pre.js:
```
(function (process) {
```
...and these lines at the start of libsodium_post.js:
```
})({
    on (event, listener) {
        if (event === 'uncaughtException' || event === 'unhandledRejection') return;
        return process.on(event, listener);
    },
    versions: process.versions,
    argv: process.argv
});
```
You could hook process.on entirely and replace it by a function that drops attempts to attach the problematic listeners if they come from the offending parts of the code (by checking the call stack in new Error().stack).

Workaround

As a workaround, I'm currently using the third solution, because I can apply this one from the outside - I'm hooking process.on and drop all uncaughtException and unhandledRejection listener attachment attempts made by code inside /node_modules/libsodium/, as well as the corresponding calls to process.removeAllListeners from that code:

// Insert this code before first loading libsodium-wrappers
for (const fn of ['on', 'removeAllListeners']) {
  const oldFn = process[fn]
  process[fn] = function (event) {
    const caller = new Error().stack.split('\n')[2]
    if (['uncaughtException', 'unhandledRejection'].includes(event) && caller.includes('/node_modules/libsodium/')) {
      return
    }
    return oldFn.apply(this, arguments)
  }
}

jedisct1 commented 4 years ago

You understand the problem and how to solve it way better than I do.

Could you submit a PR to fix it?

Thanks a lot for your help!

CherryDT commented 4 years ago

OK see #254 - however I was unfortunately unable to test it because I had trouble getting your makefile to work (I got stuck at emcc: error: Invalid command line option -s RUNNING_JS_OPTS=1: Fastcomp cared about running JS which could alter asm.js validation, but not upstream). That's why I didn't originally submit a PR, I feel I don't know this project enough. Can you please run it in your environment and see if it works?

svvac commented 3 years ago

Any progress on this?

svvac commented 3 years ago

Also, I found these emscripten settings that might be relevant here (ref emscripten-core/emscripten#5957), rather than mocking process to prevent the problematic calls.

jedisct1 commented 3 years ago

I think these settings are unrelated to Javascript and refer to C++ exceptions.

svvac commented 3 years ago

According to shell.js, NODEJS_CATCH_EXIT and NODEJS_CATCH_EXCEPTION seem to control the presence of process.on('uncaughtException', ...) and process.on('unhandledRejection', ...)

CherryDT commented 3 years ago

This looks promising, of course this would be a much better solution than my workaround.

I'm not sure how to implement it though, I don't know the project well enough, so I won't make a PR for it

svvac commented 3 years ago

I think (I have no experience with emscripten prior to this discussion) the flags should be set in libsodium/dist-build/emscripten.sh, and if it indeed works the process.removeAllListeners() calls should be removed as in the original PR

svvac commented 3 years ago

Alright libsodium#1025 together with #265 seem to fix unwanted process tampering.

Keep in mind that I barely know what I'm doing here so I might have overlooked something.

CherryDT commented 3 years ago

OK, I updated #254 accordingly so it just removes the listener removal code

jedisct1 commented 3 years ago

Very cool, thanks!

Have you tried these changes? Do they fix the issue?

svvac commented 3 years ago

With my dev build (did not try to run all variants, though they should behave similarly), custom handlers for unhandledRejection do not get removed by loading the library.

svvac commented 3 years ago

@jedisct1 Is there a release schedule for NPM modules?

jedisct1 commented 3 years ago

I can cut a new release now if you confirm that the currently generated files work as expected :)

svvac commented 3 years ago

Alright I made a few test scripts, ran them against 0.7.8@npm, then replaced node_modules/libsodium/dist/libsodium.js and node_modules/libsodium-wrappers/dist/libsodium-wrappers.js with the compiled versions in 88928899b6289ff1a519235fb434ce3d6bd98f68.

Unhandled rejection handlers after `libsodium.ready`

process.on('unhandledRejection', (e) => console.log('unhandledRejection', e));
process.on('exit', (code) => console.log('process.exit', code));
const sodium = require('libsodium-wrappers');
(async () => {
    await sodium.ready;
    throw new Error('test');
})();

Without libsodium (we remove the import and dont await on ready)

Custom handler is called

$ node unhandledrejection.js
unhandledRejection Error: test
    at /tmp/testsodium/unhandledrejection.js:6:11
    at Object. (/tmp/testsodium/unhandledrejection.js:7:3)
    at Module._compile (internal/modules/cjs/loader.js:1015:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1035:10)
    at Module.load (internal/modules/cjs/loader.js:879:32)
    at Function.Module._load (internal/modules/cjs/loader.js:724:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47
process.exit 0

Against 0.7.8

Custom handler has been cleared ; node spits out its deprecation warnings then exits.

$ node unhandledrejection.js
(node:1240901) UnhandledPromiseRejectionWarning: Error: test
    at /tmp/testsodium/unhandledrejection.js:6:11
(node:1240901) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:1240901) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
process.exit 0

Against 88928899b6289ff1a519235fb434ce3d6bd98f68

Custom handler is called

$ node unhandledrejection.js
unhandledRejection Error: test
    at /tmp/testsodium/unhandledrejection.js:6:11
process.exit 0

Operational errors still work as expected

process.on('exit', (code) => console.log('process.exit', code));
const sodium = require('libsodium-wrappers');
(async () => {
    await sodium.ready;
    try {
        const buf = sodium.crypto_secretbox_open_easy(
            sodium.randombytes_buf(1024),
            sodium.randombytes_buf(sodium.crypto_secretbox_NONCEBYTES),
            sodium.randombytes_buf(sodium.crypto_secretbox_KEYBYTES),
        );
    } catch (err) {
        console.error('could not get buffer:', err);
    }
})();

Against 0.7.8

Error is thrown and caught as expected

$ node randomsecretbox.js
could not get buffer: Error: wrong secret key for the given ciphertext
    at g (/tmp/testsodium/node_modules_release/libsodium-wrappers/dist/modules/libsodium-wrappers.js:1:16941)
    at Object.ct [as crypto_secretbox_open_easy] (/tmp/testsodium/node_modules_release/libsodium-wrappers/dist/modules/libsodium-wrappers.js:1:57224)
    at /tmp/testsodium/randomsecretbox.js:6:28
process.exit 0

Against 88928899b6289ff1a519235fb434ce3d6bd98f68

Error is thrown and caught as expected

$ node randomsecretbox.js
could not get buffer: Error: wrong secret key for the given ciphertext
    at g (/tmp/testsodium/node_modules_fix/libsodium-wrappers/dist/modules/libsodium-wrappers.js:1:17549)
    at Object.Tr [as crypto_secretbox_open_easy] (/tmp/testsodium/node_modules_fix/libsodium-wrappers/dist/modules/libsodium-wrappers.js:1:64207)
    at /tmp/testsodium/randomsecretbox.js:6:28
process.exit 0

Looks good on my end

jedisct1 / libsodium.js