Subject: bug#34519: 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:03 +0000

Your bug report

#36488: Test "colors" fails on Debian/ppc64el

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: Test "colors" fails on Debian/ppc64el
From: Santiago Vila <[email-masked]>
To: [email-masked]
Date: Mon, 18 Feb 2019 01:57:09 +0100 (CET)
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello.

I've just uploaded diffutils 3.7 for Debian unstable (hopefully to be
part of Debian 10, since it contains only small fixes).

But there is a problem: The build in the ppc64el autobuilder failed:

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

In tests/colors there was a race condition which I tried to fix
by adding a "sleep 1", like this:

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

It is possible that the "head" command takes (sometimes) more than one second to
complete on ppc64el?

Are there any other places in the test where a race condition could be
happening?

Thanks.