Close specified fd may cause a double free

[zyscoder]

• Version: v16.0.0-pre
• Platform: Linux 5.8.0-38-generic The binary and long term compatibility with node #43~20.04.1-Ubuntu SMP Tue Jan 12 16:39:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: fs.close/fs.closeSync

What steps will reproduce the bug?

Setup a node instance,

» node

and run the following javascript code.

fs.closeSync(8);
// fs.close(8, (err)=>{}) works too.
process.exit();

Then the node instance occurs an abort.
If invoking fs.closeSync(8) twice, then a "bad file descriptor" error message would be alert.
However, when exiting the process with process.exit(), an abort occurs. I'm not sure if any other way to trigger this problem.
This issue is almost the same as #37874, but since a double-free may have been triggered, maybe security risk should be considered.
Feel free to close this issue if you think nothing important.

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

This abort can always be triggered following the steps above.

What is the expected behavior?

If any error occurs, an exception or other similar error-reporting stuff should be thrown. There is no reason to abort the whole node process.

What do you see instead?

» node
Welcome to Node.js v16.0.0-pre.
Type ".help" for more information.
> fs.closeSync(8);
undefined
> process.exit()
[1]    95516 abort (core dumped)  /home/zys/Toolchains/node/node                                                                                                                                                                      

Additional information

[XadillaX]

I think we should gather all the fds that are not created by exposed Node.js API and then protect them from closeing.

[jasnell]

/cc @addaleax

I'd flip that a bit @XadillaX ... instead of protecting fd's that are not opened by the current Node.js process we should track and handle those that have been opened by the current process. I believe @addaleax has suggested extending the tracked fd capability that she implemented for worker threads to also cover fds opened in the main thread.

[addaleax]

Yeah, #37874 (comment) would be my suggestion here. I'd also suggest closing this and #38377 as duplicates of that issue, there's no real point in tracking the behavior for different file descriptors separately.

[XadillaX]

Yeah, #37874 (comment) would be my suggestion here. I'd also suggest closing this and #38377 as duplicates of that issue, there's no real point in tracking the behavior for different file descriptors separately.

/cc @jasnell

I mean we should trace the fd like epoll eventfd / async handle fd, etc. And throw errors when we try to use those fds in Node.js userland manually.

[jasnell]

Closing this as a duplicate of #37874.