The OutputStream race-condition bug; discovered but not yet solved

[Tim Conkling]

Hello audiere folks,

I'm cross-posting this to audiere-users and audiere-devel -- I sent my first three posts on the topic to audiere-users, but I'm fairly certain this is a bug in audiere itself, so it seems like it might be more appropriate for audiere-devel.

The problem: audiere is causing my program to crash. I'm fairly certain this is due to a race-condition: specifically, RefImplementation<> doesn't synchronize modifications to its m_ref_count member variable, making it possible (for instance) for one thread to dispose a RefCounted object -- such as an OutputStream -- just as another thread is ref()'ing the object for its own use.

Calling stop() right before an OutputStream is unref()'d is one example of when this becomes a problem. The result of calling OutputStream::stop() is that two StopEventImpl objects will be created -- one in the thread that called stop() (the main thread), and one in the audiere update loop thread (this may be a bug, I'm not sure, but I do know that two StopEventImpl's are created when an OutputStream is stop()'d in the example program below). StopEventImpl objects have an OutputStreamPtr member variable, so the OutputStream that's being stop()'d will be ref()'d and unref()'d by the StopEventImpl. This is fine for the StopEventImpl that's created in the main thread, but not for the one that's created in the audiere update loop thread, since -- as mentioned before -- RefCounted objects don't synchronize their modifications to the m_ref_count member var.

So, every once in a while, my program will crash as a StopEventImpl in the update thread clashes with an unref of an OutputStream in my main thread.

Here is a sample program that reliably produces the crash (you will, of course, need to change the sound name). The program probably won't crash on its first few iterations through the loop, but it will eventually happen.

<code>

#include "audiere.h"
#include <cassert>

const char* kSoundName = "test.ogg";

int _tmain(int argc, _TCHAR* argv[])
{
    audiere::AudioDevicePtr device = audiere::OpenDevice();
    assert(NULL != device);

    audiere::OutputStreamPtr outputStream = NULL;
    while(1)
    {
        if(NULL != outputStream)
        {
            outputStream->stop();
        }

        outputStream = audiere::OpenSound(device, kSoundName);
        assert(NULL != outputStream);

        outputStream->play();
    }

    return 0;
}

</code>

This happens with the latest stable version of audiere ( 1.9.3) as well as the version in CVS. I'm using Visual Studio 2003 on Windows XP. I've searched the audiere lists archives and haven't seen any mention of this problem, which seems strange and makes me think that maybe I'm using audiere incorrectly -- but regardless, that RefPtr<> implementation really looks wrong...

I've been looking at thread-safe C++ RefPtr implementations online (glibmm seems to have a robust one, but looking at the source made my head explode). I haven't done a whole lot of multithreaded programming, so this isn't really my specialty.

Any thoughts?

Thanks,
Tim

[Chad Austin]

Please allow me to reply inline.

Tim Conkling wrote:

> Hello audiere folks,
>
> I'm cross-posting this to audiere-users and audiere-devel -- I sent my
> first three posts on the topic to audiere-users, but I'm fairly certain
> this is a bug in audiere itself, so it seems like it might be more
> appropriate for audiere-devel.
>
> The problem: audiere is causing my program to crash. I'm fairly certain
> this is due to a race-condition: specifically, RefImplementation<>
> doesn't synchronize modifications to its m_ref_count member variable,
> making it possible (for instance) for one thread to dispose a RefCounted
> object -- such as an OutputStream -- just as another thread is ref()'ing
> the object for its own use.

If that particular situation is possible, then something is wrong.  Herb Sutter
had an article about it in C++ Users Journal, but the gist is that how can one
thread be ref()'ing when another is releasing the only reference to the object?
  If two threads have a strong reference to an object, the reference count
should be at least two.

> Calling stop() right before an OutputStream is unref()'d is one example
> of when this becomes a problem. The result of calling
> OutputStream::stop() is that two StopEventImpl objects will be created
> -- one in the thread that called stop() (the main thread), and one in
> the audiere update loop thread (this may be a bug, I'm not sure, but I
> do know that two StopEventImpl's are created when an OutputStream is
> stop()'d in the example program below). StopEventImpl objects have an
> OutputStreamPtr member variable, so the OutputStream that's being
> stop()'d will be ref()'d and unref()'d by the StopEventImpl. This is
> fine for the StopEventImpl that's created in the main thread, but not
> for the one that's created in the audiere update loop thread, since --
> as mentioned before -- RefCounted objects don't synchronize their
> modifications to the m_ref_count member var.
>
> So, every once in a while, my program will crash as a StopEventImpl in
> the update thread clashes with an unref of an OutputStream in my main
> thread.

Nothing obviously wrong seems to stand out to me, but here are some simple
changes you can try:

In RefImplementation, change 'int m_ref_count' to 'volatile int m_ref_count'.
It's not a real solution, but it sure is easy.  ;)

Second, you could try using your platform's atomic integer functions.  I believe
on Win32 these are called InterlockedIncrement and InterlockedDecrement.

Otherwise, we'll have to try a bit harder...

> Here is a sample program that reliably produces the crash (you will, of
> course, need to change the sound name). The program probably won't crash
> on its first few iterations through the loop, but it will eventually
> happen.
>
> <code>
>
> #include "audiere.h"
> #include <cassert>
>
> const char* kSoundName = "test.ogg";
>
> int _tmain(int argc, _TCHAR* argv[])
> {
>     audiere::AudioDevicePtr device = audiere::OpenDevice();
>     assert(NULL != device);
>
>     audiere::OutputStreamPtr outputStream = NULL;
>     while(1)
>     {
>         if(NULL != outputStream)
>         {
>             outputStream->stop();
>         }
>
>         outputStream = audiere::OpenSound(device, kSoundName);
>         assert(NULL != outputStream);
>
>         outputStream->play();
>     }
>
>     return 0;
> }
>
> </code>
>
> This happens with the latest stable version of audiere ( 1.9.3) as well
> as the version in CVS. I'm using Visual Studio 2003 on Windows XP. I've
> searched the audiere lists archives and haven't seen any mention of this
> problem, which seems strange and makes me think that maybe I'm using
> audiere incorrectly -- but regardless, that RefPtr<> implementation
> really looks wrong...

What about it looks wrong?

> I've been looking at thread-safe C++ RefPtr implementations online
> (glibmm seems to have a robust one, but looking at the source made my
> head explode). I haven't done a whole lot of multithreaded programming,
> so this isn't really my specialty.
>
> Any thoughts?
>
> Thanks,
> Tim


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
_______________________________________________
Audiere-users mailing list
Audiere-users@...
https://lists.sourceforge.net/lists/listinfo/audiere-users

[Tim Conkling]

On 12/12/05, Chad Austin <aegis@... > wrote:

> The problem: audiere is causing my program to crash. I'm fairly certain
> this is due to a race-condition: specifically, RefImplementation<>
> doesn't synchronize modifications to its m_ref_count member variable,
> making it possible (for instance) for one thread to dispose a RefCounted
> object -- such as an OutputStream -- just as another thread is ref()'ing
> the object for its own use.

If that particular situation is possible, then something is wrong.  Herb Sutter
had an article about it in C++ Users Journal, but the gist is that how can one
thread be ref()'ing when another is releasing the only reference to the object?
  If two threads have a strong reference to an object, the reference count
should be at least two.

I think I was incorrect in my analysis of the problem (as I said, multithreaded programming is not something I have lots of experience with). The problem _may be_ that multiple threads can have an OutputStream*, but not all threads have necessarily wrapped that OutputStream* in a ref-counted OutputStreamPtr.

Using the sample program that I provided, I put a breakpoint in device.cpp, at the entry point to the AbstractDevice::fireStopEvent function. This function's signature is:

void AbstractDevice::fireStopEvent(OutputStream* stream, StopEvent::Reason reason);

After I call OutputStream::stop() on my OutputStream in my program, this breakpoint gets hit twice for the same OutputStream. It gets hit once in the main thread, as a direct result of the call to OutputStream::stop(). The callstack is:

>    audiere.dll!audiere::AbstractDevice::fireStopEvent(audiere::OutputStream * stream=0x01337f38, audiere::StopEvent::Reason reason=STOP_CALLED)  Line 81    C++
     audiere.dll!audiere::DSOutputBuffer::stop ()  Line 69    C++
     audieretest.exe!main(int argc=1, char * * argv=0x00b137d8)  Line 18 + 0x22    C++
     audieretest.exe!mainCRTStartup()  Line 259 + 0x19    C


 It happens again (for the same OutputStream) in a separate audiere-spawned thread. The callstack is:

     >    audiere.dll!audiere::AbstractDevice::fireStopEvent(audiere::OutputStream * stream=0x01337f38, audiere::StopEvent::Reason reason=STREAM_ENDED)  Line 81    C++
     audiere.dll!audiere::DSOutputBuffer::update ()  Line 175    C++
     audiere.dll!audiere::DSAudioDevice::update()  Line 172    C++
     audiere.dll!audiere::ThreadedDevice::run()  Line 332 + 0x25    C++
     audiere.dll!audiere::ThreadedDevice::threadRoutine (void * arg=0x00b061f0)  Line 347    C++
     audiere.dll!audiere::InternalThreadRoutine(void * opaque=0x00b063f8)  Line 76 + 0xe    C++
     audiere.dll!_threadstartex(void * ptd=0x00b06568)  Line 241 + 0xd    C

(Notice that the address of the OutputStream* is the same for both calls).

In the main thread, I unref the OutputStream immediately after calling stop by assigning a new value to its OutputStreamPtr (for reference, here's the code again):

if(NULL != outputStream)
 {
     outputStream->stop();
}
outputStream = audiere::OpenSound(device, kSoundName);

So, immediately after calling stop(), fireStopEvent() is called on the main thread. Some time later, the following two things happen: the OutputStream is unref()'d in the main thread, and fireStopEvent() is called in another thread. When the OutputStream is unref()'d in the main thread, its refcount is decremented from 1 to 0, and it is therefore disposed of. This can occur before fireStopEvent() is called in the other thread, and thus cause crashes.

This other thread is iterating a list of DSOutputBuffer*'s (DSOutputBuffer is a particular implementation OutputStream, and happens to be the implementation that is created by my test program via audiere::OpenSound() -- presumably because my sounds are being buffered to memory instead of streamed from disk) called m_open_buffers (from device_ds.cpp):

BufferList::iterator j = m_open_buffers.begin();
      while (j != m_open_buffers.end()) {
        DSOutputBuffer* b = *j++;
        b->update();
}

The DSOutputBuffer*'s in m_open_buffers aren't ref()'d -- as far as I can tell -- when they are originally inserted into the list. So when the audiere thread is iterating the m_open_buffers, it may be iterating buffers that are in the process of being disposed after having their refcounts reduced to 0 (though it appears that the buffers can't be completely disposed while they're being iterated, because DSOutputBuffer's destructor removes itself from the m_open_buffers list, and the removeBuffer() function is synchronized).

I apologize for turning this email into a sort of stream of consciousness -- I was playing with my test program and discovering new things as I wrote it. I believe my analysis of the problem is more informed this time around. My previous assertion that the RefCounted class is flawed because it doesn't synchronize its modifications to m_ref_count doesn't seem to be correct; instead, the problem may lie in the fact that OutputStreams can become completely unref()'d -- and therefore disposed -- while they still exist in the m_open_buffers list.

Are any of my thoughts correct?

Thanks,
Tim