String::detach() not thread-safe?

[Tsuda Kageyu]

Hi Kevin,

I suggested a pull request that will fix it:
https://github.com/taglib/taglib/pull/433


2014-11-15 21:59 GMT+09:00 Kevin André <hyperquantum at gmail.com>:
> 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?
> _______________________________________________
> taglib-devel mailing list
> taglib-devel at kde.org
> https://mail.kde.org/mailman/listinfo/taglib-devel