Subject: bug#64540: closed (Re: bug#64540: [PATCH] od: exit out on failure to write to stdout)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 17 Jul 2023 10:32:02 +0000

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 [email-masked].

-- 
64540: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=64540
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#64540: [PATCH] od: exit out on failure to write to stdout
From: Pádraig Brady <[email-masked]>
To: Bernhard Voelker <[email-masked]>, Paul Eggert <[email-masked]>, Josef Bacik <[email-masked]>, [email-masked]
Date: Mon, 17 Jul 2023 11:31:39 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

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

Subject: [PATCH] od: exit out on failure to write to stdout
From: Josef Bacik <[email-masked]>
To: [email-masked]
Date: Sat, 8 Jul 2023 18:43:54 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

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