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 <ler...@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 <rjo...@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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
