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

[Jim Meyering]

Jim Meyering wrote:
> 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
>> <http://lists.gnu.org/archive/html/bug-coreutils/2010-11/msg00216.html>
>> 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);
>>      }
>>  }
>
> Nice.
> That looks right to me.
>
> FYI, what happens is the first fillbuf call gets a block
> of lines, and "saved" ends up pointing at one of them.
> The second fillbuf's fread races to overwrite a byte or two of the
> saved->text pointer that is being dereferenced by the other thread.
...

Hi Paul,

I'm getting ready to push something like the following
(I'll add a NEWS entry first, though)
Is there anything you'd like to add?

>From 46589f6be5ce6cd7ff474059a33f47b57094ff21 Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Tue, 30 Nov 2010 22:30:12 +0100
Subject: [PATCH] sort -u: fix a thread-race pointer corruption bug

* src/sort.c (write_unique): Save the entire "struct line", not
just a pointer to one.  Otherwise, with a multi-thread run,
sometimes, with some inputs, fillbuf would would win a race
and clobber a "saved->text" pointer in one thread just before
it was dereferenced in a comparison in another thread.
---
 src/sort.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

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);
     }
 }
--
1.7.3.2.846.gf4b062