TagLib::String::null considered harmful

[Peter Hartley]

TagLib (1.4 and 1.5) is nearly, but not quite, thread-safe, in the sense
of being able to read the metadata from two different files in two
different threads at the same time without them interfering. (With a
multi-core CPU or with good disk queueing, this should be faster than
the single-threaded solution when scanning a large library.)

AFAICS, the only thing stopping it being thread-safe (in that sense) is
the way that just one String::null, and just one RefCounter belonging to
String::null, is shared between all threads.

Consider what happens when (say) ID3v2::Tag::artist() finds the artist
is unset: it returns String::null, which increments the reference count
on String::null's StringPrivate. But there's nothing protecting that
increment operation against racing operations on other threads, so the
count can get out of step with reality, and eventually the StringPrivate
could get freed before its time, causing crashes.

Currently I work around this by having a big fat mutex lock round all my
calls to tag->artist() and friends, but IMO it would be better (and more
efficient) if the library didn't have this shortcoming in the first
place.

Three alternative fixes come to mind, though I don't have a patch for
any of them:

- Put a mutex in TagLib::RefCounter. This seems like overkill, as the
  overwhelming majority of strings are never contended over, and would
  slow down the single-threaded case.

- Never delete the StringPrivate of String::null, like so:
    String::~String()
    {
      if(d->deref() && d != null.d)
        delete d;
    }
  which is a gruesome hack, and a (small, finite) memory leak, but would
  solve the actual problem.

- Arrange for String::null to have no StringPrivate at all, which is
  less hacky but does mean extra special-case code checking "d != NULL"
  in all the other methods.

	Peter