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

Big Sur hardened runtime versus fork yields large slowdown in node-pty.spawn() #476

Closed
starpit opened this issue May 30, 2021 · 5 comments · Fixed by #487
Closed

Big Sur hardened runtime versus fork yields large slowdown in node-pty.spawn() #476

starpit opened this issue May 30, 2021 · 5 comments · Fixed by #487
Assignees
Labels
enhancement help wanted Issues identified as good community contribution opportunities

Comments

@starpit
Copy link

starpit commented May 30, 2021

Environment details

  • OS: macOS
  • OS version: Big Sur 11.4
  • node-pty version: 0.10.1

Issue description

When running node-pty.spawn() from within a macOS "hardened runtime" context, it appears that the fork() POSIX API is very slow [1] [2]. The referenced issues discuss the use of posix_spawn() versus fork(), and I see that node-pty's pty.cc uses fork().

This only seems to affect the combination of macOS 11 and the use of the macOS hardened runtime. You get this whenever signing node-pty.

We are seeing that each spawn call takes about 300ms, which is at least 100x slower than in other combinations.

[1] libuv/libuv#3050
[2] electron/electron#26143

@starpit starpit changed the title Big Sur hardened runtime versus fork Big Sur hardened runtime versus fork yields large slowdown in node-pty.spawn() May 30, 2021
@jerch
Copy link
Collaborator

jerch commented May 31, 2021

I think we dont use any fork/exec specialties and should be able to transit to posix_spawn.

Things to watch out for though are:

  • signal handlers: We currently stop them before forking, and reactivate them afterwards. No clue yet, how posix_spawn deals with that. Ideally it works likewise.
  • custom privileges: We allow the PTY session leader to be spawned under custom user rights if done as root. Not sure how this works with posix_spawn. (Idk if that feature is used by anyone at all, never saw an issue or request regarding this.)
  • plattform support: I suggest to do the transition only for Big Sur+, and leave other POSIX platforms untouched. This creates a bit more noise on code level, but is safer than a full transition. Thought being POSIX specced idk if there are semantic differences for posix_spawn on different platforms, a full transition would need much more testing.

@Tyriar
Copy link
Member

Tyriar commented Jun 1, 2021

We did hit this in VS Code and I think may have worked around it with a patch. It's an upstream issue in electron/libuv which should get fixed eventually so I'm not sure its worth the effort to try fix.

@deepak1556
Copy link
Contributor

@Tyriar the patch in vscode only handles the calls done to libuv for process spawn which currently is from child_process module in nodejs. Since node-pty manages its own process spawn implementation, the transition to posix_spawn family must also be done here separately.

  1. posix_spawnattr_setsigdefault and posix_spawnattr_setsigmask can help achieve the changes in block signals during forking #218
  2. posix_spawnattr_set_uid_np, posix_spawnattr_set_gid_np can help with it
  3. I suggest to do the transition only for Big Sur+, and leave other POSIX platforms untouched

Agree with this, the libuv patch does the same. I will take a stab at this in July if it hasn't been covered.

@jerch
Copy link
Collaborator

jerch commented Jun 1, 2021

@deepak1556 Just a wild idea - does libuv-spawn expose enough primitives to use it instead of any own fork/spawn logic? I think all we need for a PTY process attach is the functionality of login_tty swapping the std channels, a setsid call and the TIOCSCTTY ioctl (resp. their macos counterparts). If thats not directly possible, it could be done by a small helper exec'ing into the final binary.

Not sure if there can be won anything, at least it sounds intriguing to me to delegate the low level process spawning to a well maintained lib node-pty depends on anyway.

@Eugeny
Copy link
Contributor

Eugeny commented Aug 7, 2021

Added a PoC implementation in #486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted Issues identified as good community contribution opportunities
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants