Thread safety for ID3v1 genres

Stephen F. Booth me at sbooth.org
Mon May 2 13:27:18 CEST 2011


>> I've been experiencing a crash that I can reproduce 100% of the time,
>> but I'm struggling on coming up with a good fix.
>
> In theory the new atomic ref counting in trunk should solve this.  Can you see if you can still reproduce it with trunk?

I should have mentioned this in my original message- I am using taglib
trunk on Mac OS X10.6.7 and gcc 4.2.1.

Perhaps I'm missing something obvious, but as far as I can tell there
is no regard to refcounting in toCString.  The entire method body
consists of:

const char *String::toCString(bool unicode) const
{
  delete [] d->CString;

  std::string buffer = to8Bit(unicode);
  d->CString = new char[buffer.size() + 1];
  strcpy(d->CString, buffer.c_str());

  return d->CString;
}

So the first line of code executed, regardless of the string's
refcount, deletes the old buffer.  Thus any string object held by more
than one thread can't be trusted to give a pointer with a guaranteed
lifetime from toCString.

>>     static const int genresSize = 148;
>> -    static const String genres[] = {
>> +    static const char * genres[] = {
>>       "Blues",
>
> This is actually one of those things that I've mentioned that if TagLib wants to be truly reentrant that the static initializers (which there are a fair number of) will need to be possible to invoke manually, mutexed or to go away.  While I doubt the initialization the problem there, the function is fundamentally not reentrant since there's a possible race condition on the first call to the function.

I still would suggest the patch above, because no C++ objects are
being created at static initialization time.

Stephen


More information about the taglib-devel mailing list