Re: feature request: gzip/bzip support for sort

[Jim Meyering]

"James Youngman" <address@hidden> wrote:
> It might be worth ensuring that we don't pass an invalid signal mask
> to sigprocmask(SET_MASK,...) if the previous call to
> sigprocmask(SIG_BLOCK,...) had failed.  Offhand I can't think of a way
> for sigprocmask() to fail unless the first argument is invalid, but
> looking that the unchecked return code makes me mildly nervous.  How
> about something resembling this (untested since my version of Automake
> is still only 1.9.6)?
>
> 2007-01-21  James Youngman  <address@hidden>
>
>       Centralize all the uses of sigprocmask().  Don't restore an invalid
>       saved mask.
>       * src/sort.c (enter_cs, leave_cs): New functions for protecting
>        code sequences against signal delivery.
>       * (exit_cleanup): Use enter_cs and leave_cs instead of
>        calling sigprocmask directly.
>       (create_temp_file, pipe_fork, zaptemp): Likewise

Hi James,

Good idea.
I've applied it on a branch with some minor changes:
- leave_cs should be void
- remove now-unused declarations of oldset
- rename new struct and functions to start with cs_

BTW, your patch was mangled: as if something filtered it through sed 's/^ //'.

I've applied this on top of a modified version of Dan's patch.
I'm fiddling with git now, with an eye to publishing this working branch.
In the mean time, I've included my working version of sort.c below.

Here's the adjusted patch:

diff --git a/ChangeLog b/ChangeLog
index fe8cd7a..764aef8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2007-01-21  James Youngman  <address@hidden>
+
+       Centralize all the uses of sigprocmask.  Don't restore an invalid
+       saved mask.
+       * src/sort.c (cs_enter, cs_leave): New functions for protecting
+       code sequences against signal delivery.
+       (exit_cleanup): Use cs_enter and cs_leave instead of calling
+       sigprocmask directly.
+       (create_temp_file, pipe_fork, zaptemp): Likewise
+
 2007-01-21  Jim Meyering  <address@hidden>

        * src/sort.c (MAX_FORK_RETRIES_COMPRESS, MAX_FORK_RETRIES_DECOMPRESS):
diff --git a/src/sort.c b/src/sort.c
index da10759..57003ea 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -64,7 +64,8 @@ struct rlimit { size_t rlim_cur; };
    present.  */
 #ifndef SA_NOCLDSTOP
 # define SA_NOCLDSTOP 0
-# define sigprocmask(How, Set, Oset) /* empty */
+/* No sigprocmask.  Always 'return' zero. */
+# define sigprocmask(How, Set, Oset) (0)
 # define sigset_t int
 # if ! HAVE_SIGINTERRUPT
 #  define siginterrupt(sig, flag) /* empty */
@@ -412,6 +413,33 @@ static struct option const long_options[] =
 /* The set of signals that are caught.  */
 static sigset_t caught_signals;

+/* Critical section status.  */
+struct cs_status
+{
+  bool valid;
+  sigset_t sigs;
+};
+
+/* Enter a critical section.  */
+static struct cs_status
+cs_enter (void)
+{
+  struct cs_status status;
+  status.valid = (sigprocmask (SIG_BLOCK, &caught_signals, &status.sigs) == 0);
+  return status;
+}
+
+/* Leave a critical section.  */
+static void
+cs_leave (struct cs_status status)
+{
+  if (status.valid)
+    {
+      /* Ignore failure when restoring the signal mask. */
+      sigprocmask (SIG_SETMASK, &status.sigs, NULL);
+    }
+}
+
 /* The list of temporary files. */
 struct tempnode
 {
@@ -577,10 +605,9 @@ exit_cleanup (void)
     {
       /* Clean up any remaining temporary files in a critical section so
         that a signal handler does not try to clean them too.  */
-      sigset_t oldset;
-      sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
+      struct cs_status cs = cs_enter ();
       cleanup ();
-      sigprocmask (SIG_SETMASK, &oldset, NULL);
+      cs_leave (cs);
     }

   close_stdout ();
@@ -594,7 +621,6 @@ create_temp_file (int *pfd)
 {
   static char const slashbase[] = "/sortXXXXXX";
   static size_t temp_dir_index;
-  sigset_t oldset;
   int fd;
   int saved_errno;
   char const *temp_dir = temp_dirs[temp_dir_index];
@@ -602,6 +628,7 @@ create_temp_file (int *pfd)
   struct tempnode *node =
     xmalloc (offsetof (struct tempnode, name) + len + sizeof slashbase);
   char *file = node->name;
+  struct cs_status cs;

   memcpy (file, temp_dir, len);
   memcpy (file + len, slashbase, sizeof slashbase);
@@ -611,7 +638,7 @@ create_temp_file (int *pfd)
     temp_dir_index = 0;

   /* Create the temporary file in a critical section, to avoid races.  */
-  sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
+  cs = cs_enter ();
   fd = mkstemp (file);
   if (0 <= fd)
     {
@@ -619,7 +646,7 @@ create_temp_file (int *pfd)
       temptail = &node->next;
     }
   saved_errno = errno;
-  sigprocmask (SIG_SETMASK, &oldset, NULL);
+  cs_leave (cs);
   errno = saved_errno;

   if (fd < 0)
@@ -699,10 +726,10 @@ pipe_fork (int pipefds[2], size_t tries)
 {
 #if HAVE_WORKING_FORK
   struct tempnode *saved_temphead;
-  sigset_t oldset;
   int saved_errno;
   unsigned int wait_retry = 1;
   pid_t pid IF_LINT (= 0);
+  struct cs_status cs;

   if (pipe (pipefds) < 0)
     return -1;
@@ -711,7 +738,7 @@ pipe_fork (int pipefds[2], size_t tries)
     {
       /* This is so the child process won't delete our temp files
         if it receives a signal before exec-ing.  */
-      sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
+      cs = cs_enter ();
       saved_temphead = temphead;
       temphead = NULL;

@@ -720,7 +747,7 @@ pipe_fork (int pipefds[2], size_t tries)
       if (pid)
        temphead = saved_temphead;

-      sigprocmask (SIG_SETMASK, &oldset, NULL);
+      cs_leave (cs);
       errno = saved_errno;

       if (0 <= pid || errno != EAGAIN)
@@ -878,20 +905,20 @@ zaptemp (const char *name)
   struct tempnode *volatile *pnode;
   struct tempnode *node;
   struct tempnode *next;
-  sigset_t oldset;
   int unlink_status;
   int unlink_errno = 0;
+  struct cs_status cs;

   for (pnode = &temphead; (node = *pnode)->name != name; pnode = &node->next)
     continue;

   /* Unlink the temporary file in a critical section to avoid races.  */
   next = node->next;
-  sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
+  cs = cs_enter ();
   unlink_status = unlink (name);
   unlink_errno = errno;
   *pnode = next;
-  sigprocmask (SIG_SETMASK, &oldset, NULL);
+  cs_leave (cs);

   if (unlink_status != 0)
     error (0, unlink_errno, _("warning: cannot remove: %s"), name);

sort.c.gz
Description: Binary data