GNU bug report logs - #64540
[PATCH] od: exit out on failure to write to stdout

Previous Next

Full log

View this message in rfc822 format

From: help-debbugs <at> gnu.org (GNU bug Tracking System)
To: Josef Bacik <josef <at> toxicpanda.com>
Subject: bug#64540: closed (Re: bug#64540: [PATCH] od: exit out on failure
 to write to stdout)
Date: Mon, 17 Jul 2023 10:32:02 +0000

[Message part 1 (text/plain, inline)]

Your bug report

#64540: [PATCH] od: exit out on failure to write to stdout

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to 64540 <at> debbugs.gnu.org.

-- 
64540: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=64540
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems

[Message part 2 (message/rfc822, inline)]

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Josef Bacik <josef <at> toxicpanda.com>,
 64540-done <at> debbugs.gnu.org
Subject: Re: bug#64540: [PATCH] od: exit out on failure to write to stdout
Date: Mon, 17 Jul 2023 11:31:39 +0100

On 17/07/2023 07:35, Bernhard Voelker wrote:
> On 7/15/23 23:08, Pádraig Brady wrote:
>> The attached patch-set addresses two classes of issue:
>> 1. Doubled error messages upon write errors
>> 2. Continued processing upon write errors  (the orig problem reported).
> 
> Nice work!
> 
> Patch 1:
> 
>   > --- /dev/null
>   > +++ b/tests/misc/write-errors.sh
>   > @@ -0,0 +1,70 @@
> 
>   > +if ! test -w /dev/full || ! test -c /dev/full; then
>   > +  skip_ 'Need /dev/full'
> 
> Other tests use a slightly different skip message:
>     skip_ '/dev/full is required'
> 
> I'd favor the latter, so that we don't miss it once we'd like
> to factor it out to a require_dev_full in init.cfg.
> 
> Re. patch 002:
> 
>   >     all: avoid repeated diagnostic upon write error
>   >
>   >     * cfg.mk (sc_some_programs_must_avoid_exit_failure): Adjust to
>   >     avoid false positive.
>   >     (sc_prohibit_exit_write_error): A new syntax check to prohibit
>   >     open coding error(..., "write error"); instead directiing to use...
> 
> s/ii/i/
> 
>   > --- a/src/system.h
>   > +++ b/src/system.h
>   > @@ -762,6 +762,14 @@ The following directory is part of the cycle:\n  %s\n"), \
>   >      }					\
>   >    while (0)
>   >
>   > +static inline void
>   > +write_error (void)
>   > +{
>   > +  fflush (stdout);    /* Ensure nothing buffered that might induce an error. */
>   > +  clearerr (stdout);  /* To avoid extraneous diagnostic from close_stdout.  */
>   > +  error (EXIT_FAILURE, errno, _("write error"));
>   > +}
> 
> Hmm, fflush could theoretically change errno, couldn't it?
> In that case, error() would give a wrong diagnostic.
> FWIW: `man clearerr` states that this function doesn't touch errno.

Yes good call.
Current version is:

/* exit with a _single_ "write error" diagnostic.  */
static inline void
write_error (void)
{
  int saved_errno = errno;
  fflush (stdout);    /* Ensure nothing buffered that might induce an error. */
  clearerr (stdout);  /* To avoid extraneous diagnostic from close_stdout.  */
  error (EXIT_FAILURE, saved_errno, _("write error"));
}

I'll push later.

Marking this as done.

Thanks for the review.

Pádraig

[Message part 3 (message/rfc822, inline)]

From: Josef Bacik <josef <at> toxicpanda.com>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] od: exit out on failure to write to stdout
Date: Sat,  8 Jul 2023 18:43:54 -0400

A very weird bug was uncovered when using fstests with github actions.
In fstests we are doing something like this

od /dev/urandom | dd of=somefile bs=1M count=10

The above works fine, except in the case of github actions which runs
this script remotely, capturing the output in a pipe to print on their
console.  Somehow od ends up writing forever into the stdout pipe
without exiting in this scenario, normally it doesn't do this.
strace'ing the process we can see it getting SIGPIPE and simply ignoring
the error.  This is because when we don't provide a limit we never
actually check if there was an error on the stdout fd.  Fix this by
adjusting the logic to check if there was an error on every loop, and
break out if appropriate.  With this patch applied this configuration no
longer results in a hang.
---
 src/od.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/od.c b/src/od.c
index 6b66ceb4f..a07b4de03 100644
--- a/src/od.c
+++ b/src/od.c
@@ -1380,7 +1380,7 @@ dump (void)
 
   if (limit_bytes_to_format)
     {
-      while (true)
+      while (ok)
         {
           size_t n_needed;
           if (current_offset >= end_offset)
@@ -1396,13 +1396,14 @@ dump (void)
           affirm (n_bytes_read == bytes_per_block);
           write_block (current_offset, n_bytes_read,
                        block[!idx], block[idx]);
+	  ok &= check_and_close(0);
           current_offset += n_bytes_read;
           idx = !idx;
         }
     }
   else
     {
-      while (true)
+      while (ok)
         {
           ok &= read_block (bytes_per_block, block[idx], &n_bytes_read);
           if (n_bytes_read < bytes_per_block)
@@ -1410,6 +1411,7 @@ dump (void)
           affirm (n_bytes_read == bytes_per_block);
           write_block (current_offset, n_bytes_read,
                        block[!idx], block[idx]);
+	  ok &= check_and_close(0);
           current_offset += n_bytes_read;
           idx = !idx;
         }
-- 
2.41.0

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.