Re: prevent seq from inflow on I/O errors

[Bernhard Voelker]

Some findings below:

On 04/04/2016 01:57 PM, Pádraig Brady wrote:
> From 65741749a324ff640e15a9b2959c8d6ee85131f9 Mon Sep 17 00:00:00 2001
> From: Assaf Gordon <address@hidden>
> Date: Mon, 4 Apr 2016 12:31:43 +0100
> Subject: [PATCH] seq: detect and report I/O errors immediately
> 
> Ensure I/O errors are detected (and terminate seq), preventing seq
> from infloop (or running for long time with a large
> range) upon write errors or ignored SIGPIPE. Examples:
> 
>      seq 1 inf > /dev/full             (seq_fast)
>      seq 1.1 0.1 inf >/dev/full        (print_numbers)
> 
> * src/seq.c (io_error): A new function to diagnose appropriate
> stdio errors and exit the program with failure status.
> (seq_fast, print_numbers): Explicitly check for write errors
> and terminate the program with diagnostic.
> * tests/misc/seq-io-errors.sh: Eest new error detection.

s/Eest/Test/

> * tests/local.mk: Add new test.
> * NEWS: Mention the fix.
> ---
>  NEWS                        |  3 +++
>  src/seq.c                   | 27 ++++++++++++++++++++-----
>  tests/local.mk              |  1 +
>  tests/misc/seq-io-errors.sh | 48 
> +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 5 deletions(-)
>  create mode 100644 tests/misc/seq-io-errors.sh
> 
> diff --git a/NEWS b/NEWS
> index dd3ee9c..ffb8641 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -15,6 +15,9 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>     stty --help no longer outputs extraneous gettext header lines
>     for translated languages. [bug introduced in coreutils-8.24]
>  
> +   seq now immediately exits upon write errors.
> +   [This bug was present in "the beginning".]
> +
>  ** Changes in behavior
>  
>     stat now outputs nanosecond information for time stamps even if
> diff --git a/src/seq.c b/src/seq.c
> index fbb94a0..3095715 100644
> --- a/src/seq.c
> +++ b/src/seq.c
> @@ -267,6 +267,18 @@ long_double_format (char const *fmt, struct layout 
> *layout)
>        }
>  }
>  
> +static void ATTRIBUTE_NORETURN
> +io_error (void)
> +{
> +  int status = EXIT_FAILURE;
> +  if (errno != EPIPE)
> +    error (0, errno, _("standard output"));
> +  else
> +    status = EXIT_SUCCESS;
> +  clearerr (stdout);
> +  exit (status);
> +}
> +
>  /* Actually print the sequence of numbers in the specified range, with the
>     given or default stepping and format.  */
>  
> @@ -284,7 +296,8 @@ print_numbers (char const *fmt, struct layout layout,
>        for (i = 1; ; i++)
>          {
>            long double x0 = x;
> -          printf (fmt, x);
> +          if (printf (fmt, x) < 0)
> +            io_error ();
>            if (out_of_range)
>              break;
>            x = first + i * step;
> @@ -325,10 +338,12 @@ print_numbers (char const *fmt, struct layout layout,
>                  break;
>              }
>  
> -          fputs (separator, stdout);
> +          if (fputs (separator, stdout) == EOF)
> +            io_error ();
>          }
>  
> -      fputs (terminator, stdout);
> +      if (fputs (terminator, stdout) == EOF)
> +        io_error ();
>      }
>  }
>  
> @@ -495,14 +510,16 @@ seq_fast (char const *a, char const *b)
>               output buffer so far, and reset to start of buffer.  */
>            if (buf_end - (p_len + 1) < bufp)
>              {
> -              fwrite (buf, bufp - buf, 1, stdout);
> +              if (fwrite (buf, bufp - buf, 1, stdout) != 1)
> +                io_error ();
>                bufp = buf;
>              }
>          }
>  
>        /* Write any remaining buffered output, and the terminator.  */
>        *bufp++ = *terminator;
> -      fwrite (buf, bufp - buf, 1, stdout);
> +      if (fwrite (buf, bufp - buf, 1, stdout) != 1)
> +        io_error ();
>  
>        IF_LINT (free (buf));
>      }
> diff --git a/tests/local.mk b/tests/local.mk
> index a83c3d0..8b2a19a 100644
> --- a/tests/local.mk
> +++ b/tests/local.mk
> @@ -235,6 +235,7 @@ all_tests =                                       \
>    tests/misc/ptx.pl                          \
>    tests/misc/test.pl                         \
>    tests/misc/seq.pl                          \
> +  tests/misc/seq-io-errors.sh                        \
>    tests/misc/seq-long-double.sh                      \
>    tests/misc/seq-precision.sh                        \
>    tests/misc/head.pl                         \
> diff --git a/tests/misc/seq-io-errors.sh b/tests/misc/seq-io-errors.sh
> new file mode 100644
___________________^^^

maint.mk: Please make test executable: tests/misc/seq-io-errors.sh
cfg.mk:120: recipe for target 'sc_tests_executable' failed
make: *** [sc_tests_executable] Error 1


> index 0000000..ac44ca2
> --- /dev/null
> +++ b/tests/misc/seq-io-errors.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +# Test for proper detection of I/O errors in seq
> +
> +# Copyright (C) 2016 Free Software Foundation, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
> +print_ver_ seq
> +
> +if ! test -w /dev/full || ! test -c /dev/full; then
> +  skip_ '/dev/full is required'
> +fi
> +
> +# Run 'seq' with a timeout, preventing infinite-loop run.
> +# if seq fails (returns 1) - the test passed.
> +# if seq times-out (timeout forces exitcode of 124) - the test failed.
> +# any other code - framework failure.
> +timed_seq_fail() { returns_ 1 timeout 10 seq "$@" >/dev/full 2>/dev/null; }

The comment somehow doesn't seem to match anymore.

> +# Test infinite sequence, using fast-path method (seq_fast).
> +timed_seq_fail 1 inf

s/$/ || fail=1/

returns_ only ensures that $? = 1, but doesn't set fail=1
Therefore, this would pass with the previous seq.c!

Maybe we'd add a syntax-check for this one day, so I'd suggest

  timed_seq_fail() { timeout 10 seq "$@" >/dev/full 2>/dev/null; }

  returns_ 1 timed_seq_fail 1 inf || fail=1


> +# Test infinite sequence, using slow-path method (print_numbers).
> +timed_seq_fail 1.1 .1 inf

Likewise.

> +# Test non-infinite sequence, using slow-path method (print_numbers).
> +# (despite being non-infinite, the entire sequence should take long time to
> +#  print. Thus, either an I/O error is detected immedaitely, or seq will
> +#  timeout).
> +timed_seq_fail 1 0.0001 99999999

Likewise.

> +(trap '' PIPE; timeout 10 sh -c 'seq inf 2>err | head -n1') || fail=1
> +compare /dev/null err || fail=1
> +
> +Exit $fail

seq.c looks good to me.

Have a nice day,
Berny