bug#10243: 8.14: ls --color is uninterruptible with ctrl+c (and no network fs in use)

[Jim Meyering]

Arkadiusz Miśkiewicz wrote:
> On Wednesday 07 of December 2011, Pádraig Brady wrote:
>> On 12/07/2011 08:16 PM, Arkadiusz Miśkiewicz wrote:
>> > On Wednesday 07 of December 2011, Pádraig Brady wrote:
>> >> On 12/07/2011 05:56 PM, Jim Meyering wrote:
>> >>> Arkadiusz Miśkiewicz wrote:
>> >>>> When doing "ls --color=tty" or "ls --color=auto" on directory then ls
>> >>>> ignores (?) ctrl+c or ctrl+z signals. Basically I'm unable to
>> >>>> interrupt ls in such case. Easily reproducible with bigger
>> >>>> directories.
>> >>>
>> >>> Thanks for the report.
>> >>>
>> >>> I reproduced it starting in an empty directory like this:
>> >>>     seq 100000|xargs touch
>> >>>     env ls --color -1
>> >>>
>> >>> and tried to interrupt that.
>> >>> Failed to interrupt every time.
>> >>>
>> >>> Here's one way to fix it:
>> >>>
>> >>> diff --git a/src/ls.c b/src/ls.c
>> >>> index 8be9b6a..58bb196 100644
>> >>> --- a/src/ls.c
>> >>> +++ b/src/ls.c
>> >>> @@ -4060,9 +4060,9 @@ print_name_with_quoting (const struct fileinfo
>> >>> *f,
>> >>>
>> >>>    if (stack)
>> >>>
>> >>>      PUSH_CURRENT_DIRED_POS (stack);
>> >>>
>> >>> +  process_signals ();
>> >>>
>> >>>    if (used_color_this_time)
>> >>>
>> >>>      {
>> >>>
>> >>> -      process_signals ();
>> >>>
>> >>>        prep_non_filename_text ();
>> >>>        if (start_col / line_length != (start_col + width - 1) /
>> >>>        line_length)
>> >>>
>> >>>          put_indicator (&color_indicator[C_CLR_TO_EOL]);
>> >>
>> >> Looks like a good fix.
>> >> It works here and had negligible impact on performance.
>> >
>> > That part works for me too. Unfortunately more changes is needed since
>> > before printing happens it it still not possible to interrupt ls (and
>> > for huge dirs it can take a while).
>> >
>> > Moving code that enables special signal handling just before actuall
>> > printing starts?
>>
>> Probably, as long as there are no long blocking calls when
>> processing large dirs, after we've starting printing.
> Don't know why that should matter.
>
> IMO before printing happens there should be no difference in signal handling
> behaviour between ls --color and ls (since the whole signal handling is just
> to "protect" printing from what I understand).
>
> I was thinking about something like:
> - do ususal things
> - setup special signal handling
> - print + process_signals () at print_name_with_quoting
> - back to original signal handling
> - do the rest of things
>
> so most of the time there won't be any special signal handling (== will be the
> same as ls without --color).
>
>> Do you get the delays with -U too?
>
> Yes, too.
>
> ls doesn't call process_signals() at all before printing starts in --color
> mode thus making ls uninterruptible in that period.

Thanks for the feedback.
Here's a more complete patch, but I haven't re-reviewed it yet,
so caveat emptor.

It may write a test, too... but it will have to be racy,
so maybe not.

Oh, and I've just realized this needs a NEWS entry, too.


>From 54d7dafb0b10daa54d9beb8e1020c2e1bfbe0370 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 7 Dec 2011 23:00:42 +0100
Subject: [PATCH] ls: do not inhibit interrupts when listing large directories
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Starting with commit adc30a83, ls inhibited interrupts to avoid
corrupting the state of the terminal, but for very large directories,
that inhibition renders ls uninterruptible for too long, including
a potentially long period even before any output is generated.
Act on interrupts as much as possible.
* src/ls.c (Sig, nsigs): New globals, moved here from "main".
Global Sig renamed from sig, to avoid shadowing "int sig".
Now used in install_signal_handlers and in main.
(install_signal_handlers): New function, extracted from...
(main): ...here.
The top-level function for printing file names is print_current_files.
There are two execution paths by which to actually print a possibly-
colorized name from this function: via print_file_name_and_frills
or via the "case long_format".  We cannot simply install signal
handlers at the top, because both print_many_per_line and
print_horizontal end up calling calculate_columns, which would
cause a time-consuming and uninterruptible prelude to printing (when
there are many file names and many columns).  Thus, we opt to install
the signal handlers in two places: that "case long_format", and upon
the first call to print_file_name_and_frills.
Reported by Arkadiusz Miśkiewicz in http://bugs.gnu.org/10243
---
 THANKS.in |    1 +
 src/ls.c  |  165 +++++++++++++++++++++++++++++++++---------------------------
 2 files changed, 92 insertions(+), 74 deletions(-)

diff --git a/THANKS.in b/THANKS.in
index 5ecc29e..afed5d4 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -59,6 +59,7 @@ Anthony Thyssen                     address@hidden
 Antonio Rendas                      address@hidden
 Ariel Faigon                        address@hidden
 Arjan Opmeer                        address@hidden
+Arkadiusz Miśkiewicz                address@hidden
 Arne Henrik Juul                    address@hidden
 Arnold Robbins                      address@hidden
 Arthur Pool                         address@hidden
diff --git a/src/ls.c b/src/ls.c
index 8be9b6a..69e40a9 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -155,6 +155,36 @@
 # define st_author st_uid
 #endif

+/* 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
+static bool caught_sig[nsigs];
+#endif
+
 enum filetype
   {
     unknown,
@@ -1229,6 +1259,53 @@ process_signals (void)
     }
 }

+static void
+install_signal_handlers (void)
+{
+  if (print_with_color)
+    {
+      /* 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
+        }
+    }
+}
+
 int
 main (int argc, char **argv)
 {
@@ -1236,36 +1313,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, "");
@@ -1299,46 +1346,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)
@@ -1468,12 +1475,12 @@ main (int argc, char **argv)
       /* 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, Sig[j]))
+          signal (Sig[j], SIG_DFL);
 #else
       for (j = 0; j < nsigs; j++)
         if (caught_sig[j])
-          signal (sig[j], SIG_DFL);
+          signal (Sig[j], SIG_DFL);
 #endif

       /* Act on any signals that arrived before the default was restored.
@@ -3483,6 +3490,7 @@ print_current_files (void)
       break;

     case long_format:
+      install_signal_handlers ();
       for (i = 0; i < cwd_n_used; i++)
         {
           set_normal_color ();
@@ -4060,9 +4068,9 @@ print_name_with_quoting (const struct fileinfo *f,
   if (stack)
     PUSH_CURRENT_DIRED_POS (stack);

+  process_signals ();
   if (used_color_this_time)
     {
-      process_signals ();
       prep_non_filename_text ();
       if (start_col / line_length != (start_col + width - 1) / line_length)
         put_indicator (&color_indicator[C_CLR_TO_EOL]);
@@ -4092,6 +4100,15 @@ static size_t
 print_file_name_and_frills (const struct fileinfo *f, size_t start_col)
 {
   char buf[MAX (LONGEST_HUMAN_READABLE + 1, INT_BUFSIZE_BOUND (uintmax_t))];
+  static bool first = true;
+  if (first)
+    {
+      /* handle interrupts so that ls cannot be made to output an
+         incomplete multi-byte sequence or a color-change escape
+         sequence that could leave the terminal messed up.  */
+      install_signal_handlers ();
+      first = false;
+    }

   set_normal_color ();

--
1.7.8