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

[Richard W.M. Jones]

On Wed, Feb 15, 2023 at 03:11:50PM +0100, Laszlo Ersek wrote:
> 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#tag_22_02_05
> 
>     | 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#tag_16_121_07
> 
>     | 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 at redhat.com>
> ---
> 
> 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);

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top