Subject: bug#25402: closed (Re: [bug-diffutils] bug#36488: diffutils 3.7 make check failure ppc64le opensuse on colors test)

From: [email-masked] (GNU bug Tracking System)
Date: Sun, 29 Aug 2021 07:23:02 +0000

Your bug report

#36488: Race condition in "colors" test

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

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

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

Subject: Re: [bug-diffutils] bug#36488: diffutils 3.7 make check failure ppc64le opensuse on colors test
From: Paul Eggert <[email-masked]>
To: Thiago Jung Bauermann <[email-masked]>
Cc: Frédéric Bonnard <[email-masked]>, [email-masked], Santiago Vila <[email-masked]>, [email-masked], Normand <[email-masked]>
Date: Sun, 29 Aug 2021 00:22:27 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 8/28/21 8:40 AM, Thiago Jung Bauermann via bug-diffutils via All 
diffutils discussion. wrote:

> I believe this is the same problem reported in bug 34519.
> The Debian build also fails with "diff: standard output: Broken pipe".

Thanks for tracking that down. Frédéric's analysis in 
<https://bugs.debian.org/922552#19> was helpful.

I found some time to look into this bug, and installed into 
Savannah.gnu.org diffutils the attached patch, which I hope fixes the 
bug although I don't have the relevant platform to test it. Please give 
it a try.

Once this patch is part of a release, Debian shouldn't need any patches 
for diffutils.

For now I am closing the diffutils bug report 
<https://bugs.gnu.org/36488>; if I was too optimistic and the patch 
doesn't fix things we can always reopen it.
From 9b20182d48481c7ca647ff8926feeb8e1da4f7b0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 28 Aug 2021 23:49:32 -0700
Subject: [PATCH] diff: cleanup signal handling just before exit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This should fix an unlikely signal handling bug with colored
output, and should also fix a Debian FTBFS (Fails To Build From
Source) on powerpc64le-linux.  See Bug#34519 and Frédéric
Bonnard’s report in:
https://bugs.debian.org/922552#19
* bootstrap.conf (gnulib_modules): Add raise, sigprocmask.
* src/diff.c (main): Call cleanup_signal_handlers before exiting.
Don’t bother calling ‘exit’; no longer needed nowadays.
* src/util.c (sigprocmask, siginterrupt) [!SA_NOCLDSTOP]:
Define to 0 instead of empty, since the results are now used.
(sigset_t) [!SA_NOCLDSTOP]: Remove; we now rely on Gnulib.
(xsigaction) [SA_NOCLDSTOP]: New function.
(xsigaddset, xsigismember, xsignal, xsigprocmask): New functions.
(some_signals_caught): New static var.
(process_signals): Omit a conditional branch.
Don’t bother loading interrupt_signal if stop_signal_count is nonzero.
(process_signals, install_signal_handlers):
Check for failures from sigprocmask etc.
(sig, nsig): Now at top level, since multiple functions need them.
(install_signal_handlers): No need for caught_sig array;
just use caught_signals.  However, set some_signals_caught.
(cleanup_signal_handlers): New function.
---
 bootstrap.conf |   2 +
 src/diff.c     |   2 +-
 src/diff.h     |   1 +
 src/util.c     | 187 ++++++++++++++++++++++++++++++++-----------------
 4 files changed, 127 insertions(+), 65 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index 6560e9a..e51b2d8 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -65,11 +65,13 @@ mktime
 nstrftime
 progname
 propername
+raise
 rawmemchr
 readme-release
 regex
 sh-quote
 signal
+sigprocmask
 stat
 stat-macros
 stat-time
diff --git a/src/diff.c b/src/diff.c
index a4e5538..3b901aa 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -853,7 +853,7 @@ main (int argc, char **argv)
   print_message_queue ();
 
   check_stdout ();
-  exit (exit_status);
+  cleanup_signal_handlers ();
   return exit_status;
 }
 
diff --git a/src/diff.h b/src/diff.h
index 03f00a6..f346b43 100644
--- a/src/diff.h
+++ b/src/diff.h
@@ -388,6 +388,7 @@ extern struct change *find_change (struct change *);
 extern struct change *find_reverse_change (struct change *);
 extern enum changes analyze_hunk (struct change *, lin *, lin *, lin *, lin *);
 extern void begin_output (void);
+extern void cleanup_signal_handlers (void);
 extern void debug_script (struct change *);
 extern void fatal (char const *) __attribute__((noreturn));
 extern void finish_output (void);
diff --git a/src/util.c b/src/util.c
index dd6d3bf..8e676c8 100644
--- a/src/util.c
+++ b/src/util.c
@@ -31,10 +31,9 @@
    present.  */
 #ifndef SA_NOCLDSTOP
 # define SA_NOCLDSTOP 0
-# define sigprocmask(How, Set, Oset) /* empty */
-# define sigset_t int
+# define sigprocmask(How, Set, Oset) 0
 # if ! HAVE_SIGINTERRUPT
-#  define siginterrupt(sig, flag) /* empty */
+#  define siginterrupt(sig, flag) 0
 # endif
 #endif
 
@@ -160,16 +159,63 @@ print_message_queue (void)
     }
 }
 
-/* The set of signals that are caught.  */
 
+#if SA_NOCLDSTOP
+static void
+xsigaction (int sig, struct sigaction const *restrict act,
+	    struct sigaction *restrict oact)
+{
+  if (sigaction (sig, act, oact) != 0)
+    pfatal_with_name ("sigaction");
+}
+#endif
+
+static void
+xsigaddset (sigset_t *set, int sig)
+{
+  if (sigaddset (set, sig) != 0)
+    pfatal_with_name ("sigaddset");
+}
+
+static bool
+xsigismember (sigset_t const *set, int sig)
+{
+  int mem = sigismember (set, sig);
+  if (mem < 0)
+    pfatal_with_name ("sigismember");
+  assume (mem == 1);
+  return mem;
+}
+
+typedef void (*signal_handler) (int);
+static signal_handler
+xsignal (int sig, signal_handler func)
+{
+  signal_handler h = signal (sig, func);
+  if (h == SIG_ERR)
+    pfatal_with_name ("signal");
+  return h;
+}
+
+static void
+xsigprocmask (int how, sigset_t const *restrict set, sigset_t *restrict oset)
+{
+  if (sigprocmask (how, set, oset) != 0)
+    pfatal_with_name ("sigprocmask");
+}
+
+/* If true, some signals are caught.  This is separate from
+   'caught_signals' because POSIX doesn't require an all-zero sigset_t
+   to be valid.  */
+static bool some_signals_caught;
+
+/* The set of signals that are caught.  */
 static sigset_t caught_signals;
 
 /* If nonzero, the value of the pending fatal signal.  */
-
 static sig_atomic_t volatile interrupt_signal;
 
 /* A count of the number of pending stop signals that have been received.  */
-
 static sig_atomic_t volatile stop_signal_count;
 
 /* An ordinary signal was received; arrange for the program to exit.  */
@@ -202,21 +248,17 @@ stophandler (int sig)
 static void
 process_signals (void)
 {
-  while (interrupt_signal || stop_signal_count)
+  while (interrupt_signal | stop_signal_count)
     {
-      int sig;
-      int stops;
-      sigset_t oldset;
-
       set_color_context (RESET_CONTEXT);
       fflush (stdout);
 
-      sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
+      sigset_t oldset;
+      xsigprocmask (SIG_BLOCK, &caught_signals, &oldset);
 
-      /* Reload interrupt_signal and stop_signal_count, in case a new
-         signal was handled before sigprocmask took effect.  */
-      sig = interrupt_signal;
-      stops = stop_signal_count;
+      /* Reload stop_signal_count and (if needed) interrupt_signal, in
+	 case a new signal was handled before sigprocmask took effect.  */
+      int stops = stop_signal_count, sig;
 
       /* SIGTSTP is special, since the application can receive that signal
          more than once.  In this case, don't set the signal handler to the
@@ -227,82 +269,99 @@ process_signals (void)
           sig = SIGSTOP;
         }
       else
-        signal (sig, SIG_DFL);
+	{
+	  sig = interrupt_signal;
+	  xsignal (sig, SIG_DFL);
+	}
 
       /* Exit or suspend the program.  */
-      raise (sig);
-      sigprocmask (SIG_SETMASK, &oldset, NULL);
+      if (raise (sig) != 0)
+	pfatal_with_name ("raise");
+      xsigprocmask (SIG_SETMASK, &oldset, NULL);
 
       /* If execution reaches here, then the program has been
          continued (after being suspended).  */
     }
 }
 
-static void
-install_signal_handlers (void)
-{
-  /* The signals that are trapped, and the number of such signals.  */
-  static int const sig[] =
-    {
-      /* This one is handled specially.  */
-      SIGTSTP,
+/* The signals that can be caught, the number of such signals,
+   and which of them are actually caught.  */
+static int const sig[] =
+  {
+    /* This one is handled specially.  */
+    SIGTSTP,
 
-      /* The usual suspects.  */
-      SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
+    /* The usual suspects.  */
+    SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
 #ifdef SIGPOLL
-      SIGPOLL,
+    SIGPOLL,
 #endif
 #ifdef SIGPROF
-      SIGPROF,
+    SIGPROF,
 #endif
 #ifdef SIGVTALRM
-      SIGVTALRM,
+    SIGVTALRM,
 #endif
 #ifdef SIGXCPU
-      SIGXCPU,
+    SIGXCPU,
 #endif
 #ifdef SIGXFSZ
-      SIGXFSZ,
+    SIGXFSZ,
 #endif
-    };
-  enum { nsigs = sizeof (sig) / sizeof *(sig) };
+  };
+enum { nsigs = sizeof (sig) / sizeof *(sig) };
+
+static void
+install_signal_handlers (void)
+{
+  if (sigemptyset (&caught_signals) != 0)
+    pfatal_with_name ("sigemptyset");
 
-#if ! SA_NOCLDSTOP
-  bool caught_sig[nsigs];
-#endif
-  {
-    int j;
 #if SA_NOCLDSTOP
-    struct sigaction act;
+  for (int j = 0; j < nsigs; j++)
+    {
+      struct sigaction actj;
+      xsigaction (sig[j], NULL, &actj);
+      if (actj.sa_handler != SIG_IGN)
+	xsigaddset (&caught_signals, sig[j]);
+    }
 
-    sigemptyset (&caught_signals);
-    for (j = 0; j < nsigs; j++)
+  struct sigaction act;
+  act.sa_mask = caught_signals;
+  act.sa_flags = SA_RESTART;
+
+  for (int j = 0; j < nsigs; j++)
+    if (xsigismember (&caught_signals, sig[j]))
       {
-        sigaction (sig[j], NULL, &act);
-        if (act.sa_handler != SIG_IGN)
-          sigaddset (&caught_signals, sig[j]);
+	act.sa_handler = sig[j] == SIGTSTP ? stophandler : sighandler;
+	xsigaction (sig[j], &act, NULL);
+	some_signals_caught = true;
       }
-
-    act.sa_mask = caught_signals;
-    act.sa_flags = SA_RESTART;
-
-    for (j = 0; j < nsigs; j++)
-      if (sigismember (&caught_signals, sig[j]))
-        {
-          act.sa_handler = sig[j] == SIGTSTP ? stophandler : sighandler;
-          sigaction (sig[j], &act, NULL);
-        }
 #else
-    for (j = 0; j < nsigs; j++)
+  for (int j = 0; j < nsigs; j++)
+    if (xsignal (sig[j], SIG_IGN) != SIG_IGN)
       {
-        caught_sig[j] = (signal (sig[j], SIG_IGN) != SIG_IGN);
-        if (caught_sig[j])
-          {
-            signal (sig[j], sig[j] == SIGTSTP ? stophandler : sighandler);
-            siginterrupt (sig[j], 0);
-          }
+	xsigaddset (&caught_signals, sig[j]);
+	xsignal (sig[j], sig[j] == SIGTSTP ? stophandler : sighandler);
+	some_signals_caught = true;
+	if (siginterrupt (sig[j], 0) != 0)
+	  pfatal_with_name ("siginterrupt");
       }
 #endif
+}
+
+/* Clean up signal handlers just before exiting the program.  Do this
+   by resetting signal actions back to default, and then processing
+   any signals that arrived before resetting.  */
+void
+cleanup_signal_handlers (void)
+{
+  if (some_signals_caught)
+    {
+      for (int j = 0; j < nsigs; j++)
+	if (xsigismember (&caught_signals, sig[j]))
+	  xsignal (sig[j], SIG_DFL);
+      process_signals ();
     }
 }
 
-- 
2.30.2

Subject: Race condition in "colors" test
From: Santiago Vila <[email-masked]>
To: [email-masked]
Cc: [email-masked]
Date: Sun, 8 Jan 2017 23:35:08 +0100 (CET)
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello.

The test suite for diffutils 3.5 has a tendency to fail in our ppc64el
architecture:

https://buildd.debian.org/status/logs.php?pkg=diffutils&ver=1%3A3.5-2&arch=ppc64el

According to Christoph Biedl (Cc:ed), there is a race condition here:

| mkfifo fifo
| printf '%*s-a' 1000000 > a
| head -c 10 < fifo > /dev/null &
+ diff --color=always ---presume-output-tty a b > fifo
| test $? = 141 || fail=1

Quoting Christoph:

> There is a race condition involved, it fails in about three of
> four attempts - but not at all if the test is run under strace.

> A "sleep 0.1" before the diff made the test pass in ten of ten
> attempts. So the reason might indeed be the head command hasn't opened
> fifo yet by the time diff tries to write to it.

For the Debian package, I am going to add a "sleep 1"
(slightly more portable), but Christoph points out that this
should not really be architecture specific.

Thanks.