💖 Thanks for opening this pull request! 💖

Semantic PR titles

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Commit signing

This repo enforces commit signatures for all incoming PRs.
To sign your commits, see GitHub's documentation on Telling Git about your signing key.

PR tips

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@ckerr As explained in this issue #44881 , null device can be disabled on windows. Node doesn't handle this situation and invokes crash when trying to initialize stdio using nul device, which makes all electron apps crash definitely when nul device is disabled on user's machine.

As far as I know, nul device is part of the posix standand and will casue the os to run abnormally if disabled on macos and linux. Also, not like windows gui apps, where stdio is not associated with an underlying stream, on macos and linux, stdio usually has a valid fd and the code to initialize stdio in node will usually not be called. So I think it's both ok to use or not to use kNoStdioInitialization on macos and linux, no other potential problems.

I remember this PR being raised before but it is no longer available when I search, rewriting some comments I had from there.

1. What are the side effects of using kNoStdioInitialization specifically related to handle inheritance for child process when we attach to console via https://source.chromium.org/chromium/chromium/src/+/main:base/process/launch_win.cc;l=167-234, with the flag we will now miss this call https://github.com/nodejs/node/blob/5ef498517574e1534041cdd25c920f8d0b2cf845/src/node.cc#L819-L823
2. We also use nul device to support the ignore option on child_process and utilityProcess.fork api. When the device is not available the child process may not be created and the applications functionality will be broken, should we still continue execution in these cases ?

1. Without kNoStdioInitialization, stdio inheritance is disabled, then the child process will call AttachConsole(ATTACH_PARENT_PROCESS) to attach its stdio to the parent process's stdio. With kNoStdioInitialization, the child process will inherit the stdio of parent process. I think inheriting stdio works the same as attaching parent's console.

2. I also find that utility process is not constructed completely and the app crash later once using utility process without nul device. When failing to open nul device in utilityProcess.fork, I try to create a pipe, close it's read end, and use it's write end as a substitute for nul device. The app works normally with this method.
   But such kind of pipe (WriteFile fails) is not the same with nul device (WriteFile succeeds), so I have another idea: Once nul device service of the operating system is disabled, we could create a nul device service in our application. We could create a pipe, then read and drop strings in our nul device service (maybe as a thread in our process), and the write end of the pipe will work the same as nul device. I'm stilling considering if it's necessary to do such a heavy thing.

What's your motive for disabling the null device? Honestly you should expect disabling basic drivers would break the system in unexpected ways.

@brjsp In my personal view, I really don’t want the null device to be disabled. However, according to our statistics, around 0.1% windows machines have disabled null device. Without this issue fixed we will lose 0.1% users when upgrading electron to the new version. This is indeed a serious number.

I have tested adequately that the null device service can be safely disabled and the windows system will not break. I am also really confused about why Microsoft has implemented such a feature.

why Microsoft has implemented such a feature.

“If i delete ntoskrnl.exe, the system won't boot. I'm confused about why Microsoft has implemented such feature!”

sorry but this PR seems to be a bad-faith effort to disable security features in an effort to make the code “compatible” with intentionally broken installations.

I think disabling null devices is different from deleting core system files. As mentioned in the Issue, #44881 we can disable null devices through a registry setting - this is really a Windows feature. After disabling null devices, most software can still run normally. However, if system core files are deleted, the system cannot run.
“To reproduce this issue, we can just set Start = 4 (which is 1 normally) in registry key HKEY_LOCAL_MACHINE\SYSTEM\ControlSet001\Services\Null
”

Since this condition is so simple, malicious attackers could potentially crash all Electron applications with just one registry command - this is practically a vulnerability. If there is a security application built with Electron, an attacker will be able to crash this security app by setting this registry key, then the attacker can do everything he wants.
reg add "HKEY_LOCAL_MACHINE\SYSTEM\ControlSet001\Services\Null" /v start /t REG_DWORD /d 4 /f

Additionally, as mentioned above, some users have indeed configured this registry setting. Although we don't know why they chose to do so, but the user set this register is so many. Just like what was mentioned above, about 0.1% of the Windows machines among our users are affected. Although this data was only collected during one of our upgrades and it might be a bit high.

I think it's better to add some compatibility to allow more users to enjoy Electron applications.

we can disable null devices through a registry setting - this is really a Windows feature.

If you want to limit yourself to disabling services, then disable pci and your system won't boot. There are really no measures in place to prevent the administrator from shooting themself in the head.

Since this condition is so simple, malicious attackers could potentially crash all Electron applications with just one registry command - this is practically a vulnerability.

Even if it's a compatibility/usability issue, what it's definitely not is a vulnerability. Malicious attackers cannot write anything under HKLM\System. If you can modify services configuration, you could just add a new service that runs the code you want to run.

I don't think it is appropriate compare the nul device to core components that can cause the windows system to fail to boot.

I have disabled nul deivce on my windows computer and used it for servel days. The windows system has been functioning normally, and I haven't found any other application that crashes except for electron.

I think the nul device service is only an optional sercive that can be disabled safely on windows, although we have not fully figured out in which specific scenarios the nul device service will be disabled (I haven't found much official material describing windows nul device, and I hope anyone can find and share some authoritative information).

I think that what we should executly discuss here is what is the best way to solve this issue.

To be honest, I do not agree that my code is elegant to fix it by adding kNoStdioInitialization. I have a new idea, would it be suitable to just ignore the error of opening a nul device and continue executing? Does anyone have any opinions on this?

I have found this relevant code segment in the chromium project. When failing to open a nul device, maybe it would be ok to just ignore the error to avoid crashes ? FuzzerUtilWindows.cpp DiscardOutput

void DiscardOutput(int Fd) {
  FILE* Temp = fopen("nul", "w");
  if (!Temp)
    return;
  _dup2(_fileno(Temp), Fd);
  fclose(Temp);
}

Sorry for the delay, based on your testing given the application does break when either utilityprocess or childprocess attempts to use the nul device, ignoring the early exit from Node.js in these cases doesn't help much. But again not all use cases of the child process api require the nul device, so it cannot made into a startup check as well.

In my mind, this situation runs similar to how the application will fail to start when sandbox cannot be used on linux. But only in this case, we don't have enough feedback from the application to let users know why the application failed to launch and maybe provide an opt-out mechanism. Can this PR try the following,

1. Add a clear error message when nul device cannot be used
2. Add support for a feature flag --no-stdio-init which will
   a) enable kNoStdioInitialization for Node.js environment
3. Utility process should emit error event when nul device cannot be used.

@deepak1556 Great! I will revise my PR based on your suggestions.

@deepak1556 I have added the feature flag --no-stdio-init
When nul device disabled, launch electron without this flag will print a log to remind the user.

Then electron launches successfully with this flag on.

Also, I added Emit("error", "Failed to create null handle for ignoring stdio"); in UtilityProcessWrapper::UtilityProcessWrapper, but I encountered two problems.

1. I failed to find a way to handle this error event. I’m not sure if it suitable to emit error event here.
   
2. Still the user has no option but to use ignore in utilityProcess.fork
   This demo trying not to open a nul device will throw an exception stdin value other than ignore is not supported.

const { app, utilityProcess } = require('electron');
const path = require('node:path');

const child = utilityProcess.fork(path.join(__dirname, 'test.js'), {
    stdio: ['inherit', 'inherit', 'inherit']
});

It seems to compel the use of ignore here.

Sorry for the delay here, I missed the notification on this PR. I will review it by Monday.

@deepak1556 Hi, do you have any new opinions about my code?

Hi @deepak1556
I'm handing over this PR to @zuohuiyang .
All review feedback has been addressed in the new PR: fix: fix launch crash when null device is disabled on Windows by zuohuiyang · Pull Request #47870 · electron/electron
Please continue in the new PR.
This PR can be closed.
Thanks!

Closing in favor of #47870