Use span instead of Electron.relaunch to avoid restrictions in the new process#296711
Use span instead of Electron.relaunch to avoid restrictions in the new process#296711
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the Electron main-process relaunch flow to avoid Electron’s app.relaunch() behavior on Linux that results in relaunched VS Code instances inheriting PR_SET_NO_NEW_PRIVS (breaking sudo in integrated terminals), matching the behavior of a normal OS-level launch.
Changes:
- Use
child_process.spawn(process.execPath, args, …)for relaunch on Linux instead ofelectron.app.relaunch({ args }). - Keep
electron.app.relaunch()for macOS/Windows relaunch behavior unchanged. - Add Linux-specific tracing and inline rationale referencing issue #253204.
| // sudo from working in VS Code terminals (https://github.com/microsoft/vscode/issues/253204). | ||
| // Spawn the new process via Node.js which does not set this flag. | ||
| this.trace('Lifecycle#relaunch() - calling spawn()'); | ||
| const child = spawn(process.execPath, args, { detached: true, stdio: 'ignore' }); |
There was a problem hiding this comment.
spawn() can emit an 'error' event (e.g. ENOENT if process.execPath is temporarily unavailable during an upgrade). Without an error listener, that becomes an unhandled 'error' event and can crash the process during quit. Consider adding a child.on('error', …) handler to log the failure and (optionally) fall back to electron.app.relaunch({ args }) or otherwise prevent an unhandled exception.
| const child = spawn(process.execPath, args, { detached: true, stdio: 'ignore' }); | |
| const child = spawn(process.execPath, args, { detached: true, stdio: 'ignore' }); | |
| child.on('error', error => { | |
| this.logService.error('[Lifecycle#relaunch] Failed to spawn new process', error); | |
| this.trace('Lifecycle#relaunch() - spawn() failed, falling back to app.relaunch()'); | |
| try { | |
| electron.app.relaunch({ args }); | |
| } catch (fallbackError) { | |
| this.logService.error('[Lifecycle#relaunch] Failed to relaunch app after spawn error', fallbackError); | |
| } | |
| }); |
| electron.app.relaunch({ args }); | ||
| if (isLinux) { | ||
| // On Linux, Electron's app.relaunch() uses Chromium's base::LaunchProcess() | ||
| // which sets PR_SET_NO_NEW_PRIVS on the new process by default, preventing |
There was a problem hiding this comment.
Not true, the relaunched program is allowed privileges https://github.com/electron/electron/blob/aca83afeef7cdebce10c6fa9835baed26c806b9f/shell/browser/relauncher_linux.cc#L63
So what needs checking is,
- The relaunch path will shutdown the current app gracefully and relaunch self executable with a special commandline that will cause a different main function to be invoked https://github.com/electron/electron/blob/aca83afeef7cdebce10c6fa9835baed26c806b9f/shell/browser/relauncher.cc#L117
options.allow_new_privs = falsein here - This new main function https://github.com/electron/electron/blob/aca83afeef7cdebce10c6fa9835baed26c806b9f/shell/browser/relauncher.cc#L160 will then proceed to launch the executable with provided arguments
options.allow_new_privs = truehere
If the final application process is still getting PR_SET_NO_NEW_PRIVS then it is very likely inherited from the relauncher process, lets confirm this and get a proper fix into the relaunch code of Electron. Relying on node.js process launch will complicate shutdown sequence which we get for free from the relaunch api.
There was a problem hiding this comment.
Yep,' it's in draft as I haven't been able to test any of it yet.
Fixes #253204