src: remap invalid file descriptors using dup2

[obiwac]

When checking for the validity of the stdio file descriptors in PlatformInit(), this was the code previously used (as in #875):

// Make sure file descriptors 0-2 are valid before we start logging anything.
for (int fd = STDIN_FILENO; fd <= STDERR_FILENO; fd += 1) {
  struct stat ignored;
  if (fstat(fd, &ignored) == 0)
    continue;
  // Anything but EBADF means something is seriously wrong.  We don't
  // have to special-case EINTR, fstat() is not interruptible.
  if (errno != EBADF)
    abort();
  if (fd != open("/dev/null", O_RDWR))
    abort();
}

As I understand it, the intention behind this is to map file descriptors to /dev/null if they don't exist, by hoping the next file descriptor to be given will be the one which doesn't exist. This is imperfect as it is very platform dependent and breaks when /dev/null has already been given a different file descriptor in the same process, but e.g. stdin has disappeared in the meantime (as is the case with this issue on FreeBSD with the daemon tool when not passing -f: nodejs/help#2411).

Instead, this patch uses the dup2(2) syscall to properly remap the missing stdio file descriptor to what /dev/null's file descriptor actually is.

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[addaleax]

by hoping the next file descriptor to be given will be the one which doesn't exist

“hoping” is doing a lot of work here 🙂 This was actually spec’d behavior in previous POSIX versions, and I was quite surprised to learn (through this PR) that the spec actually changed in this regard.

I’d leave a comment in the source code about this, since others might be surprised by this as well.

[obiwac]

I’d leave a comment in the source code about this, since others might be surprised by this as well.

Got you. I'll do that in a little bit ✌️

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46354/

[bnoordhuis]

TBH, this change adds more failure paths and I don't think it's actually necessary.

The "open() returns the lowest available file descriptor" convention is so deeply codified in the UNIX ecosystem that it's never going away, no matter what POSIX says. There's simply too much software built around that assumption.

[obiwac]

The "open() returns the lowest available file descriptor" convention is so deeply codified in the UNIX ecosystem that it's never going away, no matter what POSIX says.

While I do agree with this, there still are specific cases where "available" means something different to what's expected. Case in point:

#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

int main(void) {
	system("echo && fstat | grep a.out");
	int fd = open("/dev/null", O_RDWR);
	printf("\n/dev/null: %d\n", fd);

	return 0;
}

Running this on FreeBSD in a separate session with daemon -o log.log -r /tmp/a.out and then closing said session (thus making stdin invalid):

root     a.out      89214 text [restricted]    519 -rwxr-xr-x   15424  r
root     a.out      89214   wd /        107129 drwxr-xr-x      16  r
root     a.out      89214 root /        107106 drwxr-xr-x      24  r
root     a.out      89214 jail /        107106 drwxr-xr-x      24  r
root     a.out      89214    0 -         -         bad    -
root     a.out      89214    1* pipe fffff8002e0e7d00 <-> fffff8002e0e7ba0      0 rw
root     a.out      89214    2* pipe fffff8002e0e7d00 <-> fffff8002e0e7ba0      0 rw
root     a.out      89214    3 [restricted]     24 crw-rw-rw-    null rw

/dev/null: 3

(This is actually the issue nodejs/help#2411 had.)

Here, 0 should be available, yet /dev/null is still opened as 3. Something else that works is closing 0 before opening /dev/null, but IMHO at that point it's kinda relying on flaky behaviour.

[bnoordhuis]

Ah okay, fair enough. Fd 0 points to a kind of zombie file description in your example, i.e., it's technically still open but not actually usable?

[obiwac]

Fd 0 points to a kind of zombie file description in your example, i.e., it's technically still open but not actually usable?

AFAIU the file descriptor is never actually closed, so it isn't ever marked as unused (fdunused()), which is why it's never removed from the file descriptor table.

[bnoordhuis]

Wait, but fstat() still fails with EBADF? How does that work?

[obiwac]

I don't know, there are plenty of places along the line where fstat(2) can fail with EBADF, not just because a file descriptor is not found in the file descriptor table (you can check out kern_fstat(), my quick guess is that fp->f_ops is set to &badfileops at some point before being unused).

I could investigate further if you'd like an exact answer but I don't see how it's too relevant outside of the FreeBSD kernel :P

[bnoordhuis]

"Zombie file descriptor" is good enough for me. :-)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46479/

[obiwac]

Is there anything left for me to do here?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47466/

[nodejs-github-bot]

Landed in 0c8b141