search

meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.15k stars 2.47k forks source link

Race condition when ending recording and closing stream in VideoRoom plugin #530

Closed andreasg123 closed 8 years ago

andreasg123 commented 8 years ago

There seems to be a race condition when the web browser sends a message to end recording for a publisher in a video room and immediately after closes the WebRTC stream. It crashes the server in janus_recorder_free (see stack trace). The log file indicates that the "configure" request for turning off recording is received first and then the DTLS alert. After the DTLS alert, the audio recording is closed with a file name. Immediately after, it is closed again, this time without a file name.

My colleague set up that server and he runs this version of Janus:

commit 5f1437cf49d8051a6021a0332e2fac425716fb44
Merge: 31ae4f2 85f4116
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Sat Apr 23 00:40:02 2016 +0200
lminiero commented 8 years ago

Thanks for looking into this! I guess the same issue might occur in other plugins exploiting the recording functionality as well. I'll have a look at this tomorrow, when I get back to the office.

andreasg123 commented 8 years ago

I believe that #531 should fix this for the VideoRoom plugin.

The SIP plugin should have the same potential problem (recorder being freed in response to "recording" request and hangup). janus_sip_session has the member mutex that could be used to protect the recorders. Currently, it isn't used anywhere.

The VideoCall plugin also has the same pattern (freeing recorder in "set" request and hangup). There is no mutex in janus_videocall_session.

The Streaming plugin frees the recorder in the "stop" and "disable" requests. There is no mutex in janus_streaming_rtp_source.

The EchoTest plugin frees the recorder in hangup and a request "record": false. There is no mutex in janus_echotest_session.

I don't think that the Record&Play plugin is affected because the recorder is only freed in a "stop" request. However, it may be possible that two "stop" requests would arrive together. janus_recordplay_session currently does not include a mutex.

It might be best to give all session structures that include recorders a member recorder_mutex that is used to protect the recorders. As the mutex in the SIP plugin currently isn't used, it could be renamed or you could use the generic member mutex everywhere for this purpose.