Ensure standard file descriptors are open on start
Commit Message
ISO C requires that standard input, output and error are always open on
program startup.
---
Prior to these changes, a program could be launched that has no standard input,
output or error stream. This is in conflict with the C and POSIX standards, and
causes some programs (such as autoconf configure scripts) to fail. This behavior
can be easily replicated using a "wrapper" program such as:
#include <unistd.h>
int main(int argc, char **argv) {
close(0);
execvp(argv[1], argv + 1);
}
Launching cat via ./close_stdin cat will, without these changes, result in an
EBADF, but will silently exit (as it is reading /dev/null) with these changes.
The latter behavior imitates what the C standard requires.
csu/check_fds.c | 10 +++++-----
csu/libc-start.c | 9 +++------
elf/dl-sysdep.c | 7 ++-----
3 files changed, 10 insertions(+), 16 deletions(-)
Comments
On 8/18/20 10:50 AM, Arsen Arsenović via Libc-alpha wrote:
> - check_one_fd (STDIN_FILENO, O_WRONLY | O_NOFOLLOW);
> - check_one_fd (STDOUT_FILENO, O_RDONLY | O_NOFOLLOW);
> - check_one_fd (STDERR_FILENO, O_RDONLY | O_NOFOLLOW);
> + check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
> + check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
> + check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
Why is this change needed? Even if ISO C requires that standard streams be open,
it doesn't require that one can do I/O successfully with the streams.
> > - check_one_fd (STDIN_FILENO, O_WRONLY | O_NOFOLLOW);
> > - check_one_fd (STDOUT_FILENO, O_RDONLY | O_NOFOLLOW);
> > - check_one_fd (STDERR_FILENO, O_RDONLY | O_NOFOLLOW);
> > + check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
> > + check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
> > + check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
>
> Why is this change needed? Even if ISO C requires that standard streams be
> open, it doesn't require that one can do I/O successfully with the streams.
It seems to me that the sanest option would be making all three file descriptors
read/wrote only (as appropriate), by opening /dev/null. This makes all reads
EOFs and all writes successful, which would be what, at least, I expect when
closing one of the standard streams.
Regardless, with and without this change, the configure script I mentioned still
runs fine (since the standard streams it rightfully assumes exist indeed do
exist), so I can remove it, if that's a better solution.
On 8/18/20 3:31 PM, Arsen Arsenović wrote:
> This makes all reads EOFs and all writes successful, which would be what,
> at least, I expect when closing one of the standard streams.
That's not what I expect. There is a benefit for a program like 'cat' to
report an error (instead of silently succeeding) if its invoker closed stdin
or stdout. That's why glibc behaves the way it does, and it seems a behavior
worth keeping.
> That's not what I expect. There is a benefit for a program like 'cat' to
> report an error (instead of silently succeeding) if its invoker closed stdin
> or stdout. That's why glibc behaves the way it does, and it seems a behavior
> worth keeping.
Fair enough. It is, however, still not correct to have any of those missing on
startup. I can resubmit the patch with this fixed.
@@ -58,8 +58,8 @@ check_one_fd (int fd, int mode)
}
/* Something is wrong with this descriptor, it's probably not
- opened. Open /dev/null so that the SUID program we are
- about to start does not accidentally use this descriptor. */
+ opened. Open /dev/null so that the program we are about to
+ start does not accidentally use this descriptor. */
int nullfd = __open_nocancel (name, mode, 0);
/* We are very paranoid here. With all means we try to ensure
@@ -90,7 +90,7 @@ __libc_check_standard_fds (void)
is really paranoid but some people actually are. If /dev/null
should happen to be a symlink to somewhere else and not the
device commonly known as "/dev/null" we bail out. */
- check_one_fd (STDIN_FILENO, O_WRONLY | O_NOFOLLOW);
- check_one_fd (STDOUT_FILENO, O_RDONLY | O_NOFOLLOW);
- check_one_fd (STDERR_FILENO, O_RDONLY | O_NOFOLLOW);
+ check_one_fd (STDIN_FILENO, O_RDONLY | O_NOFOLLOW);
+ check_one_fd (STDOUT_FILENO, O_WRONLY | O_NOFOLLOW);
+ check_one_fd (STDERR_FILENO, O_WRONLY | O_NOFOLLOW);
}
@@ -253,12 +253,9 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
if (fini)
__cxa_atexit ((void (*) (void *)) fini, NULL, NULL);
- /* Some security at this point. Prevent starting a SUID binary where
- the standard file descriptors are not opened. We have to do this
- only for statically linked applications since otherwise the dynamic
- loader did the work already. */
- if (__builtin_expect (__libc_enable_secure, 0))
- __libc_check_standard_fds ();
+ /* Ensure the standard streams are opened, as required by POSIX and C. For
+ dynamic programs this is already handled in the dynamic loader. */
+ __libc_check_standard_fds ();
#endif
/* Call the initializer of the program, if any. */
@@ -243,11 +243,8 @@ _dl_sysdep_start (void **start_argptr,
__sbrk (GLRO(dl_pagesize)
- ((_end - (char *) 0) & (GLRO(dl_pagesize) - 1)));
- /* If this is a SUID program we make sure that FDs 0, 1, and 2 are
- allocated. If necessary we are doing it ourself. If it is not
- possible we stop the program. */
- if (__builtin_expect (__libc_enable_secure, 0))
- __libc_check_standard_fds ();
+ /* Ensure all the standard streams are open (C and POSIX require this) */
+ __libc_check_standard_fds ();
(*dl_main) (phdr, phnum, &user_entry, GLRO(dl_auxv));
return user_entry;