GNOME Bugzilla – Bug 790508
srtpenc calling srtp_protect_rtcp after session is deallocated
Last modified: 2018-05-18 09:41:35 UTC
Hello, I've run into an issue where my program is getting a segfault in libsrtp when the srtpenc plugin is calling the srtp_protect_rtcp() function. From debugging, it looks like this function is sometimes being called after the filter->session has already been freed with the srtp_dealloc() function. The srtp_dealloc() function does not appear to set the struct's internal pointers to NULL (both filter->session->stream_list and filter->session->stream_template). Rather, the internal pointers are left pointing to memory that has been freed, so that when the srtp_protect_rtcp() function is eventually called, it tries to access these structures because they are not NULL. This could be a bug in my older version of libsrtp (1.4.5), but I was able to get around the issue by setting both the stream_list and stream_template pointers to NULL after the srtp_deallocate() call in gstsrtpenc.c. I can provide a patch if that makes sense. Thanks, Jake
Is this something not covered by https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/ext/srtp?id=029e01743f8786c23197d9697b89e9bc3eacfdf3 ?
I tried the changes you provided and the issue still occurs. libsrtp's srtp_protect_rtcp() function does not do a check if the srtp context (filter->session) is NULL, but it does check if the stream_list (filter->session->stream_list) or stream_template are NULL. We could expand your changes to add a check if filter->session is NULL before we attempt to call srtp_protect_rtcp() and srtp_protect().
Thanks for checking. Can you provide a patch?
Created attachment 364112 [details] [review] srtpenc patch with NULL check Check that the session hasn't been deallocated (NULL) before calling the srtp_protect functions.
Does this maybe also need a mutex somewhere?
I think it is fine. It looks like the srtp_dealloc and srtp_protect calls are always guarded by a lock on the 'filter' object.
Comment on attachment 364112 [details] [review] srtpenc patch with NULL check So it seems, thanks!
Review of attachment 364112 [details] [review]: ::: ext/srtp/gstsrtpenc.c @@ +1045,3 @@ + err = srtp_protect_rtcp (filter->session, mapout.data, &size); + else + err = srtp_protect (filter->session, mapout.data, &size); Actually this does not seem correct. If there is no session, we would now send data unencrypted, which is probably not the intention here, ever. It should probably cause an error instead, or GST_FLOW_FLUSHING. When exactly can this happen, what are all the situations?
Yes, an error would make sense. I'm only able to reproduce this problem in my application when I stress-test it by rapidly creating and destroying pipelines that use this element. Seems like it is an edge case or race condition.
AFAICS this can only happen during shutdown, in which case this should return GST_FLOW_FLUSHING
I fixed this a bit more comprehensively, so that failures are reflected into the upstream GstFlowReturn, returning GST_FLOW_FLUSHING if the session object disappeared. srtpenc: Handle session object disappearing During element shutdown, the srtp encryption session object can be cleaned up. In that case, return GST_FLOW_FLUSHING from the chain function. Also properly return GST_FLOW_ERROR upstream during actual errors. https://bugzilla.gnome.org/show_bug.cgi?id=790508