Re: [PATCH] sort: fix two race conditions reported by valgrind.

[Pádraig Brady]

On 01/14/2014 12:49 AM, Shayan Pooya wrote:
> 
> 
> 
> On Mon, Jan 13, 2014 at 7:31 PM, Pádraig Brady <address@hidden 
> <mailto:address@hidden>> wrote:
> 
>     On 01/14/2014 12:22 AM, Shayan Pooya wrote:
>     > Valgrind reported two race conditions when I ran sort on a small file.
>     > Both of them seem to be legitimate.
>     > ---
>     >  src/sort.c | 4 ++--
>     >  1 file changed, 2 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/src/sort.c b/src/sort.c
>     > index 3380be6..e6658e6 100644
>     > --- a/src/sort.c
>     > +++ b/src/sort.c
>     > @@ -3354,8 +3354,8 @@ queue_insert (struct merge_node_queue *queue, 
> struct merge_node *node)
>     >    pthread_mutex_lock (&queue->mutex);
>     >    heap_insert (queue->priority_queue, node);
>     >    node->queued = true;
>     > -  pthread_mutex_unlock (&queue->mutex);
>     >    pthread_cond_signal (&queue->cond);
>     > +  pthread_mutex_unlock (&queue->mutex);

valgrind is not flagging the above for me?

>     >  }
>     >
>     >  /* Pop the top node off the priority QUEUE, lock the node, return it.  
> */
>     > @@ -3950,7 +3950,7 @@ sort (char *const *files, size_t nfiles, char 
> const *output_file,
>     >                sortlines (line, nthreads, buf.nlines, root,
>     >                           &queue, tfp, temp_output);
>     >                queue_destroy (&queue);
>     > -              pthread_mutex_destroy (&root->lock);
>     > +              pthread_mutex_destroy (&merge_tree->lock);

Right this doesn't look right. That issue is noticed by valgrind 3.8.1 and 
3.9.0,
and I'd probably fix by making merge_tree_{init,destroy} symmetrical as follows.
I'm not sure how this deinit code might cause an issue in practice though.

diff --git a/src/sort.c b/src/sort.c
index 3380be6..c6ecdab 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -3239,6 +3239,8 @@ merge_tree_init (size_t nthreads, size_t nlines, struct 
line *dest)
 static void
 merge_tree_destroy (struct merge_node *merge_tree)
 {
+  struct merge_node *root = merge_tree;
+  pthread_mutex_destroy (&root->lock);
   free (merge_tree);
 }

@@ -3950,7 +3952,6 @@ sort (char *const *files, size_t nfiles, char const 
*output_file,
               sortlines (line, nthreads, buf.nlines, root,
                          &queue, tfp, temp_output);
               queue_destroy (&queue);
-              pthread_mutex_destroy (&root->lock);
               merge_tree_destroy (merge_tree);
             }
           else

>     >                merge_tree_destroy (merge_tree);
>     >              }
>     >            else
> 
>     Hi, what version of valgrind, command line options,
>     and sort input files did you use.
> 
>     thanks,
>     Pádraig.
> 
> 
> 
> Hello,
> 
> Valgrind 3.9
> The input is the output of: seq 1000 > ~/1K
> The command line options: none:
> $ valgrind --tool=drd ./coreutils/src/srot ~/1K -o ~/1K
> 
> There are some other race conditions reported by DRD for larger files (1M 
> lines) that I am investigating.
> 
> P.S.
> I am tracking down this issue: http://stackoverflow.com/q/21099839/830681

How reproducible is this lockup for you?

thanks,
Pádraig.