bug#7489: [coreutils] over aggressive threads in sort

[Paul Eggert]

On 11/26/2010 06:52 PM, Pádraig Brady wrote:
> Hmm, seems like multiple threads are racing to update the
> static "saved" variable in write_unique() ?

I don't think it's as simple as that.  write_unique
is generating output, and when it is run it is supposed
to have exclusive access to the output file, which means
it also has exclusive access to the "saved" variable.

I suspect the problem is that the line structure is
messed up when you have multiple threads and sufficiently
large input, in that when one merge node is done and
the next one is run, the "saved" variable (which points
to storage managed by the earlier node) is pointing to
storage that is no longer valid.  I haven't had time to
look into the details, though.

Chen, do you have some insight into this?  Here's the thread:

http://lists.gnu.org/archive/html/bug-coreutils/2010-11/msg00209.html

I have now reproduced the bug on RHEL 5.5 x86-64, using
coreutils 8.7 built with "configure CC='cc -m32'".  The bug
is also present in the savannah git master.

bug#7489: [coreutils] over aggressive threads in sort

[Paul Eggert]

Following up on my previous email, it appears to me that
the following line in mergelines_node is weird:

node->dest -= lo_orig - node->lo + hi_orig - node->hi;

Surely there should be a "*" in front of that line?

(This does not fix the bug; perhaps it is a different bug?)

bug#7502: wc --verbose should say xx lines, yy words, zz characters

[jidanni]

Idea: new option: --verbose
$ wc --verbose should say
xx lines, yy words, zz characters
or
xx lines
yy words
zz characters
etc.
With TAB separator too.

bug#7489: [coreutils] over aggressive threads in sort

[Paul Eggert]

Could you please try this little patch?  It should fix your
problem.  I came up with this fix in my sleep (literally!
I woke up this morning and the patch was in my head), but
haven't had time to look at the code in this area to see
if it's the best fix.

Clearly there's at least one more bug as noted in my previous email

but I expect it's less likely to fire.

diff --git a/src/sort.c b/src/sort.c
index 7e25f6a..1aa1eb4 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -3226,13 +3226,13 @@ queue_pop (struct merge_node_queue *queue)
 static void
 write_unique (struct line const *line, FILE *tfp, char const *temp_output)
 {
-  static struct line const *saved = NULL;
+  static struct line saved;
 
   if (!unique)
 write_line (line, tfp, temp_output);
-  else if (!saved || compare (line, saved))
+  else if (!saved.text || compare (line, &saved))
 {
-  saved = line;
+  saved = *line;
   write_line (line, tfp, temp_output);
 }
 }

bug#7489: [coreutils] over aggressive threads in sort

[Pádraig Brady]

On 28/11/10 00:57, Paul Eggert wrote:
> Could you please try this little patch?  It should fix your
> problem.  I came up with this fix in my sleep (literally!
> I woke up this morning and the patch was in my head), but
> haven't had time to look at the code in this area to see
> if it's the best fix.
> 
> Clearly there's at least one more bug as noted in my previous email
> 
> but I expect it's less likely to fire.
> 
> diff --git a/src/sort.c b/src/sort.c
> index 7e25f6a..1aa1eb4 100644
> --- a/src/sort.c
> +++ b/src/sort.c
> @@ -3226,13 +3226,13 @@ queue_pop (struct merge_node_queue *queue)
>  static void
>  write_unique (struct line const *line, FILE *tfp, char const *temp_output)
>  {
> -  static struct line const *saved = NULL;
> +  static struct line saved;
>  
>if (!unique)
>  write_line (line, tfp, temp_output);
> -  else if (!saved || compare (line, saved))
> +  else if (!saved.text || compare (line, &saved))
>  {
> -  saved = line;
> +  saved = *line;
>write_line (line, tfp, temp_output);
>  }
>  }
> 
> 

It passes a fleeting test between kids and sleep...

$ zcat cracklib-words-20080507.gz | ./coreutils-8.7/src/sort -u > /dev/null
Segmentation fault (core dumped)
$ zcat cracklib-words-20080507.gz | ./coreutils-8.7/src/sort -u | wc -l
0
$ zcat cracklib-words-20080507.gz | ./coreutils-8.7/src/sort-paul -u | wc -l
1671704

cheers,
Pádraig.

bug#7489: [coreutils] over aggressive threads in sort

[DJ Lucas]

On 11/27/2010 06:57 PM, Paul Eggert wrote:
> Could you please try this little patch?  It should fix your
> problem.  I came up with this fix in my sleep (literally!
> I woke up this morning and the patch was in my head), but
> haven't had time to look at the code in this area to see
> if it's the best fix.
> 

lfs [ /lfs-source-archive/coreutils-8.7-new/src ]$ cat
/lfs-source-archive/cracklib-words-20080507 | sort -u > /dev/null; echo $?
0
lfs [ /lfs-source-archive/coreutils-8.7-new/src ]$

Appears to work as expected.  Thanks for jumping on this so quickly.

-- DJ Lucas

-- 
This message has been scanned for viruses and
dangerous content, and is believed to be clean.