Skip to content
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.

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

Segmentation fault when using inspector on experimental permission enabled process #53385

Closed
thezzisu opened this issue Jun 8, 2024 · 12 comments · Fixed by #53389
Closed

Segmentation fault when using inspector on experimental permission enabled process #53385

thezzisu opened this issue Jun 8, 2024 · 12 comments · Fixed by #53389
Labels
inspector Issues and PRs related to the V8 inspector protocol permission Issues and PRs related to the Permission Model

Comments

@thezzisu
Copy link

thezzisu commented Jun 8, 2024

Version

v22.2.0

Platform

Linux AS-ZISU 5.15.146.1-microsoft-standard-WSL2 #1 SMP Thu Jan 11 04:09:03 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

> cat index.mjs
import {setTimeout} from 'node:timers/promises'
await setTimeout(1000000)

> node --experimental-permission --allow-fs-read=\* --inspect-brk index.mjs
(node:398141) ExperimentalWarning: Permission is an experimental feature
(Use `node --trace-warnings ...` to show where the warning was created)
[1]    398141 segmentation fault  node --experimental-permission --allow-fs-read=\* --inspect-brk index.mjs

The content of the script does not matter - even an empty script will work.

How often does it reproduce? Is there a required condition?

No condition required. Node v22.2.0 will crash everytime when --experimental-permission is combined with --inspect-brk.

What is the expected behavior? Why is that the expected behavior?

The inspector should work and do not segfault.

What do you see instead?

The node process crash because of segfault. Also, no stack trace is printed.

Additional information

The experimental process-based permission model have unexpected side effects with inspector, which is not well-documented.

When enabled, --inspect and SIGUSR1 both wouldn't work. Should this be a feature, or another bug?

@targos
Copy link
Member

targos commented Jun 8, 2024

I don't know how to debug C++ code, but here's some info:

Process 84595 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x58)
    frame #0: 0x00000001004f4d78 node`std::__1::__hash_table<std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>, std::__1::__unordered_map_hasher<int, std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>, std::__1::hash<int>, std::__1::equal_to<int>, true>, std::__1::__unordered_map_equal<int, std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>, std::__1::equal_to<int>, std::__1::hash<int>, true>, std::__1::allocator<std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>>>::begin(this=0x0000000000000048) at __hash_table:1561:35
   1558	typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
   1559	__hash_table<_Tp, _Hash, _Equal, _Alloc>::begin() _NOEXCEPT
   1560	{
-> 1561	    return iterator(__p1_.first().__next_);
   1562	}
   1563
   1564	template <class _Tp, class _Hash, class _Equal, class _Alloc>
Target 0: (node) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x58)
  * frame #0: 0x00000001004f4d78 node`std::__1::__hash_table<std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>, std::__1::__unordered_map_hasher<int, std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>, std::__1::hash<int>, std::__1::equal_to<int>, true>, std::__1::__unordered_map_equal<int, std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>, std::__1::equal_to<int>, std::__1::hash<int>, true>, std::__1::allocator<std::__1::__hash_value_type<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>>>::begin(this=0x0000000000000048) at __hash_table:1561:35
    frame #1: 0x00000001004f4b94 node`std::__1::unordered_map<int, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>, std::__1::hash<int>, std::__1::equal_to<int>, std::__1::allocator<std::__1::pair<int const, std::__1::unique_ptr<node::inspector::(anonymous namespace)::ChannelImpl, std::__1::default_delete<node::inspector::(anonymous namespace)::ChannelImpl>>>>>::begin[abi:un170006](this=0x0000000000000048 size=0) at unordered_map:1297:62
    frame #2: 0x00000001004f1e5c node`node::inspector::NodeInspectorClient::schedulePauseOnNextStatement(this=0x0000000000000000, reason="Break on start") at inspector_agent.cc:609:33
    frame #3: 0x00000001004f1e28 node`node::inspector::Agent::PauseOnNextJavascriptStatement(this=0x0000600003ab8b00, reason="Break on start") at inspector_agent.cc:892:12
    frame #4: 0x000000010051a3f8 node`node::inspector::(anonymous namespace)::CallAndPauseOnStart(args=0x000000016fdf5938) at inspector_js_api.cc:187:27

@targos
Copy link
Member

targos commented Jun 8, 2024

@nodejs/inspector

@VoltrexKeyva VoltrexKeyva added inspector Issues and PRs related to the V8 inspector protocol permission Issues and PRs related to the Permission Model labels Jun 8, 2024
@eugeneo
Copy link
Contributor

eugeneo commented Jun 8, 2024

I confirmed the problem, will look into it, assigning to myself.

@eugeneo
Copy link
Contributor

eugeneo commented Jun 8, 2024

Oh, can't assign, no longer a contributor :) Feel free to assign to me.

@theanarkh
Copy link
Contributor

I have made a PR to fix it.

@theanarkh
Copy link
Contributor

I think this is because setting --experimental-permission flag will disable inspector(experimental_permission,
should_create_inspector), then calling callAndPauseOnStart during startup make the process crash.

@benjamingr
Copy link
Member

Oh, can't assign, no longer a contributor :) Feel free to assign to me.

Kind reminder that collaborators removed due to inactivity are more than welcome to come back and many have done so in the past :)

@cola119
Copy link
Member

cola119 commented Jun 10, 2024

When enabled, --inspect and SIGUSR1 both wouldn't work. Should this be a feature, or another bug?

@RafaelGSS According to RafaelGSS@34d92ed, the inspector feature is restricted when the permission model is enabled, and there is no flag to enable it, correct?

@RafaelGSS
Copy link
Member

That's correct. We haven't included a --allow-inspector flag as it's pretty much the same as disabling the permission model. There are some ways to bypass --allow-fs restrictions through inspector protocol, for instance. But, I'm happy to discuss adding this cli option on nodejs. Feel free to open an issue on security-wg repository.

@eugeneo
Copy link
Contributor

eugeneo commented Jun 10, 2024

There used to be discussions with some cloud providers about optionally restricting some Inspector domains to make some operations impossible. E.g. we could disable Debugger/Runtime but leave Tracing/Profile. It should be fairly doable from a technical point of view, though I expect there would be a game of wack-a-mole with some functionality still providing too much power...

Main concern was the worry that most frontends would not expect the domains being unavailable, making this functionality mostly useless.

Feel free to initiate a discussion if there's more interest in this.

nodejs-github-bot pushed a commit that referenced this issue Jun 10, 2024
PR-URL: #53389
Fixes: #53385
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
@skypesky
Copy link

When I used pm2(node version: v22.3.0) + --experimental-permission, I ran into the following error:

2024-06-14T08:32:35: node:inspector:59

2024-06-14T08:32:35:     this.#connection = new Connection((message) => this.#onMessage(message));

2024-06-14T08:32:35:                        ^

2024-06-14T08:32:35: 

2024-06-14T08:32:35: Error: Access to this API has been restricted

2024-06-14T08:32:35:     at Session.connect (node:inspector:59:24)

2024-06-14T08:32:35:     at InspectorService.init (/Users/skypesky/workSpaces/javascript/arcblock/blocklet-server/node_modules/@pm2/io/build/main/services/inspector.js:14:22)

2024-06-14T08:32:35:     at PMX.init (/Users/skypesky/workSpaces/javascript/arcblock/blocklet-server/node_modules/@pm2/io/build/main/pmx.js:72:30)

2024-06-14T08:32:35:     at Object.<anonymous> (/Users/skypesky/workSpaces/javascript/arcblock/blocklet-server/node_modules/@pm2/io/build/main/index.js:5:65)

2024-06-14T08:32:35:     at Module._compile (node:internal/modules/cjs/loader:1460:14)

2024-06-14T08:32:35:     at Module._extensions..js (node:internal/modules/cjs/loader:1544:10)

2024-06-14T08:32:35:     at Module.load (node:internal/modules/cjs/loader:1275:32)

2024-06-14T08:32:35:     at Module._load (node:internal/modules/cjs/loader:1091:12)

2024-06-14T08:32:35:     at wrapModuleLoad (node:internal/modules/cjs/loader:212:19)

2024-06-14T08:32:35:     at Module.require (node:internal/modules/cjs/loader:1297:12) {

2024-06-14T08:32:35:   code: 'ERR_ACCESS_DENIED',

2024-06-14T08:32:35:   permission: 'Inspector',

2024-06-14T08:32:35:   resource: 'Connect'

2024-06-14T08:32:35: }

2024-06-14T08:32:35: 

2024-06-14T08:32:35: Node.js v22.3.0

The error seems to be localized at:

image

The only parameters I provide to node.js are:
image

@theanarkh
Copy link
Contributor

You can not use inspector when run on permission mode.

targos pushed a commit that referenced this issue Jun 20, 2024
PR-URL: #53389
Fixes: #53385
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53389
Fixes: nodejs#53385
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53389
Fixes: nodejs#53385
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53389
Fixes: #53385
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53389
Fixes: #53385
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol permission Issues and PRs related to the Permission Model
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants