Concurrency problem when destroying stream (thanks Michael Broughton)

[bennylp]

​Michael Broughton wrote in pjsip list:

I am looking at two functions in stream.c: pjmedia_stream_destroy and on_rx_rtp. I am trying to figure out if the following scenario is possible:

1. Thread A calls pjmedia_stream_destroy, which locks stream->jb_mutex.

2. Thread B calls on_rx_rtp, which tries to lock stream->jb_mutex but cannot right away because thread A already holds the lock.

3. Thread A goes about it's work and then free's stream->jb_mutex.

4. Thread B, still trying to lock stream->jb_mutex, segfaults because thread A deallocated stream->jb_mutex and zeroed out the pointer.

[bennylp]

Additional note:

• also currently on_dtmf_callback()is called without holding the stream's mutex. This may cause concurrency problem when stream is being destroyed at the same time.

[bennylp]

Done in r1790. This would also fix ticket #469.

Please note that most of the concurrency protection is done in the ioqueue, see ticket #474 for the details.

[bennylp]

​Additional discussions regarding on_dtmf_callback() concurrency:

We have three cases to handle:

First is processing DTMF callback while other thread is trying to destroy the stream/call. This scenario is the same as our original stream problem, so it should have been fixed by the latest patch. We wouldn't even get into the soft deadlock state as on_dtmf() callback, so we should be safe.

(this still assumes current situation where on_dtmf() callback doesn't hold any call mutexes. Once we change on_dtmf() to acquire call mutex, then yes, this would be covered by acquire_call(), i.e. one of the thread will wait until the other is done

Although, hmmm.. thinking about it again, if we implement call locking in on_dtmf() callback, actually we will get into deadlock, as media transport mutex is not included in the standard lock order!

So maybe having call lock in on_dtmf() callback is not a good idea after all).

The second case is destroying the call in the DTMF callback itself, but leaving the UDP transport alive. This scenario also should be safe.

The third case is destroying the call *and* the UDP transport in the DTMF callback. The difference between this and scenario 2 is when the UDP transport is destroyed, so is the pool associated with it, so all memory belonging to the UDP transport will be invalid after UDP transport is destroyed.

For the third scenario, it was not safe to do that, but this should have been fixed by ​http://trac.pjsip.org/repos/ticket/469#comment:2, I think.