GNU bug report logs - #24232
ls --color cannot be interrupted by a signal

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24232 in the body.
You can then email your comments to 24232 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: bug-coreutils <at> gnu.org
Subject: ls --color cannot be interrupted by a signal
Date: Mon, 15 Aug 2016 15:55:04 +0200

If 'ls --color' outputs to a terminal and a syscall blocks (e.g. while reading 
a directory from unresponsive network file system), it cannot be interrupted 
by a signal.

This seems to be caused by the code of ls, which sets the SA_RESTART flag on 
terminating signals.  A possible solution would be to reset the flag prior to 
calling certain blocking syscalls and process the signals synchronously in an 
EINTR loop.

Alternatively, we can document this behavior as intended (or broken by design) 
and suggest users to disable color output or redirect the output off terminal 
to work around the issue.

Any other idea how to solve it?

Originally reported at: https://bugzilla.redhat.com/1365933

Kamil

Message #8 received at 24232 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Kamil Dudka <kdudka <at> redhat.com>, 24232 <at> debbugs.gnu.org
Subject: Re: bug#24232: ls --color cannot be interrupted by a signal
Date: Mon, 15 Aug 2016 15:55:13 +0100

On 15/08/16 14:55, Kamil Dudka wrote:
> If 'ls --color' outputs to a terminal and a syscall blocks (e.g. while reading 
> a directory from unresponsive network file system), it cannot be interrupted 
> by a signal.
> 
> This seems to be caused by the code of ls, which sets the SA_RESTART flag on 
> terminating signals.  A possible solution would be to reset the flag prior to 
> calling certain blocking syscalls and process the signals synchronously in an 
> EINTR loop.

Sounds expensive

> Alternatively, we can document this behavior as intended (or broken by design) 
> and suggest users to disable color output or redirect the output off terminal 
> to work around the issue.
> 
> Any other idea how to solve it?
> 
> Originally reported at: https://bugzilla.redhat.com/1365933

The signal catching functionality originated trying to restore terminal color:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v4.5.3-89-g8549068

That was adjusted to only outputting reset chars once for multiple signals,
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-357-geae1b7f

and not outputting reset chars at arbitrary places as that messes up multi-byte chars:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-368-gadc30a8

Maybe we should just buffer internally at put_indicator (&color_indicator[C_LEFT]);
and output only at put_indicator (&color_indicator[C_RIGHT]) or equivalent.
That wouldn't introduce significant overhead I think.

Pádraig

Message #11 received at 24232 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 24232 <at> debbugs.gnu.org
Subject: Re: bug#24232: ls --color cannot be interrupted by a signal
Date: Mon, 15 Aug 2016 17:52:38 +0200

On Monday, August 15, 2016 15:55:13 Pádraig Brady wrote:
> The signal catching functionality originated trying to restore terminal
> color:
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v4.5.3-89-g854
> 9068
> 
> That was adjusted to only outputting reset chars once for multiple signals,
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-357-geae
> 1b7f
> 
> and not outputting reset chars at arbitrary places as that messes up
> multi-byte chars:
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-368-gad
> c30a8
> 
> Maybe we should just buffer internally at put_indicator
> (&color_indicator[C_LEFT]); and output only at put_indicator
> (&color_indicator[C_RIGHT]) or equivalent. That wouldn't introduce
> significant overhead I think.

Internal buffering is certainly doable.  I guess there is some gnulib module 
that already implements such a buffer?

However, what to do with signal handling then?  Drop the SA_RESTART flag and 
implement EINTR loop only for the fwrite() call that writes data with escape 
sequences to the terminal?

Kamil

> Pádraig

Message #14 received at 24232 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Kamil Dudka <kdudka <at> redhat.com>
Cc: 24232 <at> debbugs.gnu.org
Subject: Re: bug#24232: ls --color cannot be interrupted by a signal
Date: Mon, 15 Aug 2016 18:40:00 +0100

On 15/08/16 16:52, Kamil Dudka wrote:
> On Monday, August 15, 2016 15:55:13 Pádraig Brady wrote:
>> The signal catching functionality originated trying to restore terminal
>> color:
>> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v4.5.3-89-g854
>> 9068
>>
>> That was adjusted to only outputting reset chars once for multiple signals,
>> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-357-geae
>> 1b7f
>>
>> and not outputting reset chars at arbitrary places as that messes up
>> multi-byte chars:
>> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v5.2.1-368-gad
>> c30a8
>>
>> Maybe we should just buffer internally at put_indicator
>> (&color_indicator[C_LEFT]); and output only at put_indicator
>> (&color_indicator[C_RIGHT]) or equivalent. That wouldn't introduce
>> significant overhead I think.
> 
> Internal buffering is certainly doable.  I guess there is some gnulib module 
> that already implements such a buffer?

Not that I noticed. The conditions for flushing tend to vary per app,
so we've slightly different variations in various utils, like lbuf_flush()
in factor(1), and buffer_or_output() in relpath(1).
We could take advantage of a PATH_MAX+A_BIT sized buffer to avoid realloc
complexities.

Actually we already buffer in quote_name(), so rather than outputting
the color sequences separately in print_name_with_quoting(), we could
pass those into quote_name() and it could write to its buffer,
and output the START+name+END atomically.

> However, what to do with signal handling then?  Drop the SA_RESTART flag and 
> implement EINTR loop only for the fwrite() call that writes data with escape 
> sequences to the terminal?

Hrm, the buffering above would avoid needing to explicitly output the color reset
sequence upon exit, but it would not avoid the need to fflush(), since stdio
might actually output the start and end color sequences in separate writes.
Drats, so if we had to manage signals anyway to do the fflush() the buffering
probably isn't worth it.

I'm not sure how to improve this.
Perhaps we should look at only enabling the signal handling before printing,
in the common case of outputting the sorted entries of a single directory.
In that way the open(), stat() etc. would be done with standard signal handling.

Message #17 received at 24232 <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 24232 <at> debbugs.gnu.org
Subject: [PATCH] ls: postpone installation of signal handlers
Date: Tue,  6 Sep 2016 17:38:26 +0200

... until they are actually needed.  That is right before the first
escape sequence is printed to a terminal.

* src/ls.c (sigs): Moved from main() to global scope to make it
available in put_indicator(), too.  Renamed to prevent identifier
clashes with parameters of subsequent function definitions.
(nsigs): Likewise.
(caught_sig): Likewise.
(main): Code installing signal handlers moved to put_indicator() in
order not to keep default signal handling untouched as long as possible.
Adjusted condition for restoring signal handlers to reflect the change.
(put_indicator): Install signal handlers if called for the very first
time.  It uses the same code that was in main() prior to this commit.
* NEWS: Mention the change.

See https://bugzilla.redhat.com/1365933
---
 NEWS     |   3 ++
 src/ls.c | 167 +++++++++++++++++++++++++++++++--------------------------------
 2 files changed, 86 insertions(+), 84 deletions(-)

diff --git a/NEWS b/NEWS
index 736b95e..d4af96f 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   System V style platforms where this information is available only
   in the global variable 'tzname'. [bug introduced in coreutils-8.24]
 
+  ls is now fully responsive to signals until the first escape sequence is
+  written to a terminal.
+
   nl now resets numbering for each page section rather than just for each page.
   [This bug was present in "the beginning".]
 
diff --git a/src/ls.c b/src/ls.c
index 3572060..c438f61 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -753,6 +753,36 @@ static char const *long_time_format[2] =
     N_("%b %e %H:%M")
   };
 
+/* The signals that are trapped, and the number of such signals.  */
+static int const sigs[] =
+{
+  /* This one is handled specially.  */
+  SIGTSTP,
+
+  /* The usual suspects.  */
+  SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
+#ifdef SIGPOLL
+  SIGPOLL,
+#endif
+#ifdef SIGPROF
+  SIGPROF,
+#endif
+#ifdef SIGVTALRM
+  SIGVTALRM,
+#endif
+#ifdef SIGXCPU
+  SIGXCPU,
+#endif
+#ifdef SIGXFSZ
+  SIGXFSZ,
+#endif
+};
+enum { nsigs = ARRAY_CARDINALITY (sigs) };
+
+#if ! SA_NOCLDSTOP
+static bool caught_sig[nsigs];
+#endif
+
 /* The set of signals that are caught.  */
 
 static sigset_t caught_signals;
@@ -1251,36 +1281,6 @@ main (int argc, char **argv)
   struct pending *thispend;
   int n_files;
 
-  /* The signals that are trapped, and the number of such signals.  */
-  static int const sig[] =
-    {
-      /* This one is handled specially.  */
-      SIGTSTP,
-
-      /* The usual suspects.  */
-      SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
-#ifdef SIGPOLL
-      SIGPOLL,
-#endif
-#ifdef SIGPROF
-      SIGPROF,
-#endif
-#ifdef SIGVTALRM
-      SIGVTALRM,
-#endif
-#ifdef SIGXCPU
-      SIGXCPU,
-#endif
-#ifdef SIGXFSZ
-      SIGXFSZ,
-#endif
-    };
-  enum { nsigs = ARRAY_CARDINALITY (sig) };
-
-#if ! SA_NOCLDSTOP
-  bool caught_sig[nsigs];
-#endif
-
   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
   setlocale (LC_ALL, "");
@@ -1314,46 +1314,6 @@ main (int argc, char **argv)
           || (is_colored (C_EXEC) && color_symlink_as_referent)
           || (is_colored (C_MISSING) && format == long_format))
         check_symlink_color = true;
-
-      /* If the standard output is a controlling terminal, watch out
-         for signals, so that the colors can be restored to the
-         default state if "ls" is suspended or interrupted.  */
-
-      if (0 <= tcgetpgrp (STDOUT_FILENO))
-        {
-          int j;
-#if SA_NOCLDSTOP
-          struct sigaction act;
-
-          sigemptyset (&caught_signals);
-          for (j = 0; j < nsigs; j++)
-            {
-              sigaction (sig[j], NULL, &act);
-              if (act.sa_handler != SIG_IGN)
-                sigaddset (&caught_signals, sig[j]);
-            }
-
-          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++)
-            {
-              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);
-                }
-            }
-#endif
-        }
     }
 
   if (dereference == DEREF_UNDEFINED)
@@ -1466,31 +1426,29 @@ main (int argc, char **argv)
       print_dir_name = true;
     }
 
-  if (print_with_color)
+  if (print_with_color && used_color)
     {
       int j;
 
-      if (used_color)
-        {
-          /* Skip the restore when it would be a no-op, i.e.,
-             when left is "\033[" and right is "m".  */
-          if (!(color_indicator[C_LEFT].len == 2
-                && memcmp (color_indicator[C_LEFT].string, "\033[", 2) == 0
-                && color_indicator[C_RIGHT].len == 1
-                && color_indicator[C_RIGHT].string[0] == 'm'))
-            restore_default_color ();
-        }
+      /* Skip the restore when it would be a no-op, i.e.,
+         when left is "\033[" and right is "m".  */
+      if (!(color_indicator[C_LEFT].len == 2
+            && memcmp (color_indicator[C_LEFT].string, "\033[", 2) == 0
+            && color_indicator[C_RIGHT].len == 1
+            && color_indicator[C_RIGHT].string[0] == 'm'))
+        restore_default_color ();
+
       fflush (stdout);
 
       /* Restore the default signal handling.  */
 #if SA_NOCLDSTOP
       for (j = 0; j < nsigs; j++)
-        if (sigismember (&caught_signals, sig[j]))
-          signal (sig[j], SIG_DFL);
+        if (sigismember (&caught_signals, sigs[j]))
+          signal (sigs[j], SIG_DFL);
 #else
       for (j = 0; j < nsigs; j++)
         if (caught_sig[j])
-          signal (sig[j], SIG_DFL);
+          signal (sigs[j], SIG_DFL);
 #endif
 
       /* Act on any signals that arrived before the default was restored.
@@ -4475,6 +4433,47 @@ put_indicator (const struct bin_str *ind)
   if (! used_color)
     {
       used_color = true;
+
+      /* If the standard output is a controlling terminal, watch out
+         for signals, so that the colors can be restored to the
+         default state if "ls" is suspended or interrupted.  */
+
+      if (0 <= tcgetpgrp (STDOUT_FILENO))
+        {
+          int j;
+#if SA_NOCLDSTOP
+          struct sigaction act;
+
+          sigemptyset (&caught_signals);
+          for (j = 0; j < nsigs; j++)
+            {
+              sigaction (sigs[j], NULL, &act);
+              if (act.sa_handler != SIG_IGN)
+                sigaddset (&caught_signals, sigs[j]);
+            }
+
+          act.sa_mask = caught_signals;
+          act.sa_flags = SA_RESTART;
+
+          for (j = 0; j < nsigs; j++)
+            if (sigismember (&caught_signals, sigs[j]))
+              {
+                act.sa_handler = sigs[j] == SIGTSTP ? stophandler : sighandler;
+                sigaction (sigs[j], &act, NULL);
+              }
+#else
+          for (j = 0; j < nsigs; j++)
+            {
+              caught_sig[j] = (signal (sigs[j], SIG_IGN) != SIG_IGN);
+              if (caught_sig[j])
+                {
+                  signal (sigs[j], sigs[j] == SIGTSTP ? stophandler : sighandler);
+                  siginterrupt (sigs[j], 0);
+                }
+            }
+#endif
+        }
+
       prep_non_filename_text ();
     }
 
-- 
2.7.4

Message #22 received at 24232-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Kamil Dudka <kdudka <at> redhat.com>
Cc: 24232-done <at> debbugs.gnu.org
Subject: Re: bug#24232: [PATCH] ls: postpone installation of signal handlers
Date: Tue, 6 Sep 2016 16:59:08 +0100

On 06/09/16 16:38, Kamil Dudka wrote:
> ... until they are actually needed.  That is right before the first
> escape sequence is printed to a terminal.

Cool, that's a good improvement.
Note I moved the NEWS from "bugs" to "improvements".
Also I tweaked a long line to avoid `make syntax-check` failure.

Will push later.

thanks!
Pádraig

Message #25 received at 24232 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 24232 <at> debbugs.gnu.org, P <at> draigBrady.com, kdudka <at> redhat.com
Subject: Re: bug#24232: [PATCH] ls: postpone installation of signal handlers
Date: Tue, 6 Sep 2016 10:05:35 -0700

On 09/06/2016 08:59 AM, Pádraig Brady wrote:
> Will push later.

Before pushing, can you please change the name of "sigs" back to "sig"? 
I prefer the old name, as "sig[i]" clearly means "signal i", whereas 
"sigs[i]" is more confusing. (This is one of my pet peeves about array 
names.) Thanks.

Message #28 received at 24232 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 24232 <at> debbugs.gnu.org, Kamil Dudka <kdudka <at> redhat.com>,
 Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#24232: [PATCH] ls: postpone installation of signal handlers
Date: Tue, 6 Sep 2016 10:18:45 -0700

On Tue, Sep 6, 2016 at 10:05 AM, Paul Eggert <eggert <at> cs.ucla.edu> wrote:
> On 09/06/2016 08:59 AM, Pádraig Brady wrote:
>>
>> Will push later.
>
>
> Before pushing, can you please change the name of "sigs" back to "sig"? I
> prefer the old name, as "sig[i]" clearly means "signal i", whereas "sigs[i]"
> is more confusing. (This is one of my pet peeves about array names.) Thanks.

I agree.
A good argument for "why?" is that we optimize for readability of the
more frequent use with an index, sig[i], rather than for the sole
declaration, or the less frequent uses of the array name without an
index.

Message #31 received at 24232 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>, Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 24232 <at> debbugs.gnu.org, Kamil Dudka <kdudka <at> redhat.com>
Subject: Re: bug#24232: [PATCH] ls: postpone installation of signal handlers
Date: Tue, 6 Sep 2016 19:07:13 +0100

On 06/09/16 18:18, Jim Meyering wrote:
> On Tue, Sep 6, 2016 at 10:05 AM, Paul Eggert <eggert <at> cs.ucla.edu> wrote:
>> On 09/06/2016 08:59 AM, Pádraig Brady wrote:
>>>
>>> Will push later.
>>
>>
>> Before pushing, can you please change the name of "sigs" back to "sig"? I
>> prefer the old name, as "sig[i]" clearly means "signal i", whereas "sigs[i]"
>> is more confusing. (This is one of my pet peeves about array names.) Thanks.
> 
> I agree.
> A good argument for "why?" is that we optimize for readability of the
> more frequent use with an index, sig[i], rather than for the sole
> declaration, or the less frequent uses of the array name without an
> index.

Ok done.

I consolidated signal handler setup and restoration to a single function
to allow that.

Message #34 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kamil Dudka <kdudka <at> redhat.com>
To: bug-coreutils <at> gnu.org
Cc: 24232 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>,
 Pádraig Brady <P <at> draigbrady.com>,
 Jim Meyering <jim <at> meyering.net>
Subject: Re: bug#24232: [PATCH] ls: postpone installation of signal handlers
Date: Wed, 07 Sep 2016 10:15:37 +0200

On Tuesday, September 06, 2016 19:07:13 Pádraig Brady wrote:
> On 06/09/16 18:18, Jim Meyering wrote:
> > On Tue, Sep 6, 2016 at 10:05 AM, Paul Eggert <eggert <at> cs.ucla.edu> wrote:
> >> On 09/06/2016 08:59 AM, Pádraig Brady wrote:
> >>> Will push later.
> >> 
> >> Before pushing, can you please change the name of "sigs" back to "sig"? I
> >> prefer the old name, as "sig[i]" clearly means "signal i", whereas
> >> "sigs[i]" is more confusing. (This is one of my pet peeves about array
> >> names.) Thanks.> 
> > I agree.
> > A good argument for "why?" is that we optimize for readability of the
> > more frequent use with an index, sig[i], rather than for the sole
> > declaration, or the less frequent uses of the array name without an
> > index.
> 
> Ok done.
> 
> I consolidated signal handler setup and restoration to a single function
> to allow that.

Thanks for review!  I am fine with all the proposals.  Please let me know
in case any resubmission is needed.

Kamil

Previous Next

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