Unable to safely close duplex read/write PTY FDs

[davidfiala]

tldr; Seems impossible to read/write a TTY FD without the risk of FD corruption race conditions.

---

When one has an FD to a TTY/PTY it seems the correct way to read/write is via tty.ReadStream and tty.WriteStream. If your FD is readable and writeable, then you'd need both types of streams since there does not exist anything to my knowledge that makes like a tty.Duplex(fd=...), except net.Socket which as discussed below bans TTYs.

i.e,you want to read/write a TTY FD. You'll need two subclasses of net.Socket, one for reading and one for writing:

  const ptyReadStream = new tty.ReadStream(ptyFd);
  const ptyWriteStream = new tty.WriteStream(ptyFd);

The challenge with this setup is that when you need to tear them down, such as when receiving EIO errno**, the library will automatically close the underlying FD. And thus you need to also destroy the other stream you've created as well because it too is no longer usable. However, that second stream is now pointing to a closed FD.

(** I selected EIO as a common example when dealing with PTYs.)

An even worse problem is that in a race condition situation you'll end up with the second stream happens to now point to a valid FD created very recently by some other code.

1. Option 1: you close an FD wasn't resused: Silent EBADF errno error. Today, Node silently hides this from you.
2. Option 2: Worse: security bug where you read/write from some other FD!

---

Some thoughts:

• If one could tell tty.ReadStream and tty.WriteStream to preserve the underlying FD and never close it under all circumstances, then one could manually manage the lifecycle of closing FDs exactly once.
• You cannot use net.Socket directly for a TTY (which I think might solve the problem because a duplex would close the FD exactly once) because net.Socket bans TTY FDs in this code resulting in TypeError [ERR_INVALID_FD_TYPE]: Unsupported fd type: TTY.
  • Although: Prior discussion on a similar matter suggests that net.Socket might be OK to directly receive TTY FDs, if I read it correctly.

For what it is worth: I think the larger issue here is that we may want to consider ensuring developers can manage the lifecycle of TTY FDs themselves, rather to requiring auto-closing when the Read/Write stream is ended or destroyed.

Open to any help if I've misunderstood my options -- or any ideas to move forward with the corruption risk/FD-double-close.

[davidfiala]

As I investigated using dup(..) as a workaround where both the read and write stream would have their own clone of the same FD, I also noticed a discussion (link) on having FileHandle wrap a naked FD. From a quick read of the code, I believe that if we could wrap a FileHandle around an FD, then immediately net.socket could be used as a solution to this discussion. The reason is that net.Socket only checks if the FD is a TTY if a numeric FD is provided instead of an existing handle. If a FileHandle is provided, the check is bypassed because a constructed handle already exists in the options.

Pro of letting FileHandle wrap an FD:

• Solves this bug if developers bypass the TTY module and use net.Socket directly.
• Solves other use cases that initiated the other discussion thead.

Con:

• The real TTY module remains without a secure duplex option when it's the same FD for read and write. If people want the additional features beyond raw read/write that only TTY module provides, then they perhaps may write insecure code without realizing it.

[bnoordhuis]

You can pass {autoClose: false} to stop node from closing the file descriptor automatically if switching to fs.ReadStream and fs.WriteStream is an option for you.

Otherwise you'd have to look into teaching tty.ReadStream and tty.WriteStream (or net.Socket) about the autoClose option.

There's also an autoDestroy option inherited from the stream module but it's a) probably not what you're looking for, and b) net.Socket always sets it to true.