String::detach() not thread-safe?

Kevin André hyperquantum at gmail.com
Sat Nov 15 12:59:06 UTC 2014


I recently had an unexplained crash in my music application; the stack
trace pointed somewhere in taglib, but I didn't have debug symbols
available so I couldn't pinpoint the exact location. The problem was
not reproducible; running the program again with the same input did
not cause another crash. Recently I had moved the scanning of music
files in my application into separate threads (using QThreadPool and
QRunnable), so I suspect this is some kind of race condition.

With some googling I learned that multithreading issues in taglib were
discussed before, and that atomic operations were introduced to the
RefCounter class. Those issues were about String::null, so I took
another look at the String implementation. And I think there is a
problem with detach():

void String::detach()
{
  if(d->count() > 1) {
    d->deref();
    d = new StringPrivate(d->data);
  }
}

This code is from the 1.9.1 release, but the master branch GitHub
shows the same implementation.
The first problem I noticed was that IMO the two statements inside the
'if' should be swapped:

void String::detach()
{
  if(d->count() > 1) {
    d = new StringPrivate(d->data);
    d->deref();
  }
}

... because after the call to deref(), but before the copying of the
data, some other thread(s) could also call deref() and one of the
following things could happen:

 * the reference count reaches zero and the data is deleted before it
gets copied
 * the reference count reaches one in another thread before the data
is copied, so that other thread decides to modify the data without
making its own copy, because it thinks it owns it

Swapping those two statements should solve those problems, but then I
thought of another problem. What if the count equals 1 so the body of
the 'if' is not executed, but then another thread causes the count to
increase again right before the caller of detach() starts modifying
the data?! To fix this, one would have to start using a mutex I guess.
But that would of course decrease performance.

Thoughts?


More information about the taglib-devel mailing list