unix: inconsistent handling of closed stdio

[DDoSolitary]

• Version: 1.38.1
• Platform: All *nix systems

Currently different components of libuv behaves inconsistently when stdio fds are closed manually by user. joyent/libuv#943 introduced the check that will cause crash the whole program when attempting to close a fd <= 2. It assumes that normally libuv shall not try to close such fds, but it is not the case when a user chooses to manually close stdio fds, after which system will reuse these fd numbers when creating new fds.
Then there are changes trying to address such problems. #796 introduced uv__close_nocheckstdio for process.c, which is later used in some other components. There's even a test case for closed stdio: fd77a5d (though it was added before the introduction of uv__close in process.c)

As a result, support for closed stdio is in a very inconsistent and confusing state. Most components of libuv will crash when stdio is closed manually, because users may pass it a low-numbered fd reused by system, or libuv itself may create low-numbered fds. In the meantime, there's a small number of functions that uses uv__close_nocheckstdio and will work in these edge cases.
To make things worth, even these function are inconsistent. For example uv_spawn only calls uv__close_nocheckstdio for signal_pipe but uses uv__close for other things. So it will not fail when there's nothing in options->stdio but will fail when both stdin and stdout is closed and user specifies UV_CREATE_PIPE in options->stdio (add close(1) to the spawn_closed_process_io test it will fail)

[bnoordhuis]

I appreciate that you wrote up all that but what exactly are you proposing?

For context: the philosophy that libuv adheres to is that closing file descriptors 0-2 is almost always a program bug, potentially with security implications: get the program to reopen them as sockets and the attacker can now read the program's logging.

There are a few uses of uv__close_nocheckstdio() that should probably be a checked uv__close() but that doesn't really detract from the main point.

[DDoSolitary]

I appreciate that you wrote up all that but what exactly are you proposing?

Sorry for being unclear on this. What I'm proposing is to make things consistent. That is:

• If closing stdio fds is considered a valid use case, then either uv__close should not check it at all, or all fds created by libuv should be checked and F_DUPFDed to a higher value when necessary.
• If closing stdio fds is not supported, then I would like to propose to change all usage of uv__close_nocheckstdio to uv__close.

[DDoSolitary]

The existing code, especially the spawn_closed_process_io test case and merging of #796 gives me the impression that it is a supported scenario, which is quite misleading IMHO.

[vtjnash]

I think this is a duplicate of #2062. My comment there is that our experience in JuliaLang found that embedding applications (including nodejs) should pass dup(1) instead of 1 to libuv for reliability (inside nodejs and libuv and other dynamic libraries, among others) in this and other edge cases, including allowing reverting the workaround in joyent/libuv#943.

[DDoSolitary]

@vtjnash It seems that #2062 is about libuv's behavior when it is given a stdio fd. However, I'm trying to address something different here: when stdio fds are closed outside of libuv 1) users may pass libuv fd 0-2 which is reused for other purposes 2) libuv itself may create fds with number 0-2 and crash due to assertion failure in uv__close (e.g. just add close(1) to the spawn_closed_process_io test in addition to close(0) and the test will fail)

[vtjnash]

Yep, that was a different example, but I'm suggesting it's the same underlying issue (that libuv shouldn't assume fd 0-2 are special numbers, and that users shouldn't pass stdio fd directly to libuv).

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[hmh]

You know, this issue is still there, it is still causing random pain at surprising places (libuv being a library, after all).

If libuv's take on FD0-2 being closed is that "it is broken", to the point of assert() crashing applications that get started through multiple layers of shell scripting that somehow ended up with, e.g., stdin closed... why doesn't it open a closed fd 0-2 to /dev/null?

Either that, or drop the self-inflicting DoS caused by these asserts.

For the record, here's what I end up adding to every application under my control, since libuv is not the only library out there that is broken re. FD 0-2 expectiations at program start (glibc FILE streams on non setuid/setgid applications being another high-profile example).

I didn't know at the time I'd be working around libuv bugs with this, but... there you have it... not portable as is, but it should be possible to port it to windows, etc.

static int is_valid_fd(const int fd)
{
return fcntl(fd, F_GETFD) != -1 || errno != EBADF;
}

static void fix_fds(const int fd, const int fl)
{
int nfd;

if (is_valid_fd(fd))
return;

nfd = open("/dev/null", fl);
if (nfd == -1 || dup2(nfd, fd) == -1) {
print_err("could not attach /dev/null to file descriptor %d: %s",
fd, strerror(errno));
/* if (nfd != -1) close(nfd); - disabled as we're going to exit() now */
exit(SEXIT_FAILURE);
}
if (nfd != fd)
close(nfd);
}

/*

• glibc does not ensure sanity of the standard streams at program start
• for non suid/sgid applications. The streams are initialized as open
• and not in an error state even when their underlying FDs are invalid
• (closed). These FDs will later become valid due to an unrelated
• open(), which will cause undesired behavior (such as data corruption)
• should the stream be used.
• freopen() cannot be used to fix this directly, due to a glibc 2.14+ bug
• when freopen() is called on an open stream that has an invalid FD which
• also happens to be the first available FD.
  /
  static void sanitize_std_fds(void)
  {
  / do it in file descriptor numerical order! */
  fix_fds(STDIN_FILENO, O_RDONLY);
  fix_fds(STDOUT_FILENO, O_WRONLY);
  fix_fds(STDERR_FILENO, O_RDWR);
  }

[vtjnash]

Yep, that is a good observation too. I let stale bot close this only because I consider it to be a duplicate of #2062, which I hope will be fixed here in v2. But even for v1, I think your code logic would be a useful addition to the libuv library as a new recommended initialization helper function if you would be interested in making a PR.

Note, for github markdown comments, you can use code fences to make your code more readable like so:

```c
code goes here
```

[hmh]

Well, I don't know libuv (nor I use it currently), so it is unlikely I'd get to that PR anytime soon. If anyone else is interested in doing so, please feel free to do it. It is certainly something every POSIX application should do at startup in the general case.