Re: [Libguestfs] [libnbd PATCH v3 21/29] CONNECT_COMMAND.START: sanitize close() calls in the child process

In the child process: - we have no need for the parent-side end of the socket pair (sv[0]), - we duplicate the child-side end of the socket pair (sv[1]) to both of the child's fd#0 and fd#1, and then close sv[1]. close (STDIN_FILENO); close (STDOUT_FILENO); close (sv[0]); dup2 (sv[1], STDIN_FILENO); dup2 (sv[1], STDOUT_FILENO); close (sv[1]); This code silently assumes that sv[1] falls outside of the the fd set {0,1} -- put differently, the code assumes that each dup2() call will duplicate sv[1] to a file descriptor that is *different* from sv[1]. Let's investigate (a) if the logic is correct under said assumption, and (b) what happened if the assumption were wrong, (c) whether the assumption is valid to make. (a) Under the assumption, the initial two close() calls turn out to be superfluous. That's because dup2 (oldfd, newfd) closes "newfd" first, if "newfd" is open and *differs* from "oldfd". (b) If the assumption were wrong -- that is, if sv[1] equaled 0 or 1 --, then the logic would misbehave, at several levels. First, one of the initial close() calls would cause sv[1] to be closed, thereby breaking both dup2() calls. Second, even if we removed the first two close() calls, the final close() would still be wrong. In this case, one of the dup2() calls would be a no-op (oldfd == newfd), and the other dup2() would work correctly. However, the final close would retroactively invalidate the one dup2() call that had behaved as a no-op. (c) Now let's see whether we need to fix issue (b); that is, whether the assumption that sv[1] differs from both 0 and 1 is valid to make. This assumption is actually valid. ISO C guarantees that the stdin, stdout and stderr I/O streams are open at program startup, and POSIX translates the same requirement to file descriptors. In particular, the *informative* rationale in POSIX tallies its *normative* requirements as follows: https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html#... | B.2.5 Standard I/O Streams | | Although the ISO C standard guarantees that, at program start-up, | /stdin/ is open for reading and /stdout/ and /stderr/ are open for | writing, this guarantee is contingent (as are all guarantees made by | the ISO C and POSIX standards) on the program being executed in a | conforming environment. Programs executed with file descriptor 0 not | open for reading or with file descriptor 1 or 2 not open for writing | are executed in a non-conforming environment. Application writers | are warned (in [exec], [posix_spawn], and [Redirection]) not to | execute a standard utility or a conforming application with file | descriptor 0 not open for reading or with file descriptor 1 or 2 not | open for writing. The proper way for "disabling" these file descriptors (as described by XRAT as well) is to redirect them from/to "/dev/null". Now, if the libnbd client application closed file descriptors 0 and/or 1 after its startup, it would effectively invalidate itself. Consider the (informative) APPLICATION USAGE section of the fclose() spec: https://pubs.opengroup.org/onlinepubs/9699919799/functions/fclose.html#ta... | Since after the call to /fclose()/ any use of stream results in | undefined behavior, /fclose()/ should not be used on /stdin/, | /stdout/, or /stderr/ except immediately before process termination | (see XBD [Process Termination]), so as to avoid triggering undefined | behavior in other standard interfaces that rely on these streams. If | there are any [atexit()] handlers registered by the application, | such a call to /fclose()/ should not occur until the last handler is | finishing. Once /fclose()/ has been used to close /stdin/, /stdout/, | or /stderr/, there is no standard way to reopen any of these | streams. | | Use of [freopen()] to change /stdin/, /stdout/, or /stderr/ instead | of closing them avoids the danger of a file unexpectedly being | opened as one of the special file descriptors STDIN_FILENO, | STDOUT_FILENO, or STDERR_FILENO at a later time in the application. Thus, the assumption is deemed valid. Therefore: - While valid, the assumption is not trivial. So, assert it in the child process. Furthermore, because regular assert()'s in the parent process may be easier to read for the user, assert a slightly more comprehensive predicate about socketpair()'s output there, too. - Remove the first two close() calls, which are superfluous. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-U6 generator/states-connect.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/generator/states-connect.c b/generator/states-connect.c index f0020d6f3b26..e08608336fd6 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -248,26 +248,35 @@ CONNECT_COMMAND.START: if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socketpair"); return 0; } + /* A process is effectively in an unusable state if any of STDIN_FILENO + * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If they + * exist however, then the socket pair created above will not intersect with + * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic + * below. + */ + assert (sv[0] > STDERR_FILENO); + assert (sv[1] > STDERR_FILENO); + pid = fork (); if (pid == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "fork"); close (sv[0]); close (sv[1]); return 0; } if (pid == 0) { /* child - run command */ - close (STDIN_FILENO); - close (STDOUT_FILENO); close (sv[0]); dup2 (sv[1], STDIN_FILENO); dup2 (sv[1], STDOUT_FILENO); + NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO); + NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO); close (sv[1]); /* Restore SIGPIPE back to SIG_DFL. */ signal (SIGPIPE, SIG_DFL); execvp (h->argv.ptr[0], h->argv.ptr);