bug#7612: cp -pu NTFS problem

[Paul Eggert]

On 12/10/2010 09:41 PM, Garry Trethewey wrote:

> 1) I can see the difference
> -for (a /= res; a % 10 != 0; a /= 10)
> +for (a /= res; a % 10 == 0; a /= 10)
> but I'm just not that clever to apply a patch.

If you've built the software, then
find the line that looks like the "-" line, and change it
so that it looks like a "+" line.  Then type "make".
(If all this is beyond you, then don't worry too much; the bug
will be fixed in the next version of coreutils.)

> 2) I touch'ed a dest file so it's got tomorrow's date,
> and cp -p still copies to it, although not updating  (downdating?) it's 
> timestamp.

That sounds very much like a bug, perhaps a different bug, though I'm
afraid I can't reproduce it since I don't have your system.

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

[Paul Eggert]

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.

>From 780831a8602d9cdc22e7dcf44804e9c7183dd944 Mon Sep 17 00:00:00 2001
From: Chen Guo 
Date: Mon, 6 Dec 2010 00:15:42 -0800
Subject: [PATCH 1/4] sort: use mutexes, not spinlocks (avoid busy loop on 
blocked output)

Running a command like this on a multi-core system
  sort < big-file | less
would peg all processors at near 100% utilization.
* src/sort.c: (struct merge_node) Change member lock to mutex.
All uses changed.
* tests/Makefile.am (XFAIL_TESTS): Remove definition, now that
this test passes once again.  I.e., the sort-spinlock-abuse test
no longer fails.
* NEWS (Bug reports): Mention this.
Reported by DJ Lucas in http://debbugs.gnu.org/7489.
---
 NEWS  |5 +
 src/sort.c|   14 +++---
 tests/Makefile.am |3 ---
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/NEWS b/NEWS
index c3110a3..9f55cbb 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,11 @@ GNU coreutils NEWS-*- 
outline -*-
   sort -u with at least two threads could attempt to read through a
   corrupted pointer. [bug introduced in coreutils-8.6]
 
+  sort with at least two threads and with blocked output would busy-loop
+  (spinlock) all threads, often using 100% of available CPU cycles to
+  do no work.  I.e., "sort < big-file | less" could waste a lot of power.
+  [bug introduced in coreutils-8.6]
+
 ** New features
 
   split accepts the --number option to generate a specific number of files.
diff --git a/src/sort.c b/src/sort.c
index a4c2cbf..9cfc814 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -241,7 +241,7 @@ struct merge_node
   struct merge_node *parent;/* Parent node. */
   unsigned int level;   /* Level in merge tree. */
   bool queued;  /* Node is already in heap. */
-  pthread_spinlock_t lock;  /* Lock for node operations. */
+  pthread_mutex_t lock; /* Lock for node operations. */
 };
 
 /* Priority queue of merge nodes. */
@@ -3157,7 +3157,7 @@ compare_nodes (void const *a, void const *b)
 static inline void
 lock_node (struct merge_node *node)
 {
-  pthread_spin_lock (&node->lock);
+  pthread_mutex_lock (&node->lock);
 }
 
 /* Unlock a merge tree NODE. */
@@ -3165,7 +3165,7 @@ lock_node (struct merge_node *node)
 static inline void
 unlock_node (struct merge_node *node)
 {
-  pthread_spin_unlock (&node->lock);
+  pthread_mutex_unlock (&node->lock);
 }
 
 /* Destroy merge QUEUE. */
@@ -3479,7 +3479,7 @@ sortlines (struct line *restrict lines, struct line 
*restrict dest,
   node.parent = parent;
   node.level = parent->level + 1;
   node.queued = false;
-  pthread_spin_init (&node.lock, PTHREAD_PROCESS_PRIVATE);
+  pthread_mutex_init (&node.lock, NULL);
 
   /* Calculate thread arguments. */
   unsigned long int lo_threads = nthreads / 2;
@@ -3515,7 +3515,7 @@ sortlines (struct line *restrict lines, struct line 
*restrict dest,
   merge_loop (queue, total_lines, tfp, temp_output);
 }
 
-  pthread_spin_destroy (&node.lock);
+  pthread_mutex_destroy (&node.lock);
 }
 
 /* Scan through FILES[NTEMPS .. NFILES-1] looking for a file that is
@@ -3799,12 +3799,12 @@ sort (char * const *files, size_t nfiles, char const 
*output_file,
   node.parent = NULL;
   node.level = MERGE_END;
   node.queued = false;
-  pthread_spin_init (&node.lock, PTHREAD_PROCESS_PRIVATE);
+  pthread_mutex_init (&node.lock, NULL);
 
   sortlines (line, line, nthreads, buf.nlines, &node, true,
  &queue, tfp, temp_output);
   queue_destroy (&queue);
-  pthread_spin_destroy (&node.lock);
+  pthread_mutex_destroy (&node.lock);
 }
   else
 write_unique (line - 1, tfp, temp_output);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index d52f677..b573061 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -656,7 +656,4 @@ pr_data =   \
   pr/ttb3-FF   \
   pr/w72l24f-ll
 
-XFAIL_TESTS =  \
-  misc/sort-spinlock-abuse
-
 include $(srcdir)/check.mk
-- 
1.7.2


>From bcf9043b1f3983d099672047b36ad0a371af169d Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Fri, 10 Dec 2010 20:52:04 -0800
Subject: [PATCH 2/4] sort: comment fix

* src/sort.c: Comment fix re spin locks.
---
 src/sort.c |7 +--
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index 9cfc814..36e3b19 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -3149,10 +3149,7 @@ compare_nodes (void const *a, void const *b)
   return nodea->level < nodeb->level;
 }
 
-/* Lock a merge tree NODE.
-   Spin locks were seen to perform better than mutexes when the number
-  

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 
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 
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

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

[Paul Eggert]

Sorry for botching the NEWS and the change log.  To help
make amends, how about if I add a test case for that?
I'm thinking of the 2nd test case in
,
namely this one:

gensort -a 1 > gensort-10k
for i in $(seq 2000); do printf '% 4d\n' $i; src/sort -S 100K \
  --parallel=2 gensort-10k > j; test $(wc -c < j) = 100 || break; done

Or if you have a better one in mind, please let me know.

There are also some test cases I need to add for the
(unrelated) sort-compression bug, which is next on my
list of coreutils bugs to look at.

bug#7618: man mktemp/[deprecated] clarification

[jidanni]

man mktemp
   -p DIR use DIR as a prefix; implies -t [deprecated]

   -t interpret TEMPLATE as a single file name component, relative to a 
directory: $TMPDIR, if  set;
  else the directory specified via -p; else /tmp [deprecated]

It is not clear what is deprecated, -p, or both. Move [deprecated]
closer to the front of the sentence.

Or add a DEPRECATED OPTIONS section.

Also I know the info page says why, but at

   -u, --dry-run
  do not create anything; merely print a name (unsafe)

the reader thinks 'why, will it grind up my filesystem?'
If --dry-run is unsafe, then I'd hate to try --wet-run.

Say (unsafe to use for ).