sp_counted_base_pt.hpp: appears to suffer a race conditions

[Jeffrey Walton <noloader@…>]

It appears release() suffers a race condition:

pthread_mutex_lock( &m_ );
long new_use_count = --use_count_;
pthread_mutex_unlock( &m_ );

if( new_use_count == 0 )
{
  dispose();
  weak_release();
}

In a multithreaded and/or multicore environment, its not hard to imagine Thread A beginning a dispose()/weak_release(), Thread A preemption, and then Thread B acquiring a reference.

multicore is more interesting, as you might need a memory barrier between the locked read of new_use_count and the subsequent test of new_use_count in the 'if' statement.

For add_ref_copy(), imagine a scenario where Thread A reads use_count, Thread A preemption, Thread B read/increment/write use_count, then Thread A increcemt/write use_count:

void add_ref_copy()
{
  ++use_count_;
}

weak_add_ref() and weak_release() suffer similar problems:

void weak_add_ref() // nothrow
{
  ++weak_count_;
}

and

void weak_release() // nothrow
{
  if( --weak_count_ == 0 )
  {
    destroy();
  }
}

[Steven Watanabe]

In release, new_use_count is a local variable, so access to it doesn't need to be protected. If it is 0, then we're destroying the last shared_ptr. Thus, no other thread can modify use_count.

You're comments on the other functions are not applicable, because you've left out the pthread_mutex_[un]lock calls.

[Jeffrey Walton <noloader@…>]

Test program

[Jeffrey Walton <noloader@…>]

jeffrey$ g++ -g -O2 -D_REENTRANT=1 sp-test.cpp -o sp-test.exe -lpthread
jeffrey$ valgrind --tool=helgrind ./sp-test.exe ==19611== Helgrind, a thread error detector
==19611== Copyright (C) 2007-2010, and GNU GPL'd, by OpenWorks LLP et al.
==19611== Using Valgrind-3.6.1 and LibVEX; rerun with -h for copyright info
==19611== Command: ./sp-test.exe
==19611== 
==19611== Thread #3 was created
==19611==    at 0x58E16CE: clone (clone.S:77)
==19611==    by 0x4E37172: pthread_create@@GLIBC_2.2.5 (createthread.c:75)
==19611==    by 0x4C2CA4A: pthread_create_WRK (hg_intercepts.c:257)
==19611==    by 0x4C2CB5E: pthread_create@* (hg_intercepts.c:288)
==19611==    by 0x401669: main (sp-test.cpp:64)
==19611== 
==19611== Thread #2 was created
==19611==    at 0x58E16CE: clone (clone.S:77)
==19611==    by 0x4E37172: pthread_create@@GLIBC_2.2.5 (createthread.c:75)
==19611==    by 0x4C2CA4A: pthread_create_WRK (hg_intercepts.c:257)
==19611==    by 0x4C2CB5E: pthread_create@* (hg_intercepts.c:288)
==19611==    by 0x401669: main (sp-test.cpp:64)
==19611== 
==19611== Possible data race during read of size 8 at 0x603848 by thread #3
==19611==    at 0x401500: WorkerThreadProc(void*) (shared_count.hpp:223)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611==  This conflicts with a previous write of size 8 by thread #2
==19611==    at 0x40152C: WorkerThreadProc(void*) (shared_count.hpp:263)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611== 
==19611== Possible data race during read of size 8 at 0x603840 by thread #3
==19611==    at 0x401507: WorkerThreadProc(void*) (shared_ptr.hpp:169)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611==  This conflicts with a previous write of size 8 by thread #2
==19611==    at 0x401525: WorkerThreadProc(void*) (move.h:83)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611== 
==19611== Possible data race during read of size 8 at 0x603618 by thread #3
==19611==    at 0x40151E: WorkerThreadProc(void*) (shared_count.hpp:262)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611==  This conflicts with a previous write of size 8 by thread #2
==19611==    at 0x40152C: WorkerThreadProc(void*) (shared_count.hpp:263)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611== 
==19611== Possible data race during write of size 8 at 0x603610 by thread #3
==19611==    at 0x401525: WorkerThreadProc(void*) (move.h:83)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611==  This conflicts with a previous write of size 8 by thread #2
==19611==    at 0x401525: WorkerThreadProc(void*) (move.h:83)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611== 
==19611== Possible data race during write of size 8 at 0x603618 by thread #3
==19611==    at 0x40152C: WorkerThreadProc(void*) (shared_count.hpp:263)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611==  This conflicts with a previous write of size 8 by thread #2
==19611==    at 0x40152C: WorkerThreadProc(void*) (shared_count.hpp:263)
==19611==    by 0x4C2CBE7: mythread_wrapper (hg_intercepts.c:221)
==19611==    by 0x4E369C9: start_thread (pthread_create.c:300)
==19611==    by 0x58E170C: clone (clone.S:112)
==19611== 
==19611== 
==19611== For counts of detected and suppressed errors, rerun with: -v
==19611== Use --history-level=approx or =none to gain increased speed, at
==19611== the cost of reduced accuracy of conflicting-access information
==19611== ERROR SUMMARY: 56968 errors from 5 contexts (suppressed: 8509 from 13)

[Peter Dimov]

Your test code has a data race on blocks[idx]. This usage is not supported by shared_ptr, it is as thread-safe as an int or an ordinary pointer. If you change BlockPtr to be Block*, valgrind ought to complain as well.