bug#7597: multi-threaded sort can segfault (unrelated to the sort -u segfault)

[Jim Meyering]

Paul Eggert wrote:
> Thanks, Chen, that was much nicer than what I was writing.
> I did some minor cleanups, mostly to do with catching an
> unlikely integer overflow that would have made 'sort' crash badly,
> and pushed the following set of patches.

Thanks for helping, but...

Chen's log message would have been appropriate if that patch had been
rebased to apply after my stack->heap bug fix.  However, as a replacement
for my fix, the description is lacking, since it says nothing about fixing
the hard-to-reproduce (and harder-to-diagnose) segfault-inducing bug.
Plus, the change set includes no NEWS or test suite addition.

With each bug fix it is best to describe
or at least mention the bug in the commit log.
Also, there should be a NEWS entry and, preferably,
a test or two to exercise the bug.

Here at least is the NEWS addition and log from what I posted Thursday.
I've also fixed a few syntax nits separately:


>From 9a9d69e9e463ebfa67e90ecc061305532a91543e Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 11 Dec 2010 11:29:38 +0100
Subject: [PATCH 1/2] sort: syntax cleanup

* src/sort.c (xfopen, debug_key, sortlines, sort, main): Adjust
formatting: fix misplaced braces, use consistent spacing,
split a 2-stmt line.
---
 src/sort.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index 2c0f852..1de8115 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -939,7 +939,7 @@ stream_open (char const *file, char const *how)

 static FILE *
 xfopen (char const *file, char const *how)
- {
+{
   FILE *fp = stream_open (file, how);
   if (!fp)
     die (_("open failed"), file);
@@ -2207,7 +2207,8 @@ debug_key (struct line const *line, struct keyfield const 
*key)

       if (key->skipsblanks || key->month || key_numeric (key))
         {
-          char saved = *lim; *lim = '\0';
+          char saved = *lim;
+          *lim = '\0';

           while (blanks[to_uchar (*beg)])
             beg++;
@@ -3782,7 +3783,7 @@ merge (struct sortfile *files, size_t ntemps, size_t 
nfiles,
 /* Sort NFILES FILES onto OUTPUT_FILE.  Use at most NTHREADS threads.  */

 static void
-sort (char * const *files, size_t nfiles, char const *output_file,
+sort (char *const *files, size_t nfiles, char const *output_file,
       size_t nthreads)
 {
   struct buffer buf;
@@ -4498,7 +4499,7 @@ main (int argc, char **argv)
           files = tok.tok;
           nfiles = tok.n_tok;
           for (i = 0; i < nfiles; i++)
-          {
+            {
               if (STREQ (files[i], "-"))
                 error (SORT_FAILURE, 0, _("when reading file names from stdin, 
"
                                           "no file name of %s allowed"),
@@ -4513,7 +4514,7 @@ main (int argc, char **argv)
                          _("%s:%lu: invalid zero-length file name"),
                          quotearg_colon (files_from), file_number);
                 }
-          }
+            }
         }
       else
         error (SORT_FAILURE, 0, _("no input from %s"),
--
1.7.3.3


>From ad61335bf832937dd95739c70405db9b61ffda37 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 11 Dec 2010 11:38:21 +0100
Subject: [PATCH 2/2] sort: avoid segfault when using two or more threads

This change does not fix the actual bug.  That was done by commit
c9db0ac6, "sort: preallocate merge tree nodes to heap".  The fix
was to store each "node" structure on the heap, not on the stack.
Otherwise, a node from one thread's stack could be used in another
thread after the first thread had expired (via pthread_join).
This bug was very hard to trigger when using spinlocks, but
easier once we began using mutexes.
* NEWS (Bug fixes): Mention it.
For details, see http://debbugs.gnu.org/7597.
---
 NEWS |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/NEWS b/NEWS
index 9f55cbb..b0a11b1 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   do no work.  I.e., "sort < big-file | less" could waste a lot of power.
   [bug introduced in coreutils-8.6]

+  sort with at least two threads no longer segfaults due to use of pointers
+  into the stack of an expired thread. [bug introduced in coreutils-8.6]
+
 ** New features

   split accepts the --number option to generate a specific number of files.
--
1.7.3.3