Re: [PATCH v3] tee: handle EAGAIN returned from fwrite/fclose()

[Kamil Dudka]

On Tuesday, September 11, 2018 3:05:20 PM CET Kamil Dudka wrote:
> 'tee' expected the output file descriptors to operate in blocking mode
> but this assumption might be invalidated by other programs connected to
> the same terminal, as in the following example:
> 
> $ telnet ... | tee log_file
> 
> telnet calls ioctl(stdin, FIONBIO, [1]), which causes the O_NONBLOCK
> flag to be set on tee's output, which is connected to the same terminal.
> 
> This patch has zero impact unless EAGAIN returns from fwrite/fclose().
> In that case 'tee' waits for the underlying file descriptor to become
> writable again and then it writes the remaining data.

Are there still any concerns about the last version of this patch?

Is there anything I can do to move this forward?

Thanks in advance!

Kamil

> ---
>  src/tee.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/src/tee.c b/src/tee.c
> index dae0f1e50..264560e81 100644
> --- a/src/tee.c
> +++ b/src/tee.c
> @@ -17,6 +17,8 @@
>  /* Mike Parker, Richard M. Stallman, and David MacKenzie */
> 
>  #include <config.h>
> +#include <assert.h>
> +#include <poll.h>
>  #include <sys/types.h>
>  #include <signal.h>
>  #include <getopt.h>
> @@ -176,6 +178,75 @@ main (int argc, char **argv)
>    return ok ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
> 
> +/* On EAGAIN, wait for the underlying file descriptor to become writable.
> +   Return true, if EAGAIN has been successfully handled. */
> +static bool
> +handle_eagain_on_write (FILE *f)
> +{
> +  struct pollfd pfd;
> +  if (errno != EAGAIN)
> +    /* non-recoverable write error */
> +    return false;
> +
> +  /* obtain the file descriptor we are writing to */
> +  pfd.fd = fileno (f);
> +  if (pfd.fd == -1)
> +    goto fail;
> +
> +  /* wait for the file descriptor to become writable */
> +  pfd.events = POLLOUT;
> +  pfd.revents = 0;
> +  if (poll (&pfd, 1, -1) != 1)
> +    goto fail;
> +  if (!(pfd.revents & POLLOUT))
> +    goto fail;
> +
> +  /* successfully waited for the descriptor to become writable */
> +  clearerr(f);
> +  return true;
> +
> +fail:
> +  errno = EAGAIN;
> +  return false;
> +}
> +
> +/* wrapper of fwrite() that handles EAGAIN properly in case the O_NONBLOCK
> +   flag was set on the output file descriptor */
> +static bool
> +write_buf (const char *buf, ssize_t size, FILE *f)
> +{
> +  for (;;)
> +    {
> +      const size_t written = fwrite (buf, 1, size, f);
> +      size -= written;
> +      assert (size >= 0);
> +      if (size <= 0)
> +        /* everything written */
> +        return true;
> +
> +      if (!handle_eagain_on_write (f))
> +        return false;
> +
> +      /* update position in the write buffer */
> +      buf += written;
> +    }
> +}
> +
> +/* wrapper of fclose() that handles EAGAIN properly in case the O_NONBLOCK
> +   flag was set on the output file descriptor */
> +static bool
> +close_file (FILE *f)
> +{
> +  for (;;)
> +    {
> +      if (fclose (f) == 0)
> +        return true;
> +
> +      if (!handle_eagain_on_write (f))
> +        return false;
> +    }
> +}
> +
>  /* Copy the standard input into each of the NFILES files in FILES
>     and into the standard output.  As a side effect, modify FILES[-1].
>     Return true if successful.  */
> @@ -238,7 +309,7 @@ tee_files (int nfiles, char **files)
>           Standard output is the first one.  */
>        for (i = 0; i <= nfiles; i++)
>          if (descriptors[i]
> -            && fwrite (buffer, bytes_read, 1, descriptors[i]) != 1)
> +            && !write_buf (buffer, bytes_read, descriptors[i]))
>            {
>              int w_errno = errno;
>              bool fail = errno != EPIPE || (output_error ==
> output_error_exit @@ -266,7 +337,7 @@ tee_files (int nfiles, char **files)
> 
>    /* Close the files, but not standard output.  */
>    for (i = 1; i <= nfiles; i++)
> -    if (descriptors[i] && fclose (descriptors[i]) != 0)
> +    if (descriptors[i] && !close_file (descriptors[i]))
>        {
>          error (0, errno, "%s", quotef (files[i]));
>          ok = false;